[PATCH] D32649: [scudo] Add Android support

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added inline comments.


================
Comment at: lib/scudo/scudo_tls.h:28
 struct ALIGNED(64) ScudoThreadContext {
  public:
+  StaticSpinMutex Mutex;
----------------
You don't need public here.


================
Comment at: lib/scudo/scudo_tls.h:47
 
 }  // namespace __scudo
 
----------------
How about structuring it this way:

  namespace __scudo {

  #include "scudo_tls_context_android.inc"
  #include "scudo_tls_context_linux.inc"

  struct ALIGNED(64) ScudoThreadContext : public ScudoThreadContextPlatform {
    AllocatorCache Cache;
    Xorshift128Plus Prng;
    uptr QuarantineCachePlaceHolder[4];
  };

  void initThread();

  // Fastpath functions are defined in the following platform specific headers.
  #include "scudo_tls_android.inc"
  #include "scudo_tls_linux.inc"

  }  // namespace __scudo


You can define lock/unlock/etc functions inline on the struct and Linux version might not need them defined at all (hard to tell for me off the bat).




================
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;
----------------
cryptoad wrote:
> 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.
> 
Good to know. My understanding was that compiler is supposed to select the most restricted model which works for the code being compiled. Apparently, it's not always the case.


https://reviews.llvm.org/D32649