[scudo] Allow for non-Android Shared TSD platforms, part 2

[cryptoad]

Summary

Follow up to D38826.

We introduce pthread_{get,set}specific versions of {get,set}CurrentTSD to
allow for non Android platforms to use the Shared TSD model.
We now allow SCUDO_TSD_EXCLUSIVE to be defined at compile time.

A couple of things:

• I know that #if SANITIZER_ANDROID is not ideal within a function, but in the end I feel it looks more compact and clean than going the .inc route; I am open to an alternative if anyone has one;
• SCUDO_TSD_EXCLUSIVE=1 requires ELF TLS support (and not emutls as this uses malloc). I haven't found anything to enforce that, so it's currently not checked.

[cryptoad]

Remove a comment that is no longer relevant: the pthread key is used.

[alekseyshl]

lib/scudo/scudo_platform.h

29–30

What's going to happen if we define SCUDO_TSD_EXCLUSIVE at compile time on these platforms?

lib/scudo/scudo_tsd_shared.cpp

21

Why is it not static anymore?

35

You do not need this PThreadKey on Android, right? Maybe it does make sense to separate implementations along these lines? Move PThreadKey and Android into separate cpp files and link what you need?

[cryptoad]

lib/scudo/scudo_platform.h

29–30

It will blow up at runtime (see main comment of the patch).
Android doesn't have ELF TLS but it's planned for some point. They do have emutls but it doesn't work for us.
Fuchsia I think supports a non emulated TLS, I'd have to double check.
I can put some safeguards?

lib/scudo/scudo_tsd_shared.cpp

21

It is used from scudo_tsd_shared.inc (defined as extern there).

35

Correct, it's not needed, it's useful on Android to makes things work on older platforms (the comment that I removed explains it).
I guess the issue with the separation is that it will end up affecting the .inc as well, which would have to be split or have further #ifs (the pthread key is used there). If you see a way to make this work, I can accommodate that.

[alekseyshl]

lib/scudo/scudo_platform.h

29–30

Yep, something like this:

#define SCUDO_TSD_EXCLUSIVE_SUPPORTED (!SANITIZER_ANDROID && !SANITIZER_FUCHSIA)

#ifndef SCUDO_TSD_EXCLUSIVE
...
#endif  // SCUDO_TSD_EXCLUSIVE

#if SCUDO_TSD_EXCLUSIVE && !SCUDO_TSD_EXCLUSIVE_SUPPORTED
# error ...
#endif

lib/scudo/scudo_tsd_shared.cpp

35

If you are going to support O and below, I guess, the current way is fine. If you are not going to support releases before N, I'd suggest to separate implementations and either have 4 more files:
for getCurrentTSD:

scudo_tsd_shared_android.inc
scudo_tsd_shared_pthread.inc

for setCurrentTSD and the function called from initOnce (empty for Android):

scudo_tsd_shared_android_impl.inc
scudo_tsd_shared_pthread_impl.inc

or, another option is to define SCUDO_TSD_CPP_ and add two files, scudo_tsd_shared_android.inc and scudo_tsd_shared_pthread.inc, the pthread one will do something like this (android inc will look a bit simpler):

#ifdef SCUDO_TSD_CPP_
pthread_key_t PThreadKey;
ALWAYS_INLINE void setCurrentTSD(ScudoTSD *TSD) { ... }
void initSharedTsdOnce() { ... }
#else
extern pthread_key_t PThreadKey;
ALWAYS_INLINE ScudoTSD* getCurrentTSD() { ... }
#endif

Both ways are pretty ugly.

Well, ok, I guess enough with that, I'll let you pick the way and accept your choice.

[cryptoad]

lib/scudo/scudo_tsd_shared.cpp

35

Does me picking a way allows for keeping things the way they are now? :)

[cryptoad]

