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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld accepted this revision.
Hahnfeld added a comment.
This revision is now accepted and ready to land.

LG



================
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:
> > 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?
> 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.
Ah, now I understand, thanks! (did I write this back then?)


================
Comment at: openmp/tools/archer/ompt-tsan.cpp:1020
+  if (NULL == (f = fSig dlsym(RTLD_DEFAULT, #f)))                              \
+  printf("Unable to find TSan function " #f ".\n")
+
----------------
Hahnfeld wrote:
> 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?
Can you see what `clang-format` thinks about this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103607