[PATCH rdma-core] verbs: Fix C++ compilation break

[PATCH rdma-core] verbs: Fix C++ compilation break

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The commit 983f80191923 ("verbs: fix compilation error with ICC") fixed
warning by using UINTPTR_MAX, however such change breaks compilation
of C++ applications.

In C++ world, the UINTPTR_MAX is declared in <cstdint> and not stdint.h,
so in order to avoid messing with various defines to decide which header
file include: stdint.h or <cstdint>, we will check for the existence
of UINTPTR_MAX and will fallback to old implementation.

Fixes: 983f80191923 ("verbs: fix compilation error with ICC")
Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Nelio Laranjeiro <nelio.laranjeiro-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
Cc: Adrien Mazarguil <adrien.mazarguil-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
Cc: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 libibverbs/verbs.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
index cc633a12..f540b660 100644
--- a/libibverbs/verbs.h
+++ b/libibverbs/verbs.h
@@ -82,7 +82,11 @@ union ibv_gid {

 #define vext_field_avail(type, fld, sz) (offsetof(type, fld) < (sz))

+#ifdef UINTPTR_MAX
 static void *__VERBS_ABI_IS_EXTENDED = (void *)UINTPTR_MAX;
+#else
+static void *__VERBS_ABI_IS_EXTENDED = ((uint8_t *) NULL) - 1;
+#endif

 enum ibv_node_type {
 	IBV_NODE_UNKNOWN	= -1,
--
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] verbs: Fix C++ compilation break

[Nicolas Morey-Chaisemartin]

Le 09/10/2017 à 10:17, Leon Romanovsky a écrit :
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> The commit 983f80191923 ("verbs: fix compilation error with ICC") fixed
> warning by using UINTPTR_MAX, however such change breaks compilation
> of C++ applications.
>
> In C++ world, the UINTPTR_MAX is declared in <cstdint> and not stdint.h,
> so in order to avoid messing with various defines to decide which header
> file include: stdint.h or <cstdint>, we will check for the existence
> of UINTPTR_MAX and will fallback to old implementation.
>
> Fixes: 983f80191923 ("verbs: fix compilation error with ICC")
> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Nelio Laranjeiro <nelio.laranjeiro-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> Cc: Adrien Mazarguil <adrien.mazarguil-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> Cc: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
>  libibverbs/verbs.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
> index cc633a12..f540b660 100644
> --- a/libibverbs/verbs.h
> +++ b/libibverbs/verbs.h
> @@ -82,7 +82,11 @@ union ibv_gid {
>
>  #define vext_field_avail(type, fld, sz) (offsetof(type, fld) < (sz))
>
> +#ifdef UINTPTR_MAX
>  static void *__VERBS_ABI_IS_EXTENDED = (void *)UINTPTR_MAX;
> +#else
> +static void *__VERBS_ABI_IS_EXTENDED = ((uint8_t *) NULL) - 1;
> +#endif
>
>  enum ibv_node_type {
>  	IBV_NODE_UNKNOWN	= -1,
> --
> 2.14.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wouldn't something like this solve the issue without messing up the code ?

diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
index 8cdf8ab5..c5d932bf 100644
--- a/libibverbs/verbs.h
+++ b/libibverbs/verbs.h
@@ -44,6 +44,7 @@
 #include <linux/types.h>
 
 #ifdef __cplusplus
+#include  <cstdint>
 #  define BEGIN_C_DECLS extern "C" {
 #  define END_C_DECLS   }
 #else /* !__cplusplus */

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] verbs: Fix C++ compilation break

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2468 bytes --]

On Mon, Oct 09, 2017 at 03:12:00PM +0200, Nicolas Morey-Chaisemartin wrote:
>
>
> Le 09/10/2017 à 10:17, Leon Romanovsky a écrit :
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > The commit 983f80191923 ("verbs: fix compilation error with ICC") fixed
> > warning by using UINTPTR_MAX, however such change breaks compilation
> > of C++ applications.
> >
> > In C++ world, the UINTPTR_MAX is declared in <cstdint> and not stdint.h,
> > so in order to avoid messing with various defines to decide which header
> > file include: stdint.h or <cstdint>, we will check for the existence
> > of UINTPTR_MAX and will fallback to old implementation.
> >
> > Fixes: 983f80191923 ("verbs: fix compilation error with ICC")
> > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Cc: Nelio Laranjeiro <nelio.laranjeiro-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> > Cc: Adrien Mazarguil <adrien.mazarguil-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> > Cc: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > ---
> >  libibverbs/verbs.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
> > index cc633a12..f540b660 100644
> > --- a/libibverbs/verbs.h
> > +++ b/libibverbs/verbs.h
> > @@ -82,7 +82,11 @@ union ibv_gid {
> >
> >  #define vext_field_avail(type, fld, sz) (offsetof(type, fld) < (sz))
> >
> > +#ifdef UINTPTR_MAX
> >  static void *__VERBS_ABI_IS_EXTENDED = (void *)UINTPTR_MAX;
> > +#else
> > +static void *__VERBS_ABI_IS_EXTENDED = ((uint8_t *) NULL) - 1;
> > +#endif
> >
> >  enum ibv_node_type {
> >  	IBV_NODE_UNKNOWN	= -1,
> > --
> > 2.14.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Wouldn't something like this solve the issue without messing up the code ?
>
> diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
> index 8cdf8ab5..c5d932bf 100644
> --- a/libibverbs/verbs.h
> +++ b/libibverbs/verbs.h
> @@ -44,6 +44,7 @@
>  #include <linux/types.h>
>  
>  #ifdef __cplusplus
> +#include  <cstdint>
>  #  define BEGIN_C_DECLS extern "C" {
>  #  define END_C_DECLS   }
>  #else /* !__cplusplus */
>

It will be enough, I'll resend the patch.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core] verbs: Fix C++ compilation break

[Jason Gunthorpe]

On Mon, Oct 09, 2017 at 11:17:17AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> The commit 983f80191923 ("verbs: fix compilation error with ICC") fixed
> warning by using UINTPTR_MAX, however such change breaks compilation
> of C++ applications.
> 
> In C++ world, the UINTPTR_MAX is declared in <cstdint> and not stdint.h,
> so in order to avoid messing with various defines to decide which
> header

I don't think this statement is right. In C++ land the 'cXXX' headers
and the 'XXX.h' headers are usually the same. On my system I see
nothing that would cause stdint.h to not define the macros in C++ mode:

$ echo t.cc
#include <stdint.h>

enum {foo = UINTPTR_MAX};

$ g++-5 -c -Wall t.cc

So, what environment fails here? It should be described in the
commit message..

The best fix is something like this:

#ifdef __cplusplus
#include <limits>
#define __VERBS_ABI_IS_EXTENDED ((void *)std::numeric_limits<uintptr_t>::max())
#else
#define __VERBS_ABI_IS_EXTENDED ((void *)UINTPTR_MAX)
#endif

The static in a header file also a mistake.

Using <cstdint> is a bit risky since it is a c++11 header and may not
be in some of the older distros we still support.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] verbs: Fix C++ compilation break

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]

On Mon, Oct 09, 2017 at 09:41:04AM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 09, 2017 at 11:17:17AM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > The commit 983f80191923 ("verbs: fix compilation error with ICC") fixed
> > warning by using UINTPTR_MAX, however such change breaks compilation
> > of C++ applications.
> >
> > In C++ world, the UINTPTR_MAX is declared in <cstdint> and not stdint.h,
> > so in order to avoid messing with various defines to decide which
> > header
>
> I don't think this statement is right. In C++ land the 'cXXX' headers
> and the 'XXX.h' headers are usually the same.

Most of the time, it is true, but stdint.h is different and it is
different between C99 and C++11.

> On my system I see
> nothing that would cause stdint.h to not define the macros in C++ mode:
>
> $ echo t.cc
> #include <stdint.h>
>
> enum {foo = UINTPTR_MAX};
>
> $ g++-5 -c -Wall t.cc
>
> So, what environment fails here? It should be described in the
> commit message..

It means that your glibc is new enough.
On some systems, the stdint.h contains the following ifdef to protect UINTPTR_MAX
"#if !defined __cplusplus || defined __STDC_LIMIT_MACROS"

>
> The best fix is something like this:
>
> #ifdef __cplusplus
> #include <limits>
> #define __VERBS_ABI_IS_EXTENDED ((void *)std::numeric_limits<uintptr_t>::max())
> #else
> #define __VERBS_ABI_IS_EXTENDED ((void *)UINTPTR_MAX)
> #endif
>
> The static in a header file also a mistake.
>
> Using <cstdint> is a bit risky since it is a c++11 header and may not
> be in some of the older distros we still support.
>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core] verbs: Fix C++ compilation break