Safeguarding the use of the exclusive TSD model against misuse (eg: using it
on a platform that doesn't support it).

[alekseyshl]

lib/scudo/scudo_platform.h

36

No a big deal at all, but why add "#else" part? Less nesting in these # statements is always better. But, anyway, whatever works better for you.

lib/scudo/scudo_tsd_shared.cpp

35

Yep, none of the ways I came up with is significantly better. So, up to you.

[cryptoad]

lib/scudo/scudo_platform.h

36

In my head it's one this check applies if SCUDO_TSD_EXCLUSIVE was defined initially, hence the #else.
I'll unnest it, it's not like we lose any performance that way!

lib/scudo/scudo_tsd_shared.cpp

35

I'd like to keep it that way for now.
If more platform specific stuff comes up in the future, I can see the split becoming necessary, but right now I feel it's less fragmented this way.

[cryptoad]

Unnesting the #if.

lib/scudo/scudo_platform.h

 #define SCUDO_PLATFORM_H_
 
 #include "sanitizer_common/sanitizer_allocator.h"
 
 #if !SANITIZER_LINUX && !SANITIZER_FUCHSIA
 # error "The Scudo hardened allocator is not supported on this platform."
 #endif
 
-#if SANITIZER_ANDROID || SANITIZER_FUCHSIA
+#define SCUDO_TSD_EXCLUSIVE_SUPPORTED (!SANITIZER_ANDROID && !SANITIZER_FUCHSIA)
+
+#ifndef SCUDO_TSD_EXCLUSIVE
+// SCUDO_TSD_EXCLUSIVE wasn't defined, use a default TSD model for the platform.
+# if SANITIZER_ANDROID || SANITIZER_FUCHSIA
 // Android and Fuchsia use a pool of TSDs shared between threads.
-# define SCUDO_TSD_EXCLUSIVE 0
-#elif SANITIZER_LINUX && !SANITIZER_ANDROID
+#  define SCUDO_TSD_EXCLUSIVE 0
+# elif SANITIZER_LINUX && !SANITIZER_ANDROID
 // Non-Android Linux use an exclusive TSD per thread.
-# define SCUDO_TSD_EXCLUSIVE 1
-#else
-# error "No default TSD model defined for this platform."
-#endif  // SANITIZER_ANDROID || SANITIZER_FUCHSIA
+#  define SCUDO_TSD_EXCLUSIVE 1
+# else
+#  error "No default TSD model defined for this platform."
+# endif  // SANITIZER_ANDROID || SANITIZER_FUCHSIA
+#endif  // SCUDO_TSD_EXCLUSIVE
+
+// If the exclusive TSD model is chosen, make sure the platform supports it.
+#if SCUDO_TSD_EXCLUSIVE && !SCUDO_TSD_EXCLUSIVE_SUPPORTED
+# error "The exclusive TSD model is not supported on this platform."
+#endif
 
 namespace __scudo {
 
 #if SANITIZER_CAN_USE_ALLOCATOR64
 # if defined(__aarch64__) && SANITIZER_ANDROID
 const uptr AllocatorSize = 0x2000000000ULL;  // 128G.
 typedef VeryCompactSizeClassMap SizeClassMap;
 # elif defined(__aarch64__)

lib/scudo/scudo_tsd_shared.cpp

 
 #include "scudo_tsd.h"
 
 #if !SCUDO_TSD_EXCLUSIVE
 
 namespace __scudo {
 
 static pthread_once_t GlobalInitialized = PTHREAD_ONCE_INIT;
-static pthread_key_t PThreadKey;
+pthread_key_t PThreadKey;
 
 static atomic_uint32_t CurrentIndex;
 static ScudoTSD *TSDs;
 static u32 NumberOfTSDs;
 
 // sysconf(_SC_NPROCESSORS_{CONF,ONLN}) cannot be used as they allocate memory.
 static uptr getNumberOfCPUs() {
   cpu_set_t CPUs;
   CHECK_EQ(sched_getaffinity(0, sizeof(cpu_set_t), &CPUs), 0);
   return CPU_COUNT(&CPUs);
 }
 
 static void initOnce() {
-  // Hack: TLS_SLOT_TSAN was introduced in N. To be able to use it on M for
-  // testing, we create an unused key. Since the key_data array follows the tls
-  // array, it basically gives us the extra entry we need.
-  // TODO(kostyak): remove and restrict to N and above.
   CHECK_EQ(pthread_key_create(&PThreadKey, NULL), 0);
   initScudo();
   NumberOfTSDs = getNumberOfCPUs();
   if (NumberOfTSDs == 0)
     NumberOfTSDs = 1;
   if (NumberOfTSDs > 32)
     NumberOfTSDs = 32;
   TSDs = reinterpret_cast<ScudoTSD *>(
       MmapOrDie(sizeof(ScudoTSD) * NumberOfTSDs, "ScudoTSDs"));
   for (u32 i = 0; i < NumberOfTSDs; i++)
     TSDs[i].init(/*Shared=*/true);
 }
 
 ALWAYS_INLINE void setCurrentTSD(ScudoTSD *TSD) {
+#if SANITIZER_ANDROID
   *get_android_tls_ptr() = reinterpret_cast<uptr>(TSD);
+#else
+  CHECK_EQ(pthread_setspecific(PThreadKey, reinterpret_cast<void *>(TSD)), 0);
+#endif  // SANITIZER_ANDROID
 }
 
 void initThread(bool MinimalInit) {
   pthread_once(&GlobalInitialized, initOnce);
   // Initial context assignment is done in a plain round-robin fashion.
   u32 Index = atomic_fetch_add(&CurrentIndex, 1, memory_order_relaxed);
   setCurrentTSD(&TSDs[Index % NumberOfTSDs]);
 }

lib/scudo/scudo_tsd_shared.inc

 //===----------------------------------------------------------------------===//
 
 #ifndef SCUDO_TSD_H_
 # error "This file must be included inside scudo_tsd.h."
 #endif  // SCUDO_TSD_H_
 
 #if !SCUDO_TSD_EXCLUSIVE
 
+extern pthread_key_t PThreadKey;
+
 ALWAYS_INLINE ScudoTSD* getCurrentTSD() {
+#if SANITIZER_ANDROID
   return reinterpret_cast<ScudoTSD *>(*get_android_tls_ptr());
+#else
+  return reinterpret_cast<ScudoTSD *>(pthread_getspecific(PThreadKey));
+#endif  // SANITIZER_ANDROID
 }
 
 ALWAYS_INLINE void initThreadMaybe(bool MinimalInit = false) {
   if (LIKELY(getCurrentTSD()))
     return;
   initThread(MinimalInit);
 }