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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/scudo_platform.h:41
+# endif
+#endif  // SCUDO_TSD_EXCLUSIVE
 
----------------
alekseyshl wrote:
> 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.
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!


================
Comment at: lib/scudo/scudo_tsd_shared.cpp:35
 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();
----------------
alekseyshl wrote:
> cryptoad wrote:
> > alekseyshl wrote:
> > > cryptoad wrote:
> > > > alekseyshl wrote:
> > > > > 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?
> > > > 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.
> > > 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.
> > Does me picking a way allows for keeping things the way they are now? :)
> Yep, none of the ways I came up with is significantly better. So, up to you.
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.


https://reviews.llvm.org/D38854