[OpenMP][Tool] Runtime warning for missing TSan-option

[protze.joachim]

Summary

TSan reports for any OpenMP application a race on the initialization of a runtime internal mutex:

Atomic read of size 1 at 0x7b6800005940 by thread T4:
  #0 pthread_mutex_lock <null> (a.out+0x43f39e)
  #1 __kmp_resume_64 <null> (libomp.so.5+0x84db4)

Previous write of size 1 at 0x7b6800005940 by thread T7:
  #0 pthread_mutex_init <null> (a.out+0x424793)
  #1 __kmp_suspend_initialize_thread <null> (libomp.so.5+0x8422e)

According to @AndreyChurbanov this is a false positiv report, as the control flow of the runtime guarantees the ordering of the mutex initialization and the lock: https://software.intel.com/en-us/forums/intel-open-source-openmp-runtime-library/topic/530363

To suppress this report, I suggest the use of TSAN_OPTIONS='ignore_uninstrumented_modules=1'. With this patch, a runtime warning is provided in case an OpenMP application is built with Tsan and executed without this Tsan-option.

[protze.joachim]

Attached the right diff.

[jdoerfert]

Wasn't the conclusion of the intel forum thread that we should remove a call? Is this still necessary afterward?

[protze.joachim]

The conclusion of the discussion in the intel forum thread was, that the call from __kmp_resume to __kmp_suspend_initialize_thread is superflous, because the thread will call __kmp_suspend_initialize_thread always during the own initialization before changing a variable to signal readiness.

[jdoerfert]

In D70412#1763950, @protze.joachim wrote:

The conclusion of the discussion in the intel forum thread was, that the call from __kmp_resume to __kmp_suspend_initialize_thread is superflous, because the thread will call __kmp_suspend_initialize_thread always during the own initialization before changing a variable to signal readiness.

But that doesn't make this obsolete?

This looks generally fine to me I just want to understand why we can't fix the runtime instead.

[Hahnfeld]

This revision was not accepted before being committed!

openmp/tools/archer/ompt-tsan.cpp

868

This memory is leaked, it should be deleted in ompt_tsan_finalize

[protze.joachim]

In D70412#1820406, @Hahnfeld wrote:

This revision was not accepted before being committed!

It was a misunderstanding on my side. Johannes told me that he is fine with the changes before I pushed. I didn't check that the accepted flag was not on the patch.
What procedure do you suggest?

[Hahnfeld]

In D70412#1821136, @protze.joachim wrote:

In D70412#1820406, @Hahnfeld wrote:

This revision was not accepted before being committed!

It was a misunderstanding on my side. Johannes told me that he is fine with the changes before I pushed. I didn't check that the accepted flag was not on the patch.
What procedure do you suggest?

Now that the patch is already in the repository, I don't know how much sense it would make to revert it in full. We should still fix the memory leak asap and make sure it's in the release.

openmp/tools/archer/ompt-tsan.cpp

868

Do we really need global lifetime for tsan_flags? Right now it would be enough to just have the object allocated on the stack and let the compiler handle the cleanup.

[jdoerfert]

In D70412#1821136, @protze.joachim wrote:

In D70412#1820406, @Hahnfeld wrote:

This revision was not accepted before being committed!

It was a misunderstanding on my side. Johannes told me that he is fine with the changes before I pushed. I didn't check that the accepted flag was not on the patch.
What procedure do you suggest?

I did say I was generally fine with this but it should not trigger if we build the runtime with tsan instrumentation.
That, and especially the memory leak @Hahnfeld found, need to be taken care of ASAP.

Also, when was the CREDITS.txt change added?

[Hahnfeld]

Long addressed by D72779, but I can't seem to be able to close this (?)

[protze.joachim]

Phab also only lets me abandon the review

openmp/CREDITS.txt

 
 N: Pavel Neytchev
 D: Contributor to testsuite from OpenUH
 
 N: Steven Noonan
 E: [e-mail]
 D: Patches for the ARM architecture and removal of several inconsistencies.
 
+N: Joachim Protze
+E: [e-mail]
+D: OpenMP Tools Interface, Archer tool
+
 N: Alp Toker
 E: [e-mail]
 D: Making build work for FreeBSD.
 
 N: Cheng Wang
 D: Contributor to testsuite from OpenUH

openmp/tools/archer/ompt-tsan.cpp

#endif
           continue;
         std::cerr << "Illegal values for ARCHER_OPTIONS variable: " << token
                   << std::endl;
       }
     }
   }
 };
 
+class TsanFlags {
+public:
+  int ignore_noninstrumented_modules;
+
+  TsanFlags(const char *env) : ignore_noninstrumented_modules(0) {
+    if (env) {
+      std::vector<std::string> tokens;
+      std::string token;
+      std::string str(env);
+      std::istringstream iss(str);
+      while (std::getline(iss, token, ' '))
+        tokens.push_back(token);
+
+      for (std::vector<std::string>::iterator it = tokens.begin();
+           it != tokens.end(); ++it) {
+        // we are interested in ignore_noninstrumented_modules to print a
+        // warning
+        if (sscanf(it->c_str(), "ignore_noninstrumented_modules=%d",
+                   &ignore_noninstrumented_modules))
+          continue;
+      }
+    }
+  }
+};
+
 #if (LLVM_VERSION) >= 40
 extern "C" {
 int __attribute__((weak)) __archer_get_omp_status();
 void __attribute__((weak)) __tsan_flush_memory() {}
 }
 #endif
 ArcherFlags *archer_flags;
+TsanFlags *tsan_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 .

 
 #define SET_CALLBACK(event) SET_CALLBACK_T(event, event)
 
 static int ompt_tsan_initialize(ompt_function_lookup_t lookup,
                                 int device_num,
                                 ompt_data_t *tool_data) {
   const char *options = getenv("ARCHER_OPTIONS");
   archer_flags = new ArcherFlags(options);
+  options = getenv("TSAN_OPTIONS");
+  tsan_flags = new TsanFlags(options);
 
   ompt_set_callback_t ompt_set_callback =
       (ompt_set_callback_t)lookup("ompt_set_callback");
   if (ompt_set_callback == NULL) {
     std::cerr << "Could not set callback, exiting..." << std::endl;
     std::exit(1);
   }
   ompt_get_parallel_info =
static int ompt_tsan_initialize(ompt_function_lookup_t lookup,
 
   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");
   return 1; // success
 }
 
 static void ompt_tsan_finalize(ompt_data_t *tool_data) {
   if (archer_flags->print_max_rss) {
     struct rusage end;
     getrusage(RUSAGE_SELF, &end);
     printf("MAX RSS[KBytes] during execution: %ld\n", end.ru_maxrss);