This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][hwasan] Setup hwasan thread handling on Fuchsia
ClosedPublic

Authored by leonardchan on Jun 10 2021, 6:27 PM.

Details

Summary

This patch splits up hwasan thread creation between __sanitizer_before_thread_create_hook, __sanitizer_thread_create_hook, and __sanitizer_thread_start_hook. The linux implementation creates the hwasan thread object inside the new thread. On Fuchsia, we know the stack bounds before thread creation, so we can initialize part of the thread object in __sanitizer_before_thread_create_hook, then initialize the stack ring buffer in __sanitizer_thread_start_hook once we enter the thread.

Diff Detail

Event Timeline

leonardchan created this revision.Jun 10 2021, 6:27 PM
leonardchan requested review of this revision.Jun 10 2021, 6:27 PM
Herald added a subscriber: Restricted Project. · View Herald TranscriptJun 10 2021, 6:27 PM

The non-fuchsia stuff can be split up in a separate patch when ready, but I'll keep it here for now until we're set on the thread handling.

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.

compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
27

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.

33–34

department of redundancy department

35

Nit: I'd call it something more like InitState.

42

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.

85–89

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.

138

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.

compiler-rt/lib/hwasan/hwasan_thread.cpp
43–46

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.

64–65

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.

compiler-rt/lib/hwasan/hwasan_thread.h
85 ↗(On Diff #351313)

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.

leonardchan marked 7 inline comments as done.
leonardchan edited the summary of this revision. (Show Details)

Rebased against recent refactoring commits and addressed some comments.

I think I can omit some functions like GetCurrentThread or GetRingBufferSize which are small enough to merit their own changes .

leonardchan planned changes to this revision.Jun 23 2021, 4:04 PM
leonardchan added inline comments.
compiler-rt/lib/hwasan/hwasan_fuchsia.cpp
42

Added.

138

Updated such that this just calls InitStackRingBuffer which handles the stack ring buffer initialization.

compiler-rt/lib/hwasan/hwasan_thread.cpp
43–46

With the refactoring from D104248, this bit will be done in the before-create hook without any changes to this code.

64–65

This is done in D104248.

Ok. Ready for reviews.

leonardchan marked 2 inline comments as done.Jun 29 2021, 12:08 PM

Let me know if this should be split up also. I think this might be small and and simple enough for one patch.

mcgrathr accepted this revision.Jul 5 2021, 2:14 PM

lgtm

This revision is now accepted and ready to land.Jul 5 2021, 2:14 PM
This revision was landed with ongoing or failed builds.Jul 7 2021, 3:09 PM
This revision was automatically updated to reflect the committed changes.