Add ThreadSanitizer build type
authorMark Abraham <mark.j.abraham@gmail.com>
Fri, 18 Oct 2013 15:47:25 +0000 (17:47 +0200)
committerGerrit Code Review <gerrit@gerrit.gromacs.org>
Wed, 26 Feb 2014 02:47:10 +0000 (03:47 +0100)
This might be useful for helping detect misuse of thread
operations.

It does run quite slowly, so use in an automatic per-patch Jenkins
build is probably not a good idea. In any case, it should be run
on a real workload, as well as toy examples.

As discussed in the CMake file that manages the new build type, using
TSan requires that CMake can demand that atomic operations are not
used. Accordingly, the atomic.h functionality and the CMake atomics
testing that uses it have some changes so all the ways atomic.h is
used work correctly. Documentation of how and why things work is
expanded.

A benign race condition with the (unused) return value of
mdrunner_start_threads is fixed.

Change-Id: I0da01c0bedeec2e49d4456d757c54fbe58c56ef4

CMakeLists.txt
cmake/ThreadMPI.cmake
cmake/gmxBuildTypeThreadSanitizer.cmake [new file with mode: 0644]
src/gromacs/legacyheaders/thread_mpi/atomic.h
src/programs/mdrun/runner.c

index f30bcd6e3d8534ce76cb0dac43b1887a64a1aac9..cce50946949e8b94e0dcc813cfa27afc6e401d6a 100644 (file)
@@ -95,9 +95,11 @@ set(GMX_INSTALL_PREFIX "" CACHE STRING "Prefix gets appended to CMAKE_INSTALL_PR
 mark_as_advanced(GMX_INSTALL_PREFIX)
 
 include(gmxBuildTypeReference)
+include(gmxBuildTypeThreadSanitizer)
 
 if(NOT CMAKE_BUILD_TYPE)
     set(CMAKE_BUILD_TYPE "Release" CACHE STRING "Choose the type of build, options are: Debug Release RelWithDebInfo MinSizeRel Reference." FORCE)
+    # There's no need to offer a user the choice of ThreadSanitizer
     # Set the possible values of build type for cmake-gui
     set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS "Debug" "Release"
         "MinSizeRel" "RelWithDebInfo")
@@ -433,6 +435,10 @@ if(GMX_THREAD_MPI)
     set(MPI_IN_PLACE_EXISTS 1)
 endif(GMX_THREAD_MPI)
 tmpi_get_source_list(THREAD_MPI_SRC)
+# If atomics are manually disabled a define is needed because atomics.h doesn't depend on config.h
+if (TMPI_ATOMICS_DISABLED)
+   add_definitions(-DTMPI_ATOMICS_DISABLED)
+endif()
 
 if(GMX_GPU)
     # now that we have detected the dependencies, do the second configure pass
index d0a02e5e03caf1346c0b04ab1d4542f1ed3fe068..53c6719d477c09073e8d7e43a7fdeb9b3b034955 100644 (file)
@@ -37,21 +37,26 @@ include(CheckIncludeFiles)
 include(CheckFunctionExists)
 include(CheckCSourceCompiles)
 
-# sets TMPI_ATOMICS to 1 if atomic operations are found, 0 otherwise
+# sets TMPI_ATOMICS to 1 if atomic operations are found, undef otherwise
 # Options:
 # include directory for thread_mpi/atomic.h
 MACRO(TMPI_TEST_ATOMICS INCDIR)
     if (NOT DEFINED TMPI_ATOMICS)
         try_compile(TEST_ATOMICS "${CMAKE_BINARY_DIR}"
                 "${CMAKE_SOURCE_DIR}/cmake/TestAtomics.c"
-                COMPILE_DEFINITIONS "-I${INCDIR}")
+                COMPILE_DEFINITIONS "-I${INCDIR} -DTMPI_ATOMICS")
         if (TEST_ATOMICS)
             message(STATUS "Atomic operations found")
+            # If the check fails, we want to be able to check again,
+            # in case the user has been able to fix this without
+            # needing to delete the cache. Thus we only cache
+            # positive results.
+            set(TMPI_ATOMICS ${TEST_ATOMICS} CACHE INTERNAL "Whether atomic operations are found")
+            set(TMPI_ATOMICS_INCDIR ${INCDIR} CACHE INTERNAL "Atomic operations check include dir")
         else (TEST_ATOMICS)
             message(STATUS "Atomic operations not found")
+            undef(TEST_ATOMICS)
         endif(TEST_ATOMICS)
-        set(TMPI_ATOMICS ${TEST_ATOMICS} CACHE INTERNAL "Whether atomic operations are found")
-        set(TMPI_ATOMICS_INCDIR ${INCDIR} CACHE INTERNAL "Atomic operations check include dir")
     endif(NOT DEFINED TMPI_ATOMICS)
 ENDMACRO(TMPI_TEST_ATOMICS VARIABLE)
 
@@ -114,15 +119,10 @@ ENDMACRO(TMPI_ENABLE_CXX)
 
 # Turns on thread_mpi MPI functions.
 MACRO(TMPI_ENABLE)
