[PATCH rdma-core 0/5] clang support

[PATCH rdma-core 0/5] clang support

[Jason Gunthorpe]

This series gets clang running warning free and has travis run the build
through clang-3.9 so we don't break it again.

Jason Gunthorpe (5):
  Pull uninitialized_var into util/compiler.h
  ibacm: Avoid clang 3.8 warning -Wmissing-field-initializers
  mthca: Avoid bogus gcc 4.8 warning -Wmaybe-uninitialized
  Improve linker flag detection to work with clang and -Werror
  Update TravisCI to use clang 3.9 as well

 .travis.yml                    | 20 +++++++++++++++++---
 CMakeLists.txt                 | 22 ++++++++++++++--------
 buildlib/FindLDSymVer.cmake    | 16 ++++++++++++----
 buildlib/RDMA_EnableCStd.cmake | 25 +++++++++++++++++++++++++
 ibacm/prov/acmp/src/acmp.c     |  2 +-
 providers/cxgb4/cq.c           |  1 +
 providers/cxgb4/libcxgb4.h     |  2 --
 providers/cxgb4/qp.c           |  1 +
 providers/mlx4/mlx4.h          |  4 ----
 providers/mlx4/qp.c            |  1 +
 providers/mlx5/mlx5.h          |  4 ----
 providers/mlx5/qp.c            |  1 +
 providers/mthca/qp.c           | 23 +++++------------------
 util/CMakeLists.txt            |  3 +++
 util/compiler.h                | 18 ++++++++++++++++++
 15 files changed, 99 insertions(+), 44 deletions(-)
 create mode 100644 util/CMakeLists.txt
 create mode 100644 util/compiler.h

-- 
2.1.4

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

[PATCH rdma-core 1/5] Pull uninitialized_var into util/compiler.h

[Jason Gunthorpe]

The new common definition turns it off for new compilers since
it is not needed and is too easy to abuse.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 CMakeLists.txt             |  1 +
 providers/cxgb4/cq.c       |  1 +
 providers/cxgb4/libcxgb4.h |  2 --
 providers/cxgb4/qp.c       |  1 +
 providers/mlx4/mlx4.h      |  4 ----
 providers/mlx4/qp.c        |  1 +
 providers/mlx5/mlx5.h      |  4 ----
 providers/mlx5/qp.c        |  1 +
 util/CMakeLists.txt        |  3 +++
 util/compiler.h            | 18 ++++++++++++++++++
 10 files changed, 26 insertions(+), 10 deletions(-)
 create mode 100644 util/CMakeLists.txt
 create mode 100644 util/compiler.h