[Jason Gunthorpe]

On Mon, Oct 09, 2017 at 10:05:19PM +0300, Leon Romanovsky wrote:

> It means that your glibc is new enough.
> On some systems, the stdint.h contains the following ifdef to protect UINTPTR_MAX
> "#if !defined __cplusplus || defined __STDC_LIMIT_MACROS"

Ah, that old thing. __STDC_LIMIT_MACROS is removed from the latest
standards, and IIRC, including cstdint isn't even enough to reliably
get the macros under the old standards.

The commit comment should describe the problem in these terms..

So this is the best fix:

> > #ifdef __cplusplus
> > #include <limits>
> > #define __VERBS_ABI_IS_EXTENDED ((void *)std::numeric_limits<uintptr_t>::max())
> > #else
> > #define __VERBS_ABI_IS_EXTENDED ((void *)UINTPTR_MAX)
> > #endif

Because numeric_limits is never subject to __STDC_LIMIT_MACROS

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] verbs: Fix C++ compilation break

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On Mon, Oct 09, 2017 at 01:11:01PM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 09, 2017 at 10:05:19PM +0300, Leon Romanovsky wrote:
>
> > It means that your glibc is new enough.
> > On some systems, the stdint.h contains the following ifdef to protect UINTPTR_MAX
> > "#if !defined __cplusplus || defined __STDC_LIMIT_MACROS"
>
> Ah, that old thing. __STDC_LIMIT_MACROS is removed from the latest
> standards, and IIRC, including cstdint isn't even enough to reliably
> get the macros under the old standards.
>
> The commit comment should describe the problem in these terms..
>
> So this is the best fix:
>
> > > #ifdef __cplusplus
> > > #include <limits>
> > > #define __VERBS_ABI_IS_EXTENDED ((void *)std::numeric_limits<uintptr_t>::max())
> > > #else
> > > #define __VERBS_ABI_IS_EXTENDED ((void *)UINTPTR_MAX)
> > > #endif
>
> Because numeric_limits is never subject to __STDC_LIMIT_MACROS

Thanks, I'll test it on that system and resend.

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]