-    # first check whether threads and atomics are available.
-    if(NOT TMPI_ATOMICS)
-        # check again, to allow the user to fix this.
-        unset(TMPI_ATOMICS CACHE)
-        TMPI_TEST_ATOMICS(${TMPI_ATOMICS_INCDIR})
-    endif(NOT TMPI_ATOMICS)
-    if(NOT TMPI_ATOMICS)
+    TMPI_TEST_ATOMICS(TMPI_ATOMICS_INCDIR)
+    if(NOT DEFINED TMPI_ATOMICS)
         message(WARNING "Atomic operations not found for this CPU+compiler combination. Thread support will be unbearably slow: disable threads. Atomic operations should work on all but the most obscure CPU+compiler combinations; if your system is not obscure -- like, for example, x86 with gcc --  please contact the developers.")
-    endif(NOT TMPI_ATOMICS)
+    endif()
 
     set(TMPI_ENABLED 1)
 
diff --git a/cmake/gmxBuildTypeThreadSanitizer.cmake b/cmake/gmxBuildTypeThreadSanitizer.cmake
new file mode 100644 (file)
index 0000000..3c513d7
--- /dev/null
@@ -0,0 +1,93 @@
+#
+# This file is part of the GROMACS molecular simulation package.
+#
+# Copyright (c) 2014, by the GROMACS development team, led by
+# Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
+# and including many others, as listed in the AUTHORS file in the
+# top-level source directory and at http://www.gromacs.org.
+#
+# GROMACS is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public License
+# as published by the Free Software Foundation; either version 2.1
+# of the License, or (at your option) any later version.
+#
+# GROMACS is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with GROMACS; if not, see
+# http://www.gnu.org/licenses, or write to the Free Software Foundation,
+# Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA.
+#
+# If you want to redistribute modifications to GROMACS, please
+# consider that scientific software is very special. Version
+# control is crucial - bugs must be traceable. We will be happy to
+# consider code for inclusion in the official distribution, but
+# derived work must not be called official GROMACS. Details are found
+# in the README & COPYING files - if they are missing, get the
+# official version at http://www.gromacs.org.
+#
+# To help us fund GROMACS development, we humbly ask that you cite
+# the research papers on the package. Check out http://www.gromacs.org.
+
+# Custom build type "ThreadSanitizer", to be used for compiling GROMACS 
+# with clang 3.4 or gcc 4.8 (currently pre-release) with ThreadSanitizer
+# (aka "TSan") turned on, so that the tests can be run to detect data races.
+#
+# The main advantage of the clang version is that there can be a
+# suppressions file that acts at compile time, though there is no use of 
+# that yet. The main advantage of the gcc version is that it can be used
+# with a OpenMP (if gomp is compiled with --disable-linux-futex).
+#
+# Unfortunately, out of the box Thread-MPI provokes several false
+# positives. One example is that tMPI_Event_t contains an atomic int
+# field "sync" that is initialized before thread spawn. During a
+# collective, this is atomically updated by the source thread, and the
+# update is observed by sink threads in tMPI_Event_wait, which do a
+# yield wait when no change has occured. This means the read can
+# happen before the write (by design, whether or not the read is
+# atomic), but the surrounding logic prevents action until the write
+# has happened. There is no way for the sink thread(s) to avoid
+# reading until the write has happened - that is the point of the
+# implementation.
+#
+# This ought to be able to be suppressed, but my attempts to apply
+# suppressions on individual functions don't suppress reporting of the
+# race event. Applying the suppression to the whole thread-MPI library
+# might work, but seems to defeat the point. We want to be able to
+# detect mis-use of the primitives provided by thread-MPI.
+#
+# This means there needs to be a way for this build type to trigger
+# the use of the generic mutex-based fallback implementation within
+# thread-MPI.
+#
+# Later, if a blacklist is needed, use something like
+# "-fsanitize-blacklist=${CMAKE_SOURCE_DIR}/cmake/thread-sanitizer.supp"
+# TODO find a better home for this and other suppression files
+set(_flags "-O1 -g -fsanitize=thread")
+
+foreach(_language C CXX)
+
+    string(REPLACE "X" "+" _human_readable_language ${_language})
+
+    if (CMAKE_${_language}_COMPILER_ID MATCHES "GNU")
+        set(CMAKE_${_language}_FLAGS_THREADSANITIZER "${_flags} -pie -fPIE" CACHE STRING "${_human_readable_language} flags for thread sanitizer")
+    else()
+        set(CMAKE_${_language}_FLAGS_THREADSANITIZER ${_flags} CACHE STRING "${_human_readable_language} flags for thread sanitizer")
+    endif()
+    mark_as_advanced(CMAKE_${_language}_FLAGS_THREADSANITIZER)
+    string(TOUPPER "${CMAKE_BUILD_TYPE}" _cmake_build_type)
+    if (_cmake_build_type STREQUAL THREADSANITIZER)
+        set(TMPI_ATOMICS_DISABLED 1)
+        set(TMPI_ATOMICS 0)
+        if (NOT((CMAKE_${_language}_COMPILER_ID MATCHES "Clang" AND
+                    CMAKE_${_language}_COMPILER_VERSION VERSION_GREATER 3.2.999)
+             OR (CMAKE_${_language}_COMPILER_ID MATCHES "GNU" AND
+                    CMAKE_${_language}_COMPILER_VERSION VERSION_GREATER 4.7.999)))
+            message(FATAL_ERROR "The ThreadSanitizer build is only available with clang ${_human_readable_language} >=3.3 and gnu ${_human_readable_language} >=4.8.")
+        endif()
+    endif()
+
+endforeach()
index cf5ae9d27ee8bca1ba3e6aee86e318b26a66b417..0185bcbb20790189598ee09def0049f9320fc4db 100644 (file)
@@ -86,6 +86,23 @@ extern "C"
 } /* Avoids screwing up auto-indentation */
 #endif
 
