[PATCH] D32649: [scudo] Add Android support

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added a comment.

Thanks Dmitry. Additional question in the comment before making the code modifications.



================
Comment at: lib/scudo/scudo_tls.h:33
   void commitBack();
+  u64 getUnusedSince() { return atomic_load_relaxed(&UnusedSince); }
+  bool tryLock();
----------------
dvyukov wrote:
> This one liner is used only once, just inline it there. Additional levels of indirection do not help understanding code. 
Will do.
Additionally, UnusedSince and Mutex are only used for Android. Is worth adding an #if to not have them in the non-Android version, or is the clutter not worth it?


================
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__)
----------------
dvyukov wrote:
> 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();
Correct. TSan uses only the AArch64 & x64 versions, and I needed them all (mips being bordeline but still included).
I will move it to sanitizer_common.


================
Comment at: lib/scudo/scudo_tls_android.h:84
+
+ALWAYS_INLINE ScudoThreadContext *getThreadContextLocked() {
+  ScudoThreadContext *ThreadContext =
----------------
dvyukov wrote:
> 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();
>       }
Will do.


================
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++) {
----------------
dvyukov wrote:
> 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.
Will do.


https://reviews.llvm.org/D32649