[PATCH] D32649: [scudo] Add Android support

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/scudo_tls_android.h:28
+  }
+  if (atomic_load_relaxed(&UnusedSince) == 0)
+    atomic_store_relaxed(&UnusedSince, NanoTime());
----------------
alekseyshl wrote:
> Can we rename UnusedSince to something else? It's a bit confusing to read the code, it is set when the context is, in fact, in use (by some other thread). How about InSlowLockQueueSince or SlowLockOrder or SlowLockPrecedence? Two latter ones abstract the fact that we use timespamp, which, to me, is a good thing,
Good idea. Will do.


================
Comment at: lib/scudo/scudo_tls_linux.h:35
 };
-extern thread_local ThreadState ScudoThreadState;
-extern thread_local ScudoThreadContext ThreadLocalContext;
+__attribute__((tls_model("initial-exec")))
+extern THREADLOCAL ThreadState ScudoThreadState;
----------------
alekseyshl wrote:
> Is the explicit model necessary?
I talked to Dmitry offline about this.
When looking at the assembly, the portion of codes using `ScudoThreadState` looked like (on all archs):
```
                mov     r15, cs:_ZTHN7__scudo16ScudoThreadStateE_ptr
                test    r15, r15
                jz      short loc_7FD57
                call    __ZTHN7__scudo16ScudoThreadStateE
loc_7FD57:                              ; CODE XREF: __scudo::scudoMalloc(ulong,__scudo::AllocType)+20 j
                mov     r13, 0FFFFFFFFFFFFFFC0h
                cmp     byte ptr fs:[r13+0], 0
                jz      loc_80020
loc_7FD6A:                              ; CODE XREF: __scudo::scudoMalloc(ulong,__scudo::AllocType)+2F5 j
```
The compiler creates a weak symbol for the extern thread_local variables, looks them up, and if they exist, calls them, then proceeds with the regular flow of code.
The overhead has no use (those weak symbols are never defined), and goes away by explicitly specifying the model: it just read it from the TLS.



https://reviews.llvm.org/D32649