Skip to contentSkip to conversation

verbs: Fix C++ compilation break

PATCH In review Leon Romanovsky 3 participants 7 messages Oct 9, 2017 5 messages not in the archive InfiniBand

lore
Share & tools
lore

Leon Romanovsky

author
#0 lore raw

PATCH 1/1 verbs: Fix C++ compilation break

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

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/[email protected]>
Cc Nelio Laranjeiro <[email protected]>
Cc Adrien Mazarguil <[email protected]>
Cc Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 libibverbs/verbs.h | 4 ++++
 1 file changed, 4 insertions(+)
libibverbs/verbs.h +4 -0
index cc633a12..f540b660 100644
@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20171009081717.21478-1-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

#2 lore raw

Le 09/10/2017 à 10:17, Leon Romanovsky a écrit :

Quoted 39 lines

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

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/[email protected]>
Cc Nelio Laranjeiro <[email protected]>
Cc Adrien Mazarguil <[email protected]>
Cc Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 libibverbs/verbs.h | 4 ++++
 1 file changed, 4 insertions(+)
libibverbs/verbs.h +4 -1
index cc633a12..f540b660 100644
@@ -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

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Wouldn't something like this solve the issue without messing up the code ?

libibverbs/verbs.h +0 -0
index 8cdf8ab5..c5d932bf 100644
@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 537d72a7-19c3-d13d-4b49-538744a73845-l3A5Bk7waGM@public.gmane.org

author
#4 lore raw
Quoted 54 lines · Nicolas Morey-Chaisemartin

Le 09/10/2017 à 10:17, Leon Romanovsky a écrit :

Quoted 39 lines

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

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/[email protected]>
Cc Nelio Laranjeiro <[email protected]>
Cc Adrien Mazarguil <[email protected]>
Cc Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 libibverbs/verbs.h | 4 ++++
 1 file changed, 4 insertions(+)
libibverbs/verbs.h +4 -1
index cc633a12..f540b660 100644
@@ -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

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Wouldn't something like this solve the issue without messing up the code ?

libibverbs/verbs.h +0 -0
index 8cdf8ab5..c5d932bf 100644
@@ -44,6 +44,7 @@
  #include <linux/types.h>
Quoted 7 lines


 #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

#5 lore raw
In reply to a message not in the archive
Quoted 9 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20171009154104.GA3824-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

author
#7 lore raw
Quoted 7 lines · Jason Gunthorpe
Quoted 6 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

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

Quoted 6 lines
Quoted 3 lines

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.

Quoted 6 lines

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>
Quoted 6 lines

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"

Quoted 16 lines

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

A message that is not in the archive (sent off-list or not yet received): 20171009190519.GD1252-U/[email protected]

#9 lore raw
Quoted 3 lines · Leon Romanovsky

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:

Quoted 6 lines
Quoted 6 lines

#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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20171009191101.GC15336-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

author
#11 lore raw
Quoted 14 lines · Jason Gunthorpe
Quoted 3 lines · Leon Romanovsky

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:

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines

#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.

Quoted 2 lines

Jason