This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][hwasan] Refactor Thread::Init
ClosedPublic

Authored by leonardchan on Jun 14 2021, 11:18 AM.

Details

Summary

This allows for other implementations to define their own version of Thread::Init. This will be the case for Fuchsia where much of the thread initialization can be broken up between different thread hooks (__sanitizer_before_thread_create_hook, __sanitizer_thread_create_hook, __sanitizer_thread_start_hook). Namely, setting up the heap ring buffer and stack info and can be setup before thread creation. The stack ring buffer can also be setup before thread creation, but storing it into __hwasan_tls can only be done on the thread start hook since it's only then we can access __hwasan_tls for that thread correctly.

Diff Detail

Event Timeline

leonardchan created this revision.Jun 14 2021, 11:18 AM
leonardchan requested review of this revision.Jun 14 2021, 11:18 AM
Herald added a subscriber: Restricted Project. · View Herald TranscriptJun 14 2021, 11:18 AM
mcgrathr added inline comments.Jun 14 2021, 11:28 AM
compiler-rt/lib/hwasan/hwasan_linux.cpp
430

Most of this code can actually be reused for Fuchsia (just not necessarily in Thread::Init).
It's probably better to split it up for reuse rather than just moving the whole thing to linux-specific.

leonardchan updated this revision to Diff 351957.
leonardchan added inline comments.Jun 14 2021, 12:03 PM
compiler-rt/lib/hwasan/hwasan_linux.cpp
430

So maybe something like the current patch? Then on Fuchsia we could (1) have a custom implementation of InitStackAndTls, (2) wrap the call to Thread::InitStackRingBuffer in Thread::Init with a #if SANITIZER_FUCHSIA so it's not called before thread creation, then (2) call Thread::InitStackRingBuffer in the __sanitizer_thread_start_hook hook.

leonardchan retitled this revision from [compiler-rt][hwasan] Move Thread::Init into hwasan_linux.cpp to [compiler-rt][hwasan] Refactor Thread::Init.Jun 14 2021, 12:04 PM
leonardchan marked an inline comment as not done.
leonardchan added inline comments.
compiler-rt/lib/hwasan/hwasan_linux.cpp
430

Sorry. Meant wrapping with a #if !SANITIZER_FUCHSIA.

leonardchan marked an inline comment as not done.
vitalybuka added inline comments.Jun 15 2021, 3:21 PM
compiler-rt/lib/hwasan/hwasan_linux.cpp
430

in other sanitizers only GetThreadStackAndTls is platform specific

compiler-rt/lib/hwasan/hwasan_thread.cpp
51

clang-format: please reformat the code

please fix

mcgrathr added inline comments.Jun 15 2021, 3:57 PM
compiler-rt/lib/hwasan/hwasan_linux.cpp
430

in other sanitizers only GetThreadStackAndTls is platform specific

That's not entirely accurate. AsanThread::SetThreadStackAndTls might be a model here. But asan on Fuchsia also splits up the initialization into pre-creation and on-created-thread portions (AsanThread::Init is called before thread creation, and AsanThreadRegistry::StartThread is called on the created thread).

leonardchan marked an inline comment as done.

Rebased and formatted.

vitalybuka accepted this revision.Jun 16 2021, 12:05 PM
vitalybuka added a reviewer: mcgrathr.
vitalybuka added inline comments.
compiler-rt/lib/hwasan/hwasan_linux.cpp
430

Patch itself is LGTM.
But I am not sure if @mcgrathr is asking for particular changes.

Will submit around end of day if there's no more comments.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 18 2021, 10:36 AM
This revision was automatically updated to reflect the committed changes.