diff --git a/CMakeLists.txt b/CMakeLists.txt
index b0864da660fc..1611e90d7933 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -253,6 +253,7 @@ configure_file("${BUILDLIB}/config.h.in" "${BUILD_INCLUDE}/config.h" ESCAPE_QUOT
 #-------------------------
 # Sub-directories
 add_subdirectory(ccan)
+add_subdirectory(util)
 # Libraries
 add_subdirectory(libibumad)
 add_subdirectory(libibumad/man)
diff --git a/providers/cxgb4/cq.c b/providers/cxgb4/cq.c
index 1ed7dfdb88d4..5f662ac49d0c 100644
--- a/providers/cxgb4/cq.c
+++ b/providers/cxgb4/cq.c
@@ -37,6 +37,7 @@
 #include <sys/errno.h>
 #include <netinet/in.h>
 #include <infiniband/opcode.h>
+#include <util/compiler.h>
 #include "libcxgb4.h"
 #include "cxgb4-abi.h"
 
diff --git a/providers/cxgb4/libcxgb4.h b/providers/cxgb4/libcxgb4.h
index 4c8383209287..9a4bc98f34e9 100644
--- a/providers/cxgb4/libcxgb4.h
+++ b/providers/cxgb4/libcxgb4.h
@@ -264,6 +264,4 @@ void dump_state();
 extern int stall_to;
 #endif
 
-#define uninitialized_var(x) x = x
-
 #endif				/* IWCH_H */
diff --git a/providers/cxgb4/qp.c b/providers/cxgb4/qp.c
index 384bf11369ac..95b459a1a9b4 100644
--- a/providers/cxgb4/qp.c
+++ b/providers/cxgb4/qp.c
@@ -37,6 +37,7 @@
 #include <string.h>
 #include <stdio.h>
 #include <netinet/in.h>
+#include <util/compiler.h>
 #include "libcxgb4.h"
 
 #ifdef STATS
diff --git a/providers/mlx4/mlx4.h b/providers/mlx4/mlx4.h
index 95a6521c457b..a2d39e169c15 100644
--- a/providers/mlx4/mlx4.h
+++ b/providers/mlx4/mlx4.h
@@ -43,10 +43,6 @@
 
 #define MLX4_PORTS_NUM 2
 
-#ifndef uninitialized_var
-#define uninitialized_var(x) x = x
-#endif
-
 #include <valgrind/memcheck.h>
 
 #define PFX		"mlx4: "
diff --git a/providers/mlx4/qp.c b/providers/mlx4/qp.c
index 4b5acd71108e..268fb7dc83dd 100644
--- a/providers/mlx4/qp.c
+++ b/providers/mlx4/qp.c
@@ -39,6 +39,7 @@
 #include <pthread.h>
 #include <string.h>
 #include <errno.h>
+#include <util/compiler.h>
 
 #include "mlx4.h"
 #include "doorbell.h"
diff --git a/providers/mlx5/mlx5.h b/providers/mlx5/mlx5.h
index f8674c7a90db..cb65429b51f7 100644
--- a/providers/mlx5/mlx5.h
+++ b/providers/mlx5/mlx5.h
@@ -48,10 +48,6 @@
 #define unlikely(x)	__builtin_expect((x), 0)
 #endif
 
-#ifndef uninitialized_var
-#define uninitialized_var(x) x = x
-#endif
-
 #include <valgrind/memcheck.h>
 
 #ifdef HAVE_FUNC_ATTRIBUTE_ALWAYS_INLINE
diff --git a/providers/mlx5/qp.c b/providers/mlx5/qp.c
index 04abe1588d6e..e82b1a0bebc3 100644
--- a/providers/mlx5/qp.c
+++ b/providers/mlx5/qp.c
@@ -38,6 +38,7 @@
 #include <string.h>
 #include <errno.h>
 #include <stdio.h>
+#include <util/compiler.h>
 
 #include "mlx5.h"
 #include "doorbell.h"
diff --git a/util/CMakeLists.txt b/util/CMakeLists.txt
new file mode 100644
index 000000000000..1cda8905d8f4
--- /dev/null
+++ b/util/CMakeLists.txt
@@ -0,0 +1,3 @@
+publish_internal_headers(util
+  compiler.h
+  )
diff --git a/util/compiler.h b/util/compiler.h
new file mode 100644
index 000000000000..9b57e048df4b
--- /dev/null
+++ b/util/compiler.h
@@ -0,0 +1,18 @@
+/* GPLv2 or OpenIB.org BSD (MIT) See COPYING file */
+#ifndef UTIL_COMPILER_H
+#define UTIL_COMPILER_H
+
+/* Use to tag a variable that causes compiler warnings. Use as:
+    int uninitialized_var(sz)
+
+   This is only enabled for old compilers. gcc 6.x and beyond have excellent
+   static flow analysis. If code solicits a warning from 6.x it is almost
+   certainly too complex for a human to understand.
+*/
+#if __GNUC__ >= 6 || defined(__clang__)
+#define uninitialized_var(x) x
+#else
+#define uninitialized_var(x) x = x
+#endif
+
+#endif
-- 
2.1.4

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

[PATCH rdma-core 2/5] ibacm: Avoid clang 3.8 warning -Wmissing-field-initializers

[Jason Gunthorpe]

The canonical way to zero fill a struct in this tree is {}.

Fixes: 0db49b8b3e8f ("Avoid clang 3.6 warning -Wmissing-field-initializers")
Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 ibacm/prov/acmp/src/acmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ibacm/prov/acmp/src/acmp.c b/ibacm/prov/acmp/src/acmp.c
index 3f590d0b51f3..d70501aff8e8 100644
--- a/ibacm/prov/acmp/src/acmp.c
+++ b/ibacm/prov/acmp/src/acmp.c
@@ -2118,7 +2118,7 @@ static void acmp_parse_osm_fullv1_lid2guid(FILE *f, uint64_t *lid2guid)
 static int acmp_parse_osm_fullv1_paths(FILE *f, uint64_t *lid2guid, struct acmp_ep *ep)
 {
 	union ibv_gid sgid, dgid;
-	struct ibv_port_attr attr = { 0 };
+	struct ibv_port_attr attr = {};
 	struct acmp_dest *dest;
 	char s[128];
 	char *p, *ptr, *p_guid, *p_lid;
-- 
2.1.4

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

[PATCH rdma-core 3/5] mthca: Avoid bogus gcc 4.8 warning -Wmaybe-uninitialized

[Jason Gunthorpe]

Newer compilers get this right, so use uninitialized_var()

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 providers/mthca/qp.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/providers/mthca/qp.c b/providers/mthca/qp.c
index 715ca65e9bd6..129e6c74ef53 100644
--- a/providers/mthca/qp.c
+++ b/providers/mthca/qp.c
@@ -37,6 +37,7 @@
 #include <netinet/in.h>
 #include <pthread.h>
 #include <string.h>
+#include <util/compiler.h>
 
 #include "mthca.h"
 #include "doorbell.h"
@@ -107,15 +108,8 @@ int mthca_tavor_post_send(struct ibv_qp *ibqp, struct ibv_send_wr *wr,
 	int size;
 	int size0 = 0;
 	int i;
-	/*
-	 * f0 and op0 cannot be used unless nreq > 0, which means this
-	 * function makes it through the loop at least once.  So the
-	 * code inside the if (!size0) will be executed, and f0 and
-	 * op0 will be initialized.  So any gcc warning about "may be
-	 * used unitialized" is bogus.
-	 */
-	uint32_t f0;
-	uint32_t op0;
+	uint32_t uninitialized_var(f0);
+	uint32_t uninitialized_var(op0);
 
 	pthread_spin_lock(&qp->sq.lock);
 
@@ -449,15 +443,8 @@ int mthca_arbel_post_send(struct ibv_qp *ibqp, struct ibv_send_wr *wr,
 	int size;
 	int size0 = 0;
 	int i;
-	/*
-	 * f0 and op0 cannot be used unless nreq > 0, which means this
-	 * function makes it through the loop at least once.  So the
-	 * code inside the if (!size0) will be executed, and f0 and
-	 * op0 will be initialized.  So any gcc warning about "may be
-	 * used unitialized" is bogus.
-	 */
-	uint32_t f0;
-	uint32_t op0;
+	uint32_t uninitialized_var(f0);
+	uint32_t uninitialized_var(op0);
 
 	pthread_spin_lock(&qp->sq.lock);
 
-- 
2.1.4

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

[PATCH rdma-core 4/5] Improve linker flag detection to work with clang and -Werror

[Jason Gunthorpe]

If a linker flag is passed to a clang compile command then it
prints out a warning that the flag is a linker flag. If -Werror
is set then the warning is an error and all linker flag detection
is broken.

The fix is to only pass linker flags to the linker. cmake does not have
an integrated way to do this until 3.2, the recommended approach is to
abuse CMAKE_REQUIRED_LIBRARIES on older versions.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 CMakeLists.txt                 | 21 +++++++++++++--------
 buildlib/FindLDSymVer.cmake    | 16 ++++++++++++----
 buildlib/RDMA_EnableCStd.cmake | 25 +++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1611e90d7933..4d291d2c3fb7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -31,6 +31,11 @@ else()
   string(REPLACE "-rdynamic" "" CMAKE_SHARED_LIBRARY_LINK_C_FLAGS "${CMAKE_SHARED_LIBRARY_LINK_C_FLAGS}")
 endif()
 
+# Make RDMA_CHECK_C_LINKER_FLAG work better
+if(POLICY CMP0056)
+  cmake_policy(SET CMP0056 NEW)
+endif()
+
 set(PACKAGE_NAME "RDMA")
 
 # See Documentation/versioning.md
@@ -150,18 +155,18 @@ CHECK_C_SOURCE_COMPILES("
 
 # Enable development support features
 # Prune unneeded shared libraries during linking
-RDMA_AddOptCFlag(CMAKE_EXE_LINKER_FLAGS SUPPORTS_AS_NEEDED "-Wl,--as-needed")
-RDMA_AddOptCFlag(CMAKE_SHARED_LINKER_FLAGS SUPPORTS_AS_NEEDED "-Wl,--as-needed")
-RDMA_AddOptCFlag(CMAKE_MODULE_LINKER_FLAGS SUPPORTS_AS_NEEDED "-Wl,--as-needed")
+RDMA_AddOptLDFlag(CMAKE_EXE_LINKER_FLAGS SUPPORTS_AS_NEEDED "-Wl,--as-needed")
+RDMA_AddOptLDFlag(CMAKE_SHARED_LINKER_FLAGS SUPPORTS_AS_NEEDED "-Wl,--as-needed")
+RDMA_AddOptLDFlag(CMAKE_MODULE_LINKER_FLAGS SUPPORTS_AS_NEEDED "-Wl,--as-needed")
 
 # Ensure all shared ELFs have fully described linking
-RDMA_AddOptCFlag(CMAKE_EXE_LINKER_FLAGS SUPPORTS_NO_UNDEFINED "-Wl,--no-undefined")
-RDMA_AddOptCFlag(CMAKE_SHARED_LINKER_FLAGS SUPPORTS_NO_UNDEFINED "-Wl,--no-undefined")
+RDMA_AddOptLDFlag(CMAKE_EXE_LINKER_FLAGS SUPPORTS_NO_UNDEFINED "-Wl,--no-undefined")
+RDMA_AddOptLDFlag(CMAKE_SHARED_LINKER_FLAGS SUPPORTS_NO_UNDEFINED "-Wl,--no-undefined")
 
 # Enable gold linker - gold has different linking checks
-#RDMA_AddOptCFlag(CMAKE_EXE_LINKER_FLAGS SUPPORTS_NO_UNDEFINED "-fuse-ld=gold")
-#RDMA_AddOptCFlag(CMAKE_SHARED_LINKER_FLAGS SUPPORTS_NO_UNDEFINED "-fuse-ld=gold")
-#RDMA_AddOptCFlag(CMAKE_MODULE_LINKER_FLAGS SUPPORTS_NO_UNDEFINED "-fuse-ld=gold")
+#RDMA_AddOptLDFlag(CMAKE_EXE_LINKER_FLAGS SUPPORTS_NO_UNDEFINED "-fuse-ld=gold")
+#RDMA_AddOptLDFlag(CMAKE_SHARED_LINKER_FLAGS SUPPORTS_NO_UNDEFINED "-fuse-ld=gold")
+#RDMA_AddOptLDFlag(CMAKE_MODULE_LINKER_FLAGS SUPPORTS_NO_UNDEFINED "-fuse-ld=gold")
 
 # Verify that GNU --version-script and asm(".symver") works
 find_package(LDSymVer REQUIRED)
diff --git a/buildlib/FindLDSymVer.cmake b/buildlib/FindLDSymVer.cmake
index c0f01a562e32..7169b2c4db68 100644
--- a/buildlib/FindLDSymVer.cmake
+++ b/buildlib/FindLDSymVer.cmake
@@ -16,8 +16,14 @@ IBVERBS_1.1 {
 } IBVERBS_1.0;
 ")
 
-set(CMAKE_REQUIRED_FLAGS_SAVE ${CMAKE_REQUIRED_FLAGS})
-set(CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS} "-Wl,--version-script=${CMAKE_CURRENT_BINARY_DIR}/test.map")
+# See RDMA_CHECK_C_LINKER_FLAG
+set(SAFE_CMAKE_REQUIRED_LIBRARIES "${CMAKE_REQUIRED_LIBRARIES}")
+set(SAFE_CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS}")
+if (POLICY CMP0056)
+  set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,--version-script=${CMAKE_CURRENT_BINARY_DIR}/test.map")
+else()
+  set(CMAKE_REQUIRED_LIBRARIES "${CMAKE_REQUIRED_LIBRARIES} -Wl,--version-script=${CMAKE_CURRENT_BINARY_DIR}/test.map")
+endif()
 
 # And matching source, this also checks that .symver asm works
 check_c_source_compiles("
@@ -26,9 +32,11 @@ asm(\".symver ibv_get_device_list_1, ibv_get_device_list@IBVERBS_1.1\");
 void ibv_get_device_list_0(void){}
 asm(\".symver ibv_get_device_list_0, ibv_get_device_list@@IBVERBS_1.0\");
 
-int main(void){return 0;}" _LDSYMVER_SUCCESS)
+int main(int argc,const char *argv[]){return 0;}" _LDSYMVER_SUCCESS)
+
 file(REMOVE "${CMAKE_CURRENT_BINARY_DIR}/test.map")
-set(CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS_SAVE})
+set(CMAKE_EXE_LINKER_FLAGS "${SAFE_CMAKE_EXE_LINKER_FLAGS}")
+set(CMAKE_REQUIRED_LIBRARIES "${SAFE_CMAKE_REQUIRED_LIBRARIES}")
 
 if (_LDSYMVER_SUCCESS)
   set(LDSYMVER_MODE "GNU" CACHE INTERNAL "How to set symbol versions on shared libraries")
diff --git a/buildlib/RDMA_EnableCStd.cmake b/buildlib/RDMA_EnableCStd.cmake
index 9cbbb881cd19..bcf3ea525c73 100644
--- a/buildlib/RDMA_EnableCStd.cmake
+++ b/buildlib/RDMA_EnableCStd.cmake
@@ -1,5 +1,30 @@
 # COPYRIGHT (c) 2016 Obsidian Research Corporation. See COPYING file
 
+# cmake does not have way to do this even slightly sanely until CMP0056
+function(RDMA_CHECK_C_LINKER_FLAG FLAG CACHE_VAR)
+  set(SAFE_CMAKE_REQUIRED_LIBRARIES "${CMAKE_REQUIRED_LIBRARIES}")
+  set(SAFE_CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS}")
+
+  if (POLICY CMP0056)
+    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${FLAG}")
+  else()
+    set(CMAKE_REQUIRED_LIBRARIES "${CMAKE_REQUIRED_LIBRARIES} ${FLAG}")
+  endif()
+
+  CHECK_C_COMPILER_FLAG("" ${CACHE_VAR})
+
+  set(CMAKE_EXE_LINKER_FLAGS "${SAFE_CMAKE_EXE_LINKER_FLAGS}")
+  set(CMAKE_REQUIRED_LIBRARIES "${SAFE_CMAKE_REQUIRED_LIBRARIES}")
+endfunction()
+
+# Test if the CC compiler supports the linker flag and if so add it to TO_VAR
+function(RDMA_AddOptLDFlag TO_VAR CACHE_VAR FLAG)
+  RDMA_CHECK_C_LINKER_FLAG("${FLAG}" ${CACHE_VAR})
+  if (${CACHE_VAR})
+    SET(${TO_VAR} "${${TO_VAR}} ${FLAG}" PARENT_SCOPE)
+  endif()
+endfunction()
+
 # Test if the CC compiler supports the flag and if so add it to TO_VAR
 function(RDMA_AddOptCFlag TO_VAR CACHE_VAR FLAG)
   CHECK_C_COMPILER_FLAG("${FLAG}" ${CACHE_VAR})
-- 
2.1.4

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

[PATCH rdma-core 5/5] Update TravisCI to use clang 3.9 as well

[Jason Gunthorpe]

Now that the build runs warning free on clang 3.9 as well.

clang has a different set of static checker type warnings.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 .travis.yml | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index a2b9e3b8c3f3..d81b699294eb 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,15 +1,23 @@
 language: c
 # We need at least cmake 2.12, this means we need to use trusty.
 # Precise's glibc, etc predates what we are willing to support.
-# No reason for sudo.
+# sudo is required to get the trusty image, and at present to enable llvm 3.9
 sudo: required
 dist: trusty
 addons:
+  # We run our builds sequentially in one VM rather than try and use the
+  # matrix feature. This is because Travis is unreasonably inefficient
+  # doing this APT setup pass.
   apt:
     sources:
+      # sourceline because travis won't white list trusty builds, and hasn't
+      # whitelisted 3.9
+      - sourceline: "deb http://apt.llvm.org/trusty/ llvm-toolchain-trusty-3.9 main"
+        key_url: "http://apt.llvm.org/llvm-snapshot.gpg.key"
       - ubuntu-toolchain-r-test
     packages:
       - build-essential
+      - clang-3.9
       - cmake
       - debhelper
       - gcc
@@ -25,16 +33,22 @@ addons:
       # 32 bit support packages
       - gcc-multilib
       - lib32gcc-6-dev
+
 script:
-  - mkdir build build32
+  - mkdir build build-clang build32
   - cd build
   # The goal is warning free compile on latest gcc.
   - CC=gcc-6 CFLAGS=-Werror cmake -GNinja ..
   - ninja
 
+  # .. and latest clang
+  - cd ../build-clang
+  - CC=clang-3.9 CFLAGS=-Werror cmake -GNinja ..
+  - ninja
+
   # 32 bit build
   - cd ../build32
-  # travis's trusty is not configured in a way that enables all32 bit
+  # travis's trusty is not configured in a way that enables all 32 bit
   # packages. We could fix this with some sudo stuff.. For now turn off libnl
   - CC=gcc-6 CFLAGS="-Werror -m32" cmake -GNinja .. -DENABLE_RESOLVE_NEIGH=0
   - ninja
-- 
2.1.4

--
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 1/5] Pull uninitialized_var into util/compiler.h

[Leon Romanovsky]

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

On Fri, Oct 07, 2016 at 04:36:08PM -0600, Jason Gunthorpe wrote:
> The new common definition turns it off for new compilers since
> it is not needed and is too easy to abuse.

Jason,
I have two comments.

1. Can we remove this uninitialized_var at all and simply replace by
default values?
We have 5 places like this:
➜  rdma-core git:(master) grep -r uninitialized_var */src/*.c
libcxgb4/src/cq.c:      struct t4_cqe uninitialized_var(cqe), *rd_cqe;
libcxgb4/src/qp.c:      u8 uninitialized_var(len16);
libmlx4/src/qp.c:       struct mlx4_wqe_ctrl_seg *uninitialized_var(ctrl);
libmlx5/src/qp.c:       uint32_t *uninitialized_var(p);
libmlx5/src/qp.c:                       int uninitialized_var(sz);

2. Do we really want additional folder to ccan with such common headers?
What about rename ccan to be util and put this new file there (in case
you proceed with it)?

Thanks

>
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
>  CMakeLists.txt             |  1 +
>  providers/cxgb4/cq.c       |  1 +
>  providers/cxgb4/libcxgb4.h |  2 --
>  providers/cxgb4/qp.c       |  1 +
>  providers/mlx4/mlx4.h      |  4 ----
>  providers/mlx4/qp.c        |  1 +
>  providers/mlx5/mlx5.h      |  4 ----
>  providers/mlx5/qp.c        |  1 +
>  util/CMakeLists.txt        |  3 +++
>  util/compiler.h            | 18 ++++++++++++++++++
>  10 files changed, 26 insertions(+), 10 deletions(-)
>  create mode 100644 util/CMakeLists.txt
>  create mode 100644 util/compiler.h
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index b0864da660fc..1611e90d7933 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -253,6 +253,7 @@ configure_file("${BUILDLIB}/config.h.in" "${BUILD_INCLUDE}/config.h" ESCAPE_QUOT
>  #-------------------------
>  # Sub-directories
>  add_subdirectory(ccan)
> +add_subdirectory(util)
>  # Libraries
>  add_subdirectory(libibumad)
>  add_subdirectory(libibumad/man)
> diff --git a/providers/cxgb4/cq.c b/providers/cxgb4/cq.c
> index 1ed7dfdb88d4..5f662ac49d0c 100644
> --- a/providers/cxgb4/cq.c
> +++ b/providers/cxgb4/cq.c
> @@ -37,6 +37,7 @@
>  #include <sys/errno.h>
>  #include <netinet/in.h>
>  #include <infiniband/opcode.h>
> +#include <util/compiler.h>
>  #include "libcxgb4.h"
>  #include "cxgb4-abi.h"
>
> diff --git a/providers/cxgb4/libcxgb4.h b/providers/cxgb4/libcxgb4.h
> index 4c8383209287..9a4bc98f34e9 100644
> --- a/providers/cxgb4/libcxgb4.h
> +++ b/providers/cxgb4/libcxgb4.h
> @@ -264,6 +264,4 @@ void dump_state();
>  extern int stall_to;
>  #endif
>
> -#define uninitialized_var(x) x = x
> -
>  #endif				/* IWCH_H */
> diff --git a/providers/cxgb4/qp.c b/providers/cxgb4/qp.c
> index 384bf11369ac..95b459a1a9b4 100644
> --- a/providers/cxgb4/qp.c
> +++ b/providers/cxgb4/qp.c
> @@ -37,6 +37,7 @@
>  #include <string.h>
>  #include <stdio.h>
>  #include <netinet/in.h>
> +#include <util/compiler.h>
>  #include "libcxgb4.h"
>
>  #ifdef STATS
> diff --git a/providers/mlx4/mlx4.h b/providers/mlx4/mlx4.h
> index 95a6521c457b..a2d39e169c15 100644
> --- a/providers/mlx4/mlx4.h
> +++ b/providers/mlx4/mlx4.h
> @@ -43,10 +43,6 @@
>
>  #define MLX4_PORTS_NUM 2
>
> -#ifndef uninitialized_var
> -#define uninitialized_var(x) x = x
> -#endif
> -
>  #include <valgrind/memcheck.h>
>
>  #define PFX		"mlx4: "
> diff --git a/providers/mlx4/qp.c b/providers/mlx4/qp.c
> index 4b5acd71108e..268fb7dc83dd 100644
> --- a/providers/mlx4/qp.c
> +++ b/providers/mlx4/qp.c
> @@ -39,6 +39,7 @@
>  #include <pthread.h>
>  #include <string.h>
>  #include <errno.h>
> +#include <util/compiler.h>
>
>  #include "mlx4.h"
>  #include "doorbell.h"
> diff --git a/providers/mlx5/mlx5.h b/providers/mlx5/mlx5.h
> index f8674c7a90db..cb65429b51f7 100644
> --- a/providers/mlx5/mlx5.h
> +++ b/providers/mlx5/mlx5.h
> @@ -48,10 +48,6 @@
>  #define unlikely(x)	__builtin_expect((x), 0)
>  #endif
>
> -#ifndef uninitialized_var
> -#define uninitialized_var(x) x = x
> -#endif
> -
>  #include <valgrind/memcheck.h>
>
>  #ifdef HAVE_FUNC_ATTRIBUTE_ALWAYS_INLINE
> diff --git a/providers/mlx5/qp.c b/providers/mlx5/qp.c
> index 04abe1588d6e..e82b1a0bebc3 100644
> --- a/providers/mlx5/qp.c
> +++ b/providers/mlx5/qp.c
> @@ -38,6 +38,7 @@
>  #include <string.h>
>  #include <errno.h>
>  #include <stdio.h>
> +#include <util/compiler.h>
>
>  #include "mlx5.h"
>  #include "doorbell.h"
> diff --git a/util/CMakeLists.txt b/util/CMakeLists.txt
> new file mode 100644
> index 000000000000..1cda8905d8f4
> --- /dev/null
> +++ b/util/CMakeLists.txt
> @@ -0,0 +1,3 @@
> +publish_internal_headers(util
> +  compiler.h
> +  )
> diff --git a/util/compiler.h b/util/compiler.h
> new file mode 100644
> index 000000000000..9b57e048df4b
> --- /dev/null
> +++ b/util/compiler.h
> @@ -0,0 +1,18 @@
> +/* GPLv2 or OpenIB.org BSD (MIT) See COPYING file */
> +#ifndef UTIL_COMPILER_H
> +#define UTIL_COMPILER_H
> +
> +/* Use to tag a variable that causes compiler warnings. Use as:
> +    int uninitialized_var(sz)
> +
> +   This is only enabled for old compilers. gcc 6.x and beyond have excellent
> +   static flow analysis. If code solicits a warning from 6.x it is almost
> +   certainly too complex for a human to understand.
> +*/
> +#if __GNUC__ >= 6 || defined(__clang__)
> +#define uninitialized_var(x) x
> +#else
> +#define uninitialized_var(x) x = x
> +#endif
> +
> +#endif
> --
> 2.1.4
>
> --
> 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

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

Re: [PATCH rdma-core 1/5] Pull uninitialized_var into util/compiler.h

[Jason Gunthorpe]

On Sun, Oct 09, 2016 at 08:46:01AM +0300, Leon Romanovsky wrote:

> 1. Can we remove this uninitialized_var at all and simply replace by
> default values?

I think people would complain about the extra stores. gcc 6 will
eliminate them, but older compilers will not.

But yes, we could do this.

> 2. Do we really want additional folder to ccan with such common headers?
> What about rename ccan to be util and put this new file there (in case
> you proceed with it)?

I see the ccan directory as stuf from ccan that we are not going to
change, while util is for new stuff specific to this tree.

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 1/5] Pull uninitialized_var into util/compiler.h

[Leon Romanovsky]

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

On Sun, Oct 09, 2016 at 05:01:51PM -0600, Jason Gunthorpe wrote:
> On Sun, Oct 09, 2016 at 08:46:01AM +0300, Leon Romanovsky wrote:
>
> > 1. Can we remove this uninitialized_var at all and simply replace by
> > default values?
>
> I think people would complain about the extra stores. gcc 6 will
> eliminate them, but older compilers will not.

This unintialized_var(x) adds extra store too (... x = x ...).

>
> But yes, we could do this.
>
> > 2. Do we really want additional folder to ccan with such common headers?
> > What about rename ccan to be util and put this new file there (in case
> > you proceed with it)?
>
> I see the ccan directory as stuf from ccan that we are not going to
> change, while util is for new stuff specific to this tree.

And I see this folder as a location of shared code with different utils
(ccan, custom, e.t.c), but it doesn't really matter now and I will be fine
with any directory structure layout.

Thanks

>
> Jason

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

RE: [PATCH rdma-core 1/5] Pull uninitialized_var into util/compiler.h

[Steve Wise]

> The new common definition turns it off for new compilers since
> it is not needed and is too easy to abuse.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>


Reviewed-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>

--
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 1/5] Pull uninitialized_var into util/compiler.h

[Jason Gunthorpe]

On Mon, Oct 10, 2016 at 07:09:30AM +0300, Leon Romanovsky wrote:

> > I think people would complain about the extra stores. gcc 6 will
> > eliminate them, but older compilers will not.
> 
> This unintialized_var(x) adds extra store too (... x = x ...).

I have confirmed that the unintialized_var macro does not impact code
generation on the old gccs while the =0 approach does.

So no extra store.

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 1/5] Pull uninitialized_var into util/compiler.h

[Leon Romanovsky]

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

On Tue, Oct 11, 2016 at 12:05:17PM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 10, 2016 at 07:09:30AM +0300, Leon Romanovsky wrote:
>
> > > I think people would complain about the extra stores. gcc 6 will
> > > eliminate them, but older compilers will not.
> >
> > This unintialized_var(x) adds extra store too (... x = x ...).
>
> I have confirmed that the unintialized_var macro does not impact code
> generation on the old gccs while the =0 approach does.

Ohh, thanks.

But I'm still left under impression of this article [1] that using such
macro is a bad thing and we are "punishing" all users of modern compilers.

[1] https://lwn.net/Articles/529954/

>
> So no extra store.
>
> Jason

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

Re: [PATCH rdma-core 1/5] Pull uninitialized_var into util/compiler.h

[Jason Gunthorpe]

On Tue, Oct 11, 2016 at 10:46:33PM +0300, Leon Romanovsky wrote:

> But I'm still left under impression of this article [1] that using such
> macro is a bad thing and we are "punishing" all users of modern compilers.

I agree with the article, it is a bad idea. This is why my version is
disabling the macro entirely if gcc 6 or clang is used - aka the
compilers that run in Travis.

So, new code must not introduce control flow that is more complex than
gcc 6 can understand, and the macro is used only for gcc 4.x and 5.x
compatability to provide warning free compile on popular distros
without a performance hit.

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 0/5] clang support

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1518 bytes --]

