[PATCH] D62258: [scudo][standalone] Introduce the thread specific data structures

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: lib/scudo/standalone/tests/tsd_test.cc:34
+  }
+  void reset() { memset(this, 0, sizeof(*this)); }
+
----------------
I assume allocators will always be linker-initialized, which is why we have `reset` for testing instead of an `init`?


================
Comment at: lib/scudo/standalone/tests/tsd_test.cc:49
+  std::unique_ptr<AllocatorT> Allocator(new AllocatorT);
+  Allocator.get()->reset();
+  EXPECT_FALSE(Allocator.get()->isInitialized());
----------------
`Allocator->` is cleaner than `Allocator.get()->`, unless there's a good reason to get the raw pointer.


================
Comment at: lib/scudo/standalone/tsd.h:27
+    Instance->initCache(&Cache);
+    DestructorIterations = PTHREAD_DESTRUCTOR_ITERATIONS;
+  }
----------------
`Mutex.initLinkerInitialized()`


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:28
+  void initLinkerInitialized() { Mutex.initLinkerInitialized(); }
+  void init() { memset(this, 0, sizeof(*this)); }
+
----------------
Shouldn't this also call `initLinkerInitialized`?


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:60
+
+  NOINLINE void initThread(Allocator *Instance, bool MinimalInit) {
+    if (UNLIKELY(!OnceDone))
----------------
Will threads have different allocators?


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:64
+    if (MinimalInit)
+      return;
+    CHECK_EQ(
----------------
What is the use-case for `MinimalInit` initialization?


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:74
+  pthread_key_t PThreadKey;
+  TSD<Allocator> *FallbackTSD;
+  static THREADLOCAL ThreadState State;
----------------
Any reason this needs to be a pointer?


================
Comment at: lib/scudo/standalone/tsd_exclusive.h:94
+  // quarantine and swallowing the cache.
+  const u8 N = TSDRegistryT::ThreadTSD.DestructorIterations;
+  if (N > 1U) {
----------------
Assigning to `N` seems pointless.  Can we simplify to:
```
if (TSDRegistryT::ThreadTSD.DestructorIterations > 1) {
  TSDRegistryT::ThreadTSD.DestructorIterations--;
 ...
}
```


================
Comment at: lib/scudo/standalone/tsd_shared.h:21
+  void initLinkerInitialized() { Mutex.initLinkerInitialized(); }
+  void init() { memset(this, 0, sizeof(*this)); }
+
----------------
Shouldn't this also call `initLinkerInitialized`?


================
Comment at: lib/scudo/standalone/tsd_shared.h:23
+
+  ALWAYS_INLINE void initThreadMaybe(Allocator *Instance,
+                                     UNUSED bool MinimalInit) {
----------------
Should Allocator initialization just be done in `initLinkerInitialized`?


================
Comment at: lib/scudo/standalone/tsd_shared.h:61
+      if (A == 1)
+        CoPrimes[NumberOfCoPrimes++] = I + 1;
+    }
----------------
What are the CoPrimes used for, and what is the strange calculation above doing?


================
Comment at: lib/scudo/standalone/tsd_shared.h:74
+        pthread_setspecific(PThreadKey, reinterpret_cast<void *>(CurrentTSD)),
+        0);
+#endif
----------------
Do we really need three ways to do TLS?  Can we just use pthreads for all?


================
Comment at: lib/scudo/standalone/tsd_shared.h:97
+  NOINLINE TSD<Allocator> *getTSDAndLockSlow(TSD<Allocator> *CurrentTSD) {
+    if (MaxTSDCount > 1U && NumberOfTSDs > 1U) {
+      // Use the Precedence of the current TSD as our random seed. Since we are
----------------
Is it sufficient to just check `NumberOfTSDs`?


================
Comment at: lib/scudo/standalone/tsd_shared.h:123
+        if (Index >= NumberOfTSDs)
+          Index -= NumberOfTSDs;
+      }
----------------
`Index %= NumberOfTSDs`


================
Comment at: lib/scudo/standalone/tsd_shared.h:144
+  u32 NumberOfTSDs;
+#if SCUDO_LINUX && !SCUDO_ANDROID
+  static THREADLOCAL TSD<Allocator> *ThreadTSD;
----------------
Why do we need these guards here but not in tsd_exclusive.h?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D62258