[PATCH] D32440: [scudo] Move thread local variables into their own files

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added inline comments.


================
Comment at: lib/scudo/scudo_tls_linux.cpp:59
+void initThreadMaybe() {
+  if (LIKELY(ScudoThreadState != ThreadNotInitialized))
+    return;
----------------
cryptoad wrote:
> dvyukov wrote:
> > The fast path needs to be inlinable (defined in header file).
> Previous reviews of similar changes have edged towards less ifdef and a clear separation of the platform specific codes in separate cpp, yielding less complicated code. The issue I can see happening is multiplication of ifdefs in this header when SANITIZER_ANDROID comes into play next if defining the fast path in the header. Is this OK?
> 
You can do the same we do for source files. Namely:

  // scudo_tls.h
  ...
  #include <scudo_tls_linux.h>
  #include <scudo_tls_android.h>
  ...

  // scudo_tls_linux.h
  #if SANITIZER_LINUX
  ... necessary declarations and bodies of inline functions go here ...
  #endif

  // scudo_tls_android.h
  #if SANITIZER_ANDROID
  ... necessary declarations and bodies of inline functions go here ...
  #endif

I understand their concerns, but some pieces of sanitizer runtimes are kinda performance-critical. We do have mixed per-platform code in interception.h library, in tsan_platform.h. And tsan_update_shadow_word_inl.h is just awful in the name of performance. These are intentional compromises.


https://reviews.llvm.org/D32440