This is an archive of the discontinued LLVM Phabricator instance.

[asan] Complete the Fuchsia port
ClosedPublic

Authored by mcgrathr on Jul 25 2017, 4:05 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mcgrathr created this revision.Jul 25 2017, 4:05 PM
vitalybuka added a subscriber: vitalybuka.
vitalybuka added inline comments.
lib/asan/asan_interceptors.h
22 ↗(On Diff #108182)

I assume you disable entire things instead of particular interceptors, like other platforms, because Fuchsia does not need interceptors?

Could you please note that in the code.

lib/asan/asan_rtl.cc
89 ↗(On Diff #108182)

Why not just SANITIZER_FUCHSIA

342 ↗(On Diff #108182)

Maybe better to move some stuff into
asan_rtl_no_fuchsia.cc (as separate CL)
and put new stuff into asan_rtl_fuchsia.cc?

Same for other files

lib/asan/asan_thread.cc
234 ↗(On Diff #108182)

Could you please move this into fuchsia version of AsanThread::SetThreadStackAndTls()

filcab added a subscriber: filcab.Jul 28 2017, 6:10 AM
filcab added inline comments.
lib/asan/asan_interceptors.h
118 ↗(On Diff #108182)

It seems you can disable most of the file anyway. Do you really need InitializeAsanInterceptors if you don't have interceptors?
Can you just hoist the macro (and maybe the functions) to either the top of the header or another header?

mcgrathr marked an inline comment as done.Jul 28 2017, 4:48 PM
mcgrathr added inline comments.
lib/asan/asan_rtl.cc
342 ↗(On Diff #108182)

Done in D36037

mcgrathr updated this revision to Diff 108745.Jul 28 2017, 4:49 PM
mcgrathr marked an inline comment as done.

Rebased after D36037 split out

mcgrathr marked an inline comment as done.Aug 1 2017, 1:55 PM

I'll split this into smaller pieces.

lib/asan/asan_interceptors.h
118 ↗(On Diff #108182)

InitializeAsanInterceptors is called unconditionally from asan_rtl.cc and the norm in this code seems to be to write stub functions instead of conditionalizing the calls. The main thing I need is the ENSURE_ASAN_INITED macro, which is used by asan_interceptors_memintrinsics.h and by asan_malloc_linux.cc (both of which are used on Fuchsia).

I'll move the preserved code to the top of the file if that's what you prefer. If it's to go in a different header, I'm not sure which one you'd want it in. I'll do it however I'm told.

mcgrathr added inline comments.Aug 1 2017, 2:29 PM
lib/asan/asan_thread.cc
234 ↗(On Diff #108182)

Currently I don't have such a function. It would have to have a different signature than the generic one so it can take the arguments passed to Init.
Or I could make the generic one take optional arguments as I did for Init and always pass them down here.
Is that what you meant?

mcgrathr updated this revision to Diff 109230.Aug 1 2017, 2:54 PM

Rebased after splitting out D36189 and D36190.
Moved some work from AsanThread::Init to AsanThread::SetThreadStackAndTls.

vitalybuka added inline comments.Aug 3 2017, 6:53 PM
lib/asan/asan_errors.cc
74 ↗(On Diff #109586)

Why you can't leave the current implementation?

lib/asan/asan_rtl.cc
370 ↗(On Diff #109586)

Please move into CL with thread API refactoring as well

lib/asan/asan_shadow_setup.cc
17 ↗(On Diff #109586)

Can I ask you to try later to move such full file "#if !SANITIZER_FUCHSIA ..." into cmake rules?

lib/asan/asan_thread.cc
273 ↗(On Diff #109586)

Please move API change and other thread code movements, without fucshia, into separate patch

281 ↗(On Diff #109586)

Could you please put fuchsia version
void AsanThread::SetThreadStackAndTls() into asan_fuchsia

mcgrathr marked 3 inline comments as done.Aug 6 2017, 5:43 PM
mcgrathr added inline comments.
lib/asan/asan_errors.cc
74 ↗(On Diff #109586)

It's dead code already because Fuchsia doesn't have anything like signal handling (it would take a bunch of changes in generic code to get rid of the references to it in other dead code paths). Defining it would require me to define more functions of dead code just to satisfy its references (sanitizer::DescribeSignalOrException, sanitizer::SignalContext::DumpAllRegisters). But there is plenty of dead code cruft already and plenty of stub functions I've defined just because of all the dead code already. And reducing #if complexity is good. So I'll do that.

lib/asan/asan_rtl.cc
370 ↗(On Diff #109586)

Done in D36385.

lib/asan/asan_shadow_setup.cc
17 ↗(On Diff #109586)

That would be inconsistent with the rest of the code base.
I don't have an opinion on how this is done, but it should be consistent.
If you also want to change all the *_win, *_mac, *_posix etc. files to a new style where they don't have #if around their whole contents are are excluded at the cmake level instead, that's fine with me and I'm happy to help make the Fuchsia-specific code consistent with the rest of the code base.

lib/asan/asan_thread.cc
273 ↗(On Diff #109586)

Done in D36385.

mcgrathr updated this revision to Diff 109947.Aug 6 2017, 5:46 PM
mcgrathr marked 2 inline comments as done.
mcgrathr retitled this revision from [asan] Fuchsia port to [asan] Complete the Fuchsia port.

Split out D36385. Reduced number of #if's. Moved Fuchsia-specific implementations into asan_fuchsia.cc.

vitalybuka accepted this revision.Aug 8 2017, 1:26 PM
This revision is now accepted and ready to land.Aug 8 2017, 1:26 PM

BTW. You can try to apply for committer access.

mcgrathr updated this revision to Diff 110308.Aug 8 2017, 5:07 PM

Rebased and ready to land once D36385 has landed.
Please land it for me!

Thanks,
Roland

This revision was automatically updated to reflect the committed changes.