This is an archive of the discontinued LLVM Phabricator instance.

[lsan] Support LeakSanitizer runtime on Fuchsia
ClosedPublic

Authored by mcgrathr on Jan 16 2020, 3:25 PM.

Diff Detail

Event Timeline

mcgrathr created this revision.Jan 16 2020, 3:25 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 16 2020, 3:25 PM
Herald added subscribers: llvm-commits, Restricted Project, jfb and 2 others. · View Herald Transcript
vitalybuka added inline comments.
compiler-rt/lib/lsan/lsan.cpp
104

LsanInstalDeadlySignalHandlers()

*_fuchsic.cpp:
LsanInstalDeadlySignalHandlers() {}

*_linux.cpp:
LsanInstalDeadlySignalHandlers() {

InstallDeadlySignalHandlers(LsanOnDeadlySignal);

}

compiler-rt/lib/lsan/lsan_common.cpp
445–446

Can you please extract CheckForLeaksParam/Frontier into separate change and explain in description why this is needed

compiler-rt/lib/lsan/lsan_interceptors.cpp
66

this looks uncertainty
lsan_init_is_running is always false there?

409

thy this thread related stuff in interceptors.cpp
I think better to add lsan_thread_fuchsia.h
and put them here

also derive ThreadContextFuchsia : ThreadContext and avoid ifdefs in lsan_thread.cpp

mcgrathr updated this revision to Diff 238970.Jan 18 2020, 3:21 PM
mcgrathr marked 5 inline comments as done.

Generic refactoring patch split out.
Standalone LSan port refactored following review comments.

I've split part of the patch out and done a substantial refactoring of the standalone lsan thread stuff following Vitaly's review.

compiler-rt/lib/lsan/lsan.cpp
104

Since we already have a no-op InstallDeadlySignalHandlers in sanitizer_common, I just did the above with LsanOnDeadlySignal instead, which follows how ASan is organized.

compiler-rt/lib/lsan/lsan_interceptors.cpp
66

Yes. As the comment in the existing code explains, this hack is specifically because init code calls dlsym and that reenters calloc (in glibc). On Fuchsia, there are no dlsym calls nor anything else in init that risks any unexpected reentry of allocator APIs. I've added a comment here to this effect. (ASan's equivalent implementation is fancier with a UseLocalPool predicate that's always false on Fuchsia.)

409

I put this here because it's the exact analogue of the #else branch, which is in this file because on POSIX systems all the work is done via the pthread_create interceptor. On Fuchsia it's not literally an interceptor, but it seemed better to have the two implementations meant to do the equivalent thing right next to each other so someone changing one would realize the other might be affected. Basically, these two #ifdef branches are the sole clients of the common lsan_thread.cpp API so looking at them both together seems right.

However, with the more substantial refactoring to reduce #ifdef use in lsan_thread.cpp that I'll do now, the calculus changes and this logic won't be here since the refactored lsan_thread{,_fuchsia}.cpp won't have a need to provide that common API exactly as it is now (for Fuchsia).

vitalybuka added inline comments.Jan 21 2020, 6:44 PM
compiler-rt/lib/lsan/lsan_posix.cpp
18

Could you please move ThreadContext refactoring into a separate patch?

compiler-rt/lib/lsan/lsan_thread.cpp
26 ↗(On Diff #238970)

lets keep in static

74 ↗(On Diff #238970)

can you return ThreadCreate and ThreadStart to keep thread_registry static?
maybe as:
void ThreadStart(u32 tid, tid_t os_id, void *arg) {

mcgrathr updated this revision to Diff 240056.Jan 23 2020, 4:52 PM

Split out ThreadContext refactoring into a separate patch.
Updated related changes following review comments.

mcgrathr marked 4 inline comments as done.Jan 23 2020, 4:55 PM

I think I've done everything Vitaly's last review asked for. Thanks.

compiler-rt/lib/lsan/lsan_posix.cpp
18
vitalybuka accepted this revision.Jan 27 2020, 12:35 PM
vitalybuka added inline comments.
compiler-rt/lib/lsan/lsan_common.cpp
531

here and "ProcessThreads"
if this compile can you remove #ifdef and just add USED to supress warnings?

This revision is now accepted and ready to land.Jan 27 2020, 12:35 PM
mcgrathr marked 2 inline comments as done.Jan 27 2020, 1:35 PM
mcgrathr added inline comments.
compiler-rt/lib/lsan/lsan_common.cpp
531

I moved ReportIfNotSuspended out of the #if check and that compiles fine (we're not building with -Wunused-functions, so UNUSED is unnecessary--USED would be wrong here). It's not possible to reuse the ReportUnsuspendedThreads or ProcessThreads code on Fuchsia because SuspendedThreadsList doesn't implement any of the methods. It would be useless code on Fuchsia that wouldn't be compiled away too, since the compiler can't tell here that the thread list is always empty.

mcgrathr updated this revision to Diff 240667.Jan 27 2020, 1:36 PM

Moved ReportIfNotSuspended out of #if.

This revision was automatically updated to reflect the committed changes.
mcgrathr reopened this revision.Jan 28 2020, 12:41 AM

Reverted after landing due to patch rebasing snafu that dropped the LsanOnDeadlySignal function.

This revision is now accepted and ready to land.Jan 28 2020, 12:41 AM
mcgrathr updated this revision to Diff 240790.Jan 28 2020, 12:41 AM

Fixed patch rebasing snafu that dropped LsanOnDeadlySignal code from lsan_posix.cpp.

This revision was automatically updated to reflect the committed changes.
smeenai added inline comments.
compiler-rt/lib/lsan/lsan_common_fuchsia.cpp
129

Where is this defined? I don't see it in any of the Fuchsia SDKs or sysroots.