[Openmp-commits] [PATCH] D103607: [OpenMP][Tools] Fix Archer for MACOS

Sun Jun 6 23:12:22 PDT 2021

Hahnfeld added inline comments.

================
Comment at: openmp/tools/archer/ompt-tsan.cpp:164
   fptr = (int (*)())dlsym(RTLD_DEFAULT, "RunningOnValgrind");
-  if (fptr && fptr != RunningOnValgrind)
+  if (!fptr || fptr == RunningOnValgrind)
     runOnTsan = 0;
----------------
protze.joachim wrote:
> Hahnfeld wrote:
> > Can you explain this change? Why do we disable TSan if `dlsym` finds the current function? (`RunningOnValgrind` resolves to the current function, right?)
> 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.
>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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103607/new/

https://reviews.llvm.org/D103607