[PATCH] D32649: [scudo] Add Android support

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added inline comments.


================
Comment at: lib/scudo/scudo_tls.h:33
   void commitBack();
+  u64 getUnusedSince() { return atomic_load_relaxed(&UnusedSince); }
+  bool tryLock();
----------------
This one liner is used only once, just inline it there. Additional levels of indirection do not help understanding code. 


================
Comment at: lib/scudo/scudo_tls_android.h:46
+# define __get_tls() \
+  ({ void** __v; __asm__("mrs %0, tpidr_el0" : "=r"(__v)); __v; })
+#elif defined(__arm__)
----------------
This is duplicated from tsan, right? Please move this bulky chunk of tricky assembly into sanitizer_common (along with TLS_SLOT_TSAN). No reason to have 2 copies of a complex code, and we can potentially use it in asan and later in msan. Something like:

    uptr *get_android_tls_ptr();


================
Comment at: lib/scudo/scudo_tls_android.h:84
+
+ALWAYS_INLINE ScudoThreadContext *getThreadContextLocked() {
+  ScudoThreadContext *ThreadContext =
----------------
By sanitizer convention functions ending with Locked mean that function call normally needs to be protected with a mutex, but this is version for the case when caller already holds the mutex. For such case we generally use AndLock suffix (e.g. GetIfExistsAndLock).
Please rename this to getThreadContextAndLock. It will also make relation between Lock and Unlock much clearer in the following code:

      ScudoThreadContext *ThreadContext = getThreadContextAndLock();
      if (LIKELY(ThreadContext)) {
        ...
        ThreadContext->unlock();
      }


================
Comment at: lib/scudo/scudo_tls_android.h:91
+    return ThreadContext;
+  // If it failed, go through all the contexts and find the first unlocked one.
+  for (u32 i = 0; i < NumberOfContexts; i++) {
----------------
I would move everything starting from this line to getThreadContextLockedSlow. I think this is where slow path starts.
Then we also don't need ThreadContexts and NumberOfContexts in header.


https://reviews.llvm.org/D32649