[OpenMP][Tools] Fix Archer for MACOS

[protze.joachim]

Summary

Archer uses weak symbol overloads of TSan functions to enable loading the tool even if the application is not built with TSan. For MACOS the tool collects the function pointer at runtime.
When adding the function entry/exit markers, we missed to add the functions in the MACOS codepath.
This patch also replaces the repeated function lookup by a single initial function lookup.

[Hahnfeld]

openmp/tools/archer/ompt-tsan.cpp

157–160

Can you explain this change? Why do we disable TSan if dlsym finds the current function? (RunningOnValgrind resolves to the current function, right?)

941

Can you indent this line? It's the body of the if.

As a general comment, maybe wrap the entire macro in do { ... } while (0) so the semicolon doesn't terminate the if body?

[protze.joachim]

openmp/tools/archer/ompt-tsan.cpp

157–160

I never had a chance to test the library on Mac. While preparing this patch, I tested the library on my Linux system by manually enabling the ifdefs for mac. From my understanding the library should never have worked on Mac.

Line 1090 sets runOnTsan=1. If any other RunningOnValgrind is found (and executed in the weak function approach), the function will not modify runOnTsan. If no other RunningOnValgrind is found, the function should set runOnTsan=0. In this case, Archer will return NULL from ompt_start_tool and allow other tools to get loaded.

I think, the general question is whether it's worth to still have all the symbols in the library or whether we should allow dlopen to fail if the application is not built with TSan? The OpenMP runtime now has OMP_TOOL_VERBOSE_INIT which allows to understand why dlopen failed.

[Hahnfeld]

openmp/tools/archer/ompt-tsan.cpp

157–160

From my memory, I think the dynamic linker on macOS always prefers functions in the current image - that's why we have to play the funny dlsym tricks in the first place, right? So if we end up in this function, and fptr == RunningOnValgrind / the current function, we're not running under Valgrind and should not set runOnTsan = 0;. Am I missing something?

[protze.joachim]

openmp/tools/archer/ompt-tsan.cpp

157–160

Tsan implements this function. The goal of the function is not to distinguish between TSan/Valgrind, but rather to see whether a consumer for the DR annotations is available. I could not find valgrind documentation about this function. Probably we completely missed the idea of the function, when we used the function to detect whether TSan is available in the application.

As I wrote in my last comment, we could just remove all the fallback implementation of the TSan functions from this library and fail to load the library when the application is not built with TSan.

[Hahnfeld]

LG

openmp/tools/archer/ompt-tsan.cpp

157–160

Ah, now I understand, thanks! (did I write this back then?)

941

Can you see what clang-format thinks about this?

openmp/tools/archer/ompt-tsan.cpp

 #if (LLVM_VERSION) >= 40
 extern "C" {
 int __attribute__((weak)) __archer_get_omp_status();
 void __attribute__((weak)) __tsan_flush_memory() {}
 }
 #endif
 ArcherFlags *archer_flags;
 
