This is an archive of the discontinued LLVM Phabricator instance.

[lsan] On Fuchsia, don't use atexit hook for leak checks
ClosedPublic

Authored by mcgrathr on Aug 18 2020, 2:40 PM.

Details

Summary

Fuchsia's system libraries are instrumented and use the lsan
allocator for internal purposes. So leak checking needs to run
after all atexit hooks and after the system libraries' internal
exit-time hooks. The <zircon/sanitizer.h> hook API calls the
__sanitizer_process_exit_hook function at exactly the right time.

Diff Detail

Event Timeline

mcgrathr created this revision.Aug 18 2020, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2020, 2:40 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
mcgrathr requested review of this revision.Aug 18 2020, 2:40 PM
vitalybuka added inline comments.Aug 18 2020, 5:15 PM
compiler-rt/lib/asan/asan_rtl.cpp
502–503

can you please extract this into lsan.h/lsan_posix.cc as

AtExitCheckLeaks() {
   if (common_flags()->detect_leaks && common_flags()->leak_check_at_exit) {
      if (flags()->halt_on_error)
        Atexit(__lsan::DoLeakCheck);
      else
        Atexit(__lsan::DoRecoverableLeakCheckVoid);
    }
}

and make empty one in in lsan_fuchsia.cc

compiler-rt/lib/lsan/lsan_fuchsia.cpp
131

why do you need to move this?
if you move this please also move ExitHook to avoid it in common header

mcgrathr updated this revision to Diff 290618.Sep 8 2020, 5:22 PM
mcgrathr marked an inline comment as done.

refactored per review

compiler-rt/lib/asan/asan_rtl.cpp
502–503

Separate ones were needed for asan and lsan, so I did that.

compiler-rt/lib/lsan/lsan_fuchsia.cpp
131

ASan also calls __lsan::ExitHook on Fuchsia. The two definitions of the libc hook (__sanitizer_process_exit_hook) were different for ASan vs LSan because LSan doesn't have the halt_on_error flag. AFAICT that's still necessary because the expected behavior is not to change the exit status when halt_on_error=false. So though the mechanism is different from other platforms, I still need ASan-specific plumbing so it can test the ASan-specific flag, and separate standalone-LSan plumbing where there is no such flag to test.

I've changed this so lsan_common_fuchsia.cpp still defines this locally, but uses another __lsan:: function that is defined differently in asan_fuchsia.cpp (to check the flag) and lsan_fuchsia.cpp (to always change the exit code for leaks).

vitalybuka accepted this revision.Sep 16 2020, 1:30 PM
vitalybuka added inline comments.
compiler-rt/lib/asan/asan_posix.cpp
146–151

I guess lsan only version is enough
Just add Atexit(__lsan::DoRecoverableLeakCheckVoid) into that one.

If you do that can you add !halt_on_error test case to see if it works?

compiler-rt/lib/lsan/lsan.h
42

Maybe
AtExitCheckLeaks -> InstallAtExitLeakCheck

This revision is now accepted and ready to land.Sep 16 2020, 1:30 PM
mcgrathr updated this revision to Diff 292340.Sep 16 2020, 2:17 PM
mcgrathr marked an inline comment as done.

rebased

mcgrathr updated this revision to Diff 292684.Sep 17 2020, 5:53 PM

add windows stub

mcgrathr updated this revision to Diff 293609.Sep 22 2020, 6:03 PM

fix typo in windows stub

phosek accepted this revision.Sep 23 2020, 12:20 AM

Tested on Windows and it seems to be working now.

This revision was landed with ongoing or failed builds.Sep 23 2020, 11:11 AM
This revision was automatically updated to reflect the committed changes.
Arfrever added inline comments.
compiler-rt/lib/lsan/lsan_common_fuchsia.cpp
54

Typo: s/asan_fuchsiap.cpp/asan_fuchsia.cpp/

nikic added a subscriber: nikic.Sep 23 2020, 12:14 PM

I've reverted this change as it breaks the clang build with linker errors. D88173 did not address them either.

/home/nikic/llvm-project/compiler-rt/lib/asan/asan_rtl.cpp:503: error: undefined reference to '__asan::InstallAtExitCheckLeaks()'

This is with GCC 10.0, gold 1.16 and the following configuration:

cmake -GNinja -Bbuild llvm \
    -DLLVM_ENABLE_PROJECTS="clang;compiler-rt" \
    -DLLVM_TARGETS_TO_BUILD="all" \
    -DCMAKE_BUILD_TYPE=Release \
    -DLLVM_ENABLE_ASSERTIONS=true \
    -DLLVM_CCACHE_BUILD=true \
    -DLLVM_USE_LINKER=gold \
    -DLLVM_APPEND_VC_REV=false \
    -DCLANG_ENABLE_ARCMT=false \
    -DCLANG_ENABLE_STATIC_ANALYZER=false \
    -DLLVM_BINUTILS_INCDIR=/usr/include \
    -DLLVM_ENABLE_SPHINX=true \
    -DSPHINX_WARNINGS_AS_ERRORS=false