+/* Setting TMPI_ATOMICS_DISABLED permits the build to enforce that no
+ * atomic operations are used. This is used when building to run
+ * ThreadSanitzer.
+ *
+ * It could also be useful as a temporary measure on some
+ * compiler+hardware for which the detection below fails to produce a
+ * correct result. Performance will be greatly improved by using
+ * whatever atomic operations are available, so make sure such a
+ * measure is only temporary! */
+#ifdef TMPI_ATOMICS_DISABLED
+
+#ifndef DOXYGEN
+#define TMPI_NO_ATOMICS
+#endif
+
+#else
+
 /* first check for gcc/icc platforms.
    Some compatible compilers, like icc on linux+mac will take this path,
    too */
@@ -156,16 +173,24 @@ extern "C"
 
 
 #else
+
+#ifndef DOXYGEN
+/** Indicates that no support for atomic operations is present. */
+#define TMPI_NO_ATOMICS
+#endif
+
+#endif /* platform-specific checks */
+
+#endif /* TMPI_NO_ATOMICS */
+
+#ifdef TMPI_NO_ATOMICS
+
 /* No atomic operations, use mutex fallback. Documentation is in x86 section */
 
 #ifdef TMPI_CHECK_ATOMICS
 #error No atomic operations implemented for this cpu/compiler combination.
 #endif
 
-#ifndef DOXYGEN
-/** Indicates that no support for atomic operations is present. */
-#define TMPI_NO_ATOMICS
-#endif
 
 /** Memory barrier operation
 
@@ -587,7 +612,7 @@ TMPI_EXPORT
 void tMPI_Spinlock_wait(tMPI_Spinlock_t *x);
 
 
-#endif /* platform-specific checks */
+#endif /* TMPI_NO_ATOMICS */
 
 /* now define all the atomics that are not avaible natively. These
    are done on the assumption that a native CAS does exist. */
index 57792ae2a7df34f2c7bd45c6b6e0d30cba663207..9d6211b036421f387761d25799333d60d4d11732 100644 (file)
@@ -143,7 +143,6 @@ struct mdrunner_arglist
     real            max_hours;
     const char     *deviceOptions;
     unsigned long   Flags;
-    int             ret; /* return value */
 };
 
 
@@ -170,15 +169,15 @@ static void mdrunner_start_fn(void *arg)
         fplog = mc.fplog;
     }
 
-    mda->ret = mdrunner(&mc.hw_opt, fplog, cr, mc.nfile, fnm, mc.oenv,
-                        mc.bVerbose, mc.bCompact, mc.nstglobalcomm,
-                        mc.ddxyz, mc.dd_node_order, mc.rdd,
-                        mc.rconstr, mc.dddlb_opt, mc.dlb_scale,
-                        mc.ddcsx, mc.ddcsy, mc.ddcsz,
-                        mc.nbpu_opt, mc.nstlist_cmdline,
-                        mc.nsteps_cmdline, mc.nstepout, mc.resetstep,
-                        mc.nmultisim, mc.repl_ex_nst, mc.repl_ex_nex, mc.repl_ex_seed, mc.pforce,
-                        mc.cpt_period, mc.max_hours, mc.deviceOptions, mc.Flags);
+    mdrunner(&mc.hw_opt, fplog, cr, mc.nfile, fnm, mc.oenv,
+             mc.bVerbose, mc.bCompact, mc.nstglobalcomm,
+             mc.ddxyz, mc.dd_node_order, mc.rdd,
+             mc.rconstr, mc.dddlb_opt, mc.dlb_scale,
+             mc.ddcsx, mc.ddcsy, mc.ddcsz,
+             mc.nbpu_opt, mc.nstlist_cmdline,
+             mc.nsteps_cmdline, mc.nstepout, mc.resetstep,
+             mc.nmultisim, mc.repl_ex_nst, mc.repl_ex_nex, mc.repl_ex_seed, mc.pforce,
+             mc.cpt_period, mc.max_hours, mc.deviceOptions, mc.Flags);
 }
 
 /* called by mdrunner() to start a specific number of threads (including