ThreadSanitizer fixes and CI (neuronsimulator#2034)

* Update submodule with TSan + GCC support.
* Add Ubuntu 22.04 + TSan + GCC CI job.
* Fix some issues identified by TSan.
* Fix automatic formatting of MUTDEC .

Co-authored-by: Luc Grosheintz <[email protected]>
Co-authored-by: nrnhines <[email protected]>

Author: 3 people

.clang-format.changes

@@ -1,6 +1,6 @@
 SortIncludes: false
 Standard: c++17
-StatementMacros: [MKDLL, MKDLLdec, MKDLLif, MKDLLvp, MKDLLvpf, PyObject_HEAD,
+StatementMacros: [MKDLL, MKDLLdec, MKDLLif, MKDLLvp, MKDLLvpf, MUTDEC, PyObject_HEAD,
 declareActionCallback, declareAdjustStepper, declareArrowGlyph, declareFieldEditorCallback,
 declareFieldSEditorCallback, declareFileChooserCallback, declareIOCallback, declareList, 
 declarePtrList, declareRubberCallback, declareSelectionCallback, declareTable, declareTable2,

.github/problem-matchers/thread.json

@@ -0,0 +1,15 @@
+
+{
+  "problemMatcher": [
+      {
+          "owner": "tsan-problem-matcher",
+          "severity": "warning",
+          "pattern": [
+              {
+                  "regexp": "^.*ThreadSanitizer: (.*)$",
+                  "message": 1
+              }
+          ]
+      }
+  ]
+}

.github/workflows/neuron-ci.yml

@@ -55,27 +55,34 @@ jobs:
           - os: ubuntu-22.04
             config:
               build_mode: cmake
-              # TODO: -DNMODL_SANITIZERS=undefined
               cmake_option: -DNRN_ENABLE_CORENEURON=ON
-                -DNRN_ENABLE_INTERVIEWS=OFF
+                -DNRN_ENABLE_INTERVIEWS=OFF -DNMODL_SANITIZERS=undefined
               flag_warnings: ON
               sanitizer: undefined
           - os: ubuntu-22.04
             config:
               build_mode: cmake
-              # TODO: -DNMODL_SANITIZERS=address,leak
               # TODO: CoreNEURON is only LeakSanitizer-clean if we disable MPI
               cmake_option: -DNRN_ENABLE_CORENEURON=ON
-                -DNRN_ENABLE_INTERVIEWS=OFF
+                -DNRN_ENABLE_INTERVIEWS=OFF -DNMODL_SANITIZERS=address
               # TODO: address-leak is the dream, but there are many problems,
               # including external ones from the MPI implementations
               sanitizer: address
+          - os: ubuntu-22.04
+            config:
+              build_mode: cmake
+              # Cannot use a non-instrumented OpenMP with TSan, and we don't
+              # have a TSan-instrumented OpenMP runtime available.
+              # TODO: debug RX3D + TSan
+              cmake_option: -DNRN_ENABLE_CORENEURON=ON -DNRN_ENABLE_MPI=OFF
+                -DCORENRN_ENABLE_OPENMP=OFF -DNRN_ENABLE_RX3D=OFF
+              sanitizer: thread
           - os: macOS-12
             config:
               build_mode: cmake
               # TODO: investigate rxd test timeouts in this build and re-enable them
               cmake_option: -DNRN_ENABLE_CORENEURON=ON -DNRN_ENABLE_INTERVIEWS=OFF
-                -DNRN_ENABLE_RX3D=OFF
+                -DNRN_ENABLE_RX3D=OFF -DNMODL_SANITIZERS=address
               sanitizer: address
       fail-fast: false
 
@@ -252,10 +259,18 @@ jobs:
               # Sanitizer-specific setup
               if [[ -n "${{matrix.config.sanitizer}}" ]]; then
                 if [ "$RUNNER_OS" == "Linux" ]; then
-                  CC=$(command -v clang-14)
-                  CXX=$(command -v clang++-14)
-                  symbolizer_path=$(realpath $(command -v llvm-symbolizer-14))
-                  cmake_args+=(-DLLVM_SYMBOLIZER_PATH="${symbolizer_path}")
+                  if [[ "${{matrix.config.sanitizer}}" == "thread" ]]; then
+                    # GitHub/ubuntu-22.04 + clang-14 seems to have problems with TSan.
+                    # Vanilla 22.04 + clang-16 from apt.llvm.org seemed to work.
+                    # Use gcc-12 instead, as GitHub/ubuntu-22.04 already has it.
+                    CC=$(command -v gcc-12)
+                    CXX=$(command -v g++-12)
+                  else
+                    CC=$(command -v clang-14)
+                    CXX=$(command -v clang++-14)
+                    symbolizer_path=$(realpath $(command -v llvm-symbolizer-14))
+                    cmake_args+=(-DLLVM_SYMBOLIZER_PATH="${symbolizer_path}")
+                  fi
                 fi
                 cmake_args+=(-DCMAKE_BUILD_TYPE=Custom \
                              -DCMAKE_C_FLAGS="-O1 -g" \

.sanitizers/thread.supp

@@ -0,0 +1,4 @@
+deadlock:mca_btl_tcp_add_procs
+deadlock:mca_btl_tcp_proc_remove
+race:pmix_ptl_base_start_listening
+race:pmix_ptl_base_stop_listening

cmake/SanitizerHelper.cmake

@@ -33,6 +33,9 @@ if(NRN_SANITIZERS)
   if("address" IN_LIST nrn_sanitizers)
     list(APPEND NRN_COMPILE_DEFS NRN_ASAN_ENABLED)
   endif()
+  if("thread" IN_LIST nrn_sanitizers)
+    list(APPEND NRN_COMPILE_DEFS NRN_TSAN_ENABLED)
+  endif()
   # generate and install a launcher script called nrn-enable-sanitizer [--preload] that sets
   # *SAN_OPTIONS variables and, optionally, LD_PRELOAD -- this is useful both in CI configuration
   # and when using the sanitizers "downstream" of NEURON

docs/cmake_doc/options.rst

@@ -521,10 +521,10 @@ NRN_COVERAGE_FILES:STRING=
 
 NRN_SANITIZERS:STRING=
 ----------------------
-  Enable some combination of AddressSanitizer, LeakSanitizer and
-  UndefinedBehaviorSanitizer. Accepts a comma-separated list of ``address``,
-  ``leak`` and ``undefined``. See the "Diagnosis and Debugging" section for more
-  information.
+  Enable some combination of AddressSanitizer, LeakSanitizer, ThreadSanitizer
+  and UndefinedBehaviorSanitizer. Accepts a comma-separated list of ``address``,
+  ``leak``, ``thread`` and ``undefined``.
+  See the "Diagnosis and Debugging" section for more information.
   Note that on macOS it can be a little intricate to combine
   ``-DNRN_SANITIZERS=address`` with the use of Python virtual environments; if
   you attempt this then the CMake code should recommend a solution.

docs/dev/data-structures.rst

@@ -496,9 +496,8 @@ This is done using a token type:
 
     void whatever(my_soa_container& data) { // assume `data` was not already "frozen"
         owning_handle foo{data}; // adding an element is OK
-        // in reality you would probably apply a permutation here before marking data sorted
-        auto token = data.get_sorted_token(); // mark container "frozen" so it remains sorted
-        owning_handle disappointing_foo{data}; // this will throw
+        auto token = data.issue_frozen_token(); // mark container "frozen"
+        owning_handle disappointing_foo{data};  // this will throw
     } // `token`` is destroyed here; `data` ceases to be "frozen"
 
 The container maintains a count of how many tokens are controlling it and is "frozen" whenever that

docs/install/debug.md

@@ -128,8 +128,8 @@ the next valgrind error.
 
 
 #### Sanitizers
-The `AddressSanitizer` (ASan), `LeakSanitizer` (LSan), `ThreadSanitizer` (TSan,
-see below) and `UndefinedBehaviorSanitizer` (UBSan) are a collection of tools
+The `AddressSanitizer` (ASan), `LeakSanitizer` (LSan), `ThreadSanitizer` (TSan)
+and `UndefinedBehaviorSanitizer` (UBSan) are a collection of tools
 that rely on compiler instrumentation to catch dangerous behaviour at runtime.
 Compiler support is widespread but not ubiquitous, but both Clang and GCC
 provide support.
@@ -147,13 +147,14 @@ The typical example of this case is loading NEURON from Python, where the
 As of [#1842](https://github.com/neuronsimulator/nrn/pull/1842), the NEURON
 build system is aware of ASan, LSan and UBSan, and it tries to configure the
 sanitizers correctly based on the `NRN_SANITIZERS` CMake variable.
+In [#2034](https://github.com/neuronsimulator/nrn/pull/2034) this was extended
+to TSan via `-DNRN_SANITIZERS=thread`, and support for GCC was added.
 For example, `cmake -DNRN_SANITIZERS=address,leak ...` will enable ASan and
 LSan, while `-DNRN_SANITIZERS=undefined` will enable UBSan.
-Not all combinations of sanitizers are possible, so far ASan, ASan+LSan and
-UBSan have been tested with Clang, GCC has not been tested and likely needs
-minor changes.
-Support for standalone LSan and for TSan (see below for manual instructions)
-should be possible without major difficulties, but is not yet implemented.
+Not all combinations of sanitizers are possible, so far ASan, ASan+LSan, TSan
+and UBSan have been tested with Clang.
+Support for standalone LSan should be possible without major difficulties, but
+is not yet implemented.
 
 Depending on your system, you may also need to set the `LLVM_SYMBOLIZER_PATH`
 variable to point to the `llvm-symbolizer` executable matching your Clang.
@@ -176,59 +177,30 @@ use `nrn-enable-sanitizer special -python path/to/script.py` or
 because the `python` binary is (presumably) not linked against the sanitizer
 runtime library.
 
-LSan and UBSan support suppression files, which can be used to prevent tests
-failing due to known issues.
-NEURON includes a suppression file for UBSan under `.sanitizers/undefined.supp`
-in the GitHub repository, no LSan equivalent exists for the moment.
+LSan, TSan and UBSan support suppression files, which can be used to prevent
+tests failing due to known issues.
+NEURON includes a suppression file for TSan under `.sanitizers/thread.supp` and
+one for UBSan under `.sanitizers/undefined.supp` in the GitHub repository, no
+LSan equivalent exists for the moment.
 
 Note that LSan and MPI implementations typically do not play nicely together, so
 if you want to use LSan with NEURON, you may need to disable MPI or add a
 suppression file that is tuned to your MPI implementation.
 
-The GitHub Actions CI for NEURON at the time of writing includes test jobs for
-ASan and UBSan using Clang 14, but does not enable LSan.
+Similarly, TSan does not work very well with MPI and (especially) OpenMP
+implementations that were not compiled with TSan instrumentation (which they
+are typically not).
 
-CoreNEURON and NMODL both support the sanitizers in a similar way, but this has
-to be enabled explicitly: `-DNRN_SANITIZERS=undefined` will not compile
-CoreNEURON code with UBSan enabled, you must additionally pass
-`-DCORENRN_SANITIZERS=undefined` to enable instrumentation of CoreNEURON code.
-The equivalent variable for NMODL is `NMODL_SANITIZERS`.
-
-#### ThreadSanitizer (TSAN)
-`ThreadSanitizer` is a tool that detects data races. Be aware that a slowdown is incurred by using ThreadSanitizer of about 5x-15x, with typical memory overhead of about 5x-10x.  
-
-Here is how to enable it:
-```
-cmake ... -DNRN_ENABLE_TESTS=ON -DCMAKE_C_FLAGS="-O0 -fno-inline -g -fsanitize=thread" -DCMAKE_CXX_FLAGS="-O0 -fno-inline -g -fsanitize=thread" ..
-```
-You can then target a specific test (for example `ctest -VV -R test_name` or `bin/nrniv -nogui -nopython test.hoc`) and have a look at the generated output. In case of data races, you would see something similar to:
-```
-94: WARNING: ThreadSanitizer: data race (pid=2572)
-94:   Read of size 8 at 0x7b3c00000bf0 by thread T1:
-94:     #0 Cvode::at_time(double, NrnThread*) /home/savulesc/Workspace/nrn/src/nrncvode/cvodeobj.cpp:751 (libnrniv.so+0x38673e)
-94:     #1 at_time /home/savulesc/Workspace/nrn/src/nrncvode/cvodestb.cpp:133 (libnrniv.so+0x389e27)
-94:     #2 _nrn_current__IClamp /home/savulesc/Workspace/nrn/src/nrnoc/stim.c:266 (libnrniv.so+0x5b8f02)
-94:     #3 _nrn_cur__IClamp /home/savulesc/Workspace/nrn/src/nrnoc/stim.c:306 (libnrniv.so+0x5b9236)
-94:     #4 Cvode::rhs_memb(CvMembList*, NrnThread*) /home/savulesc/Workspace/nrn/src/nrncvode/cvtrset.cpp:68 (libnrniv.so+0x38a0eb)
-94:     #5 Cvode::rhs(NrnThread*) /home/savulesc/Workspace/nrn/src/nrncvode/cvtrset.cpp:35 (libnrniv.so+0x38a2f6)
-94:     #6 Cvode::fun_thread_transfer_part2(double*, NrnThread*) /home/savulesc/Workspace/nrn/src/nrncvode/occvode.cpp:671 (libnrniv.so+0x3bbbf1)
-94:     #7 Cvode::fun_thread(double, double*, double*, NrnThread*) /home/savulesc/Workspace/nrn/src/nrncvode/occvode.cpp:639 (libnrniv.so+0x3bd049)
-94:     #8 f_thread /home/savulesc/Workspace/nrn/src/nrncvode/cvodeobj.cpp:1532 (libnrniv.so+0x384f45)
-94:     #9 slave_main /home/savulesc/Workspace/nrn/src/nrnoc/multicore.cpp:337 (libnrniv.so+0x5157ee)
-94: 
-94:   Previous write of size 8 at 0x7b3c00000bf0 by main thread:
-94:     #0 Cvode::at_time(double, NrnThread*) /home/savulesc/Workspace/nrn/src/nrncvode/cvodeobj.cpp:753 (libnrniv.so+0x386759)
-94:     #1 at_time /home/savulesc/Workspace/nrn/src/nrncvode/cvodestb.cpp:133 (libnrniv.so+0x389e27)
-94:     #2 _nrn_current__IClamp /home/savulesc/Workspace/nrn/src/nrnoc/stim.c:266 (libnrniv.so+0x5b8f02)
-94:     #3 _nrn_cur__IClamp /home/savulesc/Workspace/nrn/src/nrnoc/stim.c:306 (libnrniv.so+0x5b9236)
-94:     #4 Cvode::rhs_memb(CvMembList*, NrnThread*) /home/savulesc/Workspace/nrn/src/nrncvode/cvtrset.cpp:68 (libnrniv.so+0x38a0eb)
-94:     #5 Cvode::rhs(NrnThread*) /home/savulesc/Workspace/nrn/src/nrncvode/cvtrset.cpp:35 (libnrniv.so+0x38a2f6)
-94:     #6 Cvode::fun_thread_transfer_part2(double*, NrnThread*) /home/savulesc/Workspace/nrn/src/nrncvode/occvode.cpp:671 (libnrniv.so+0x3bbbf1)
-..............................................................
-94: SUMMARY: ThreadSanitizer: data race /home/savulesc/Workspace/nrn/src/nrncvode/cvodeobj.cpp:751 in Cvode::at_time(double, NrnThread*)
-94: ==================
-```
+The GitHub Actions CI for NEURON at the time of writing includes three jobs
+using Ubuntu 22.04: ASan (but not LSan) using Clang 14, UBSan using Clang 14,
+and TSan using GCC 12.
+In addition, there is a macOS-based ASan build using AppleClang, which has the
+advantage that it uses `libc++` instead of `libstdc++`.
 
+NMODL supports the sanitizers in a similar way, but this has to be enabled
+explicitly: `-DNRN_SANITIZERS=undefined` will not compile NMODL code with UBSan
+enabled, you must additionally pass `-DNMODL_SANITIZERS=undefined` to enable
+instrumentation of NMODL code.
 
 Profiling and performance benchmarking
 --------------------------------------

external/coding-conventions

@@ -478,8 +478,7 @@ void nrn_setup(const char* filesdat,
     } else {
         nrn_multithread_job([](NrnThread* n) {
             Phase1 p1{n->id};
-            NrnThread& nt = *n;
-            p1.populate(nt, mut);
+            p1.populate(*n, mut);
         });
     }