[Openmp-commits] [PATCH] D13072: [OpenMP] Enable ThreadSanitizer to check OpenMP programs

[Jonathan Peyton via Openmp-commits]

jlpeyton added inline comments.


================
Comment at: runtime/CMakeLists.txt:322
+
+set(LIBOMP_TSAN_NAME)
+if(${LIBOMP_TSAN_SUPPORT})
----------------
Unused?


================
Comment at: runtime/CMakeLists.txt:323-326
+if(${LIBOMP_TSAN_SUPPORT})
+  add_definitions(-D TSAN_SUPPORT=1)
+endif()
+
----------------
It seems TSAN is only supported on Linux X86-64 so inside cmake/config-ix.cmake this should be appended:
```
if("${CMAKE_SYSTEM_NAME}" MATCHES "Linux" AND ${INTEL64})
  set(LIBOMP_HAVE_TSAN_SUPPORT TRUE)
else()
  set(LIBOMP_HAVE_TSAN_SUPPORT FALSE)
endif()
```

and in the main CMakeLists.txt file (You'll see this pattern a lot in this file):
```
set(LIBOMP_TSAN_SUPPORT FALSE CACHE BOOL
  "TSAN-support?")
if(LIBOMP_TSAN_SUPPORT AND (NOT LIBOMP_HAVE_TSAN_SUPPORT))
  libomp_error_say("TSAN functionality requested but not available")
endif()
```

Also, as Jonas pointed out, we usually use the kmp_config.h.cmake file instead of sending in definitions.  So in kmp_config.h.cmake there should be:
```
#cmakedefine01 LIBOMP_TSAN_SUPPORT
#if LIBOMP_TSAN_SUPPORT
#define TSAN_SUPPORT
#endif
```


================
Comment at: runtime/src/kmp_barrier.cpp:310
            any more - it could be deallocated by the master thread at any time.  */
+        ANNOTATE_BARRIER_BEFORE(this_thr);
         kmp_flag_64 flag(&thr_bar->b_arrived, other_threads[parent_tid]);
----------------
I'm not familiar with the thread sanitizer or what these functions do.  Can you explain a little bit about BARRIER_BEFORE and AFTER and what they do with their argument?


================
Comment at: runtime/src/tsan_annotations.h:19
+#define TSAN_ANNOTATIONS_H
+
+/* types as used in tsan/rtl/tsan_interface_ann.cc */
----------------
#include "kmp_config.h"


https://reviews.llvm.org/D13072