On 10/7/2016 6:36 PM, Jason Gunthorpe wrote:
> This series gets clang running warning free and has travis run the build
> through clang-3.9 so we don't break it again.
> 
> Jason Gunthorpe (5):
>   Pull uninitialized_var into util/compiler.h
>   ibacm: Avoid clang 3.8 warning -Wmissing-field-initializers
>   mthca: Avoid bogus gcc 4.8 warning -Wmaybe-uninitialized
>   Improve linker flag detection to work with clang and -Werror
>   Update TravisCI to use clang 3.9 as well
> 
>  .travis.yml                    | 20 +++++++++++++++++---
>  CMakeLists.txt                 | 22 ++++++++++++++--------
>  buildlib/FindLDSymVer.cmake    | 16 ++++++++++++----
>  buildlib/RDMA_EnableCStd.cmake | 25 +++++++++++++++++++++++++
>  ibacm/prov/acmp/src/acmp.c     |  2 +-
>  providers/cxgb4/cq.c           |  1 +
>  providers/cxgb4/libcxgb4.h     |  2 --
>  providers/cxgb4/qp.c           |  1 +
>  providers/mlx4/mlx4.h          |  4 ----
>  providers/mlx4/qp.c            |  1 +
>  providers/mlx5/mlx5.h          |  4 ----
>  providers/mlx5/qp.c            |  1 +
>  providers/mthca/qp.c           | 23 +++++------------------
>  util/CMakeLists.txt            |  3 +++
>  util/compiler.h                | 18 ++++++++++++++++++
>  15 files changed, 99 insertions(+), 44 deletions(-)
>  create mode 100644 util/CMakeLists.txt
>  create mode 100644 util/compiler.h
> 

Series merged.  Thanks.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]