[PATCH] D104085: [compiler-rt][hwasan] Setup hwasan thread handling on Fuchsia

[Roland McGrath via Phabricator via cfe-commits]

mcgrathr added a comment.

Usually we like to split changes up into separate small changes for the pure refactoring, and then changes that purely add new Fuchsia-specific code.
I'll do an initial review round of the Fuchsia code here since you've sent it.  But then I think this review should be tightened to just the pure refactoring, and we should have a separate review thread for the Fuchsia parts.



================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:26
+// used to always find the hwasan thread object associated with the current
+// running thread.
+SANITIZER_INTERFACE_ATTRIBUTE
----------------
Specifically call out here that the compiler generates direct TLS references to this and that's why it has a public C ABI name.

For Fuchsia the hwasan will always be in a DSO that is a dependency of libc.so and so always loaded at startup.  Hence we can use `[[gnu::tls_model("initial-exec")]]` here.


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:32-33
+
+// These are known parameters passed to the hwasan runtime on thread creation
+// when creating a thread.
+struct Thread::InitOptions {
----------------
department of redundancy department


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:34
+// when creating a thread.
+struct Thread::InitOptions {
+  uptr stack_bottom, stack_top;
----------------
Nit: I'd call it something more like InitState.



================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:41
+void InitThreads() {
+  uptr alloc_size = UINT64_C(1) << kShadowBaseAlignment;
+  uptr thread_start = reinterpret_cast<uptr>(
----------------
What depends on this alignment?  I'm not aware of anything that does in the compiler ABI we're using on Fuchsia.
If it's needed, it should have comments.



================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:84-88
+// are known before thread creation. However, we cannot setup the stack ring
+// buffer just yet because it is stored in the global __hwasan_tls, which we can
+// only correctly access once in the new thread. This will be set up in the
+// ThreadStartHook where we can safely reference __hwasan_tls while in the new
+// thread.
----------------
Actually we could do all of the allocation and setup except for storing the uptr in `__hwasan_tls`.
But that would take more refactoring of the common code and it's not clear there's a particular benefit.


================
Comment at: compiler-rt/lib/hwasan/hwasan_fuchsia.cpp:137
+// __hwasan_tls can be referenced.
+static void FinishThreadInitialization(Thread *thread) {
+  CHECK_NE(thread, nullptr);
----------------
Most of what's happening in this function is common with the Linux code.
I think a shared function can be factored out and put into hwasan_thread.cpp.



================
Comment at: compiler-rt/lib/hwasan/hwasan_thread.cpp:42-45
-  static u64 unique_id;
-  unique_id_ = unique_id++;
-  if (auto sz = flags()->heap_history_size)
-    heap_allocations_ = HeapAllocationsRingBuffer::New(sz);
----------------
This is stuff that could be done either in the before-create hook or in the on-start hook.  But it's probably not especially worthwhile to move it to before-create as long as we have on-start doing any allocation at all.  So to avoid a whole lot more refactoring to move the ringbuffer setup and such, might as well just leave this in on-start too.


================
Comment at: compiler-rt/lib/hwasan/hwasan_thread.cpp:58-63
-  uptr tls_size;
-  uptr stack_size;
-  GetThreadStackAndTls(IsMainThread(), &stack_bottom_, &stack_size, &tls_begin_,
-                       &tls_size);
-  stack_top_ = stack_bottom_ + stack_size;
-  tls_end_ = tls_begin_ + tls_size;
----------------
This is probably the only part that actually needs to be different between Linux and Fuchsia.
So this code could just move into an InitStackAndTls(options) that looks like this on Linux and on Fuchsia is just copying values out of options. 



================
Comment at: compiler-rt/lib/hwasan/hwasan_thread.h:85
   HeapAllocationsRingBuffer *heap_allocations_;
-  StackAllocationsRingBuffer *stack_allocations_;
+  StackAllocationsRingBuffer *stack_allocations_ = nullptr;
 
----------------
I think these need to be left uniformly uninitialized to guarantee they can be "linker-initialized".
At any rate, there's no reason one pointer member here should be initialized and the others not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104085