-// The following definitions are pasted from "llvm/Support/Compiler.h" to allow
-// the code
-// to be compiled with other compilers like gcc:
-
 #ifndef TsanHappensBefore
 // Thread Sanitizer is a tool that finds races in code.
 // See http://code.google.com/p/data-race-test/wiki/DynamicAnnotations .
 // tsan detects these exact functions by name.
 extern "C" {
 #if (defined __APPLE__ && defined __MACH__)
-static void AnnotateHappensAfter(const char *file, int line,
-                                 const volatile void *cv) {
-  void (*fptr)(const char *, int, const volatile void *);
-
-  fptr = (void (*)(const char *, int, const volatile void *))dlsym(
-      RTLD_DEFAULT, "AnnotateHappensAfter");
-  (*fptr)(file, line, cv);
-}
-static void AnnotateHappensBefore(const char *file, int line,
-                                  const volatile void *cv) {
-  void (*fptr)(const char *, int, const volatile void *);
-
-  fptr = (void (*)(const char *, int, const volatile void *))dlsym(
-      RTLD_DEFAULT, "AnnotateHappensBefore");
-  (*fptr)(file, line, cv);
-}
-static void AnnotateIgnoreWritesBegin(const char *file, int line) {
-  void (*fptr)(const char *, int);
-
-  fptr = (void (*)(const char *, int))dlsym(RTLD_DEFAULT,
-                                            "AnnotateIgnoreWritesBegin");
-  (*fptr)(file, line);
-}
-static void AnnotateIgnoreWritesEnd(const char *file, int line) {
-  void (*fptr)(const char *, int);
-
-  fptr = (void (*)(const char *, int))dlsym(RTLD_DEFAULT,
-                                            "AnnotateIgnoreWritesEnd");
-  (*fptr)(file, line);
-}
-static void AnnotateNewMemory(const char *file, int line,
-                              const volatile void *cv, size_t size) {
-  void (*fptr)(const char *, int, const volatile void *, size_t);
-
-  fptr = (void (*)(const char *, int, const volatile void *, size_t))dlsym(
-      RTLD_DEFAULT, "AnnotateNewMemory");
-  (*fptr)(file, line, cv, size);
-}
+static void (*AnnotateHappensAfter)(const char *, int, const volatile void *);
+static void (*AnnotateHappensBefore)(const char *, int, const volatile void *);
+static void (*AnnotateIgnoreWritesBegin)(const char *, int);
+static void (*AnnotateIgnoreWritesEnd)(const char *, int);
+static void (*AnnotateNewMemory)(const char *, int, const volatile void *,
+                                 size_t);
+static void (*__tsan_func_entry)(const void *);
+static void (*__tsan_func_exit)(void);
+
 static int RunningOnValgrind() {
   int (*fptr)();
 
   fptr = (int (*)())dlsym(RTLD_DEFAULT, "RunningOnValgrind");
-  if (fptr && fptr != RunningOnValgrind)
+  // If we found RunningOnValgrind other than this function, we assume
+  // Annotation functions present in this execution and leave runOnTsan=1
+  // otherwise we change to runOnTsan=0
+  if (!fptr || fptr == RunningOnValgrind)
     runOnTsan = 0;
   return 0;
 }
 #else
 void __attribute__((weak))
 AnnotateHappensAfter(const char *file, int line, const volatile void *cv) {}
 void __attribute__((weak))
 AnnotateHappensBefore(const char *file, int line, const volatile void *cv) {}
static int ompt_tsan_initialize(ompt_function_lookup_t lookup, int device_num,
   ompt_get_thread_data = (ompt_get_thread_data_t)lookup("ompt_get_thread_data");
 
   if (ompt_get_parallel_info == NULL) {
     fprintf(stderr, "Could not get inquiry function 'ompt_get_parallel_info', "
                     "exiting...\n");
     exit(1);
   }
 
+#if (defined __APPLE__ && defined __MACH__)
+#define findTsanFunction(f, fSig)                                              \
+  do {                                                                         \
+    if (NULL == (f = fSig dlsym(RTLD_DEFAULT, #f)))                            \
+      printf("Unable to find TSan function " #f ".\n");                        \
+  } while (0)
+
+  findTsanFunction(AnnotateHappensAfter,
+                   (void (*)(const char *, int, const volatile void *)));
+  findTsanFunction(AnnotateHappensBefore,
+                   (void (*)(const char *, int, const volatile void *)));
+  findTsanFunction(AnnotateIgnoreWritesBegin, (void (*)(const char *, int)));
+  findTsanFunction(AnnotateIgnoreWritesEnd, (void (*)(const char *, int)));
+  findTsanFunction(
+      AnnotateNewMemory,
+      (void (*)(const char *, int, const volatile void *, size_t)));
+  findTsanFunction(__tsan_func_entry, (void (*)(const void *)));
+  findTsanFunction(__tsan_func_exit, (void (*)(void)));
+#endif
+
   SET_CALLBACK(thread_begin);
   SET_CALLBACK(thread_end);
   SET_CALLBACK(parallel_begin);
   SET_CALLBACK(implicit_task);
   SET_CALLBACK(sync_region);
   SET_CALLBACK(parallel_end);
 
   SET_CALLBACK(task_create);
   SET_CALLBACK(task_schedule);
   SET_CALLBACK(dependences);
 
   SET_CALLBACK_T(mutex_acquired, mutex);
   SET_CALLBACK_T(mutex_released, mutex);
   SET_OPTIONAL_CALLBACK_T(reduction, sync_region, hasReductionCallback,
                           ompt_set_never);
 
   if (!tsan_flags.ignore_noninstrumented_modules)
     fprintf(stderr,
             "Warning: please export "
             "TSAN_OPTIONS='ignore_noninstrumented_modules=1' "
             "to avoid false positive reports from the OpenMP runtime!\n");
   if (archer_flags->ignore_serial)
     TsanIgnoreWritesBegin();
+
   return 1; // success
 }
 
 static void ompt_tsan_finalize(ompt_data_t *tool_data) {
   if (archer_flags->ignore_serial)
     TsanIgnoreWritesEnd();
   if (archer_flags->print_max_rss) {
     struct rusage end;
if (!archer_flags->enabled) {
     if (archer_flags->verbose)
       std::cout << "Archer disabled, stopping operation" << std::endl;
     delete archer_flags;
     return NULL;
   }
 
   static ompt_start_tool_result_t ompt_start_tool_result = {
       &ompt_tsan_initialize, &ompt_tsan_finalize, {0}};
+
+  // The OMPT start-up code uses dlopen with RTLD_LAZY. Therefore, we cannot
+  // rely on dlopen to fail if TSan is missing, but would get a runtime error
+  // for the first TSan call. We use RunningOnValgrind to detect whether
+  // an implementation of the Annotation interface is available in the
+  // execution or disable the tool (by returning NULL).
+
   runOnTsan = 1;
   RunningOnValgrind();
   if (!runOnTsan) // if we are not running on TSAN, give a different tool the
                   // chance to be loaded
   {
     if (archer_flags->verbose)
       std::cout << "Archer detected OpenMP application without TSan "
                    "stopping operation"