This is an archive of the discontinued LLVM Phabricator instance.

[ubsan] Implement __sanitizer_print_stack_trace for standalone UBSan runtime.
ClosedPublic

Authored by Dor1s on Apr 26 2017, 9:09 AM.

Details

Summary

Landed by @glider (thanks!) as git-svn-id: https://llvm.org/svn/llvm-project/compiler-rt/trunk@302218 91177308-0d34-0410-b5e6-96231b3b80d8

Diff Detail

Event Timeline

Dor1s created this revision.Apr 26 2017, 9:09 AM
glider accepted this revision.Apr 27 2017, 6:30 AM

LGTM

This revision is now accepted and ready to land.Apr 27 2017, 6:30 AM
glider edited the summary of this revision. (Show Details)Apr 27 2017, 7:08 AM
glider added a reviewer: eugenis.
Dor1s added a comment.Apr 27 2017, 7:36 AM

Thanks Alexander!

FTR, the reason why I'm adding this change: https://github.com/google/oss-fuzz/issues/552

I tested this change manually and it works as expected.

Regarding llvm tests, I tried check-ubsan and check-sanitizer, both of them are not executing print-stack-trace test for UBSan. However, check-sanitizer executes print-stack-trace test for other sanitizers, as Alexander mentioned.

I'll try to enable UBSan to llvm/projects/compiler-rt/test/sanitizer_common/lit.common.cfg, but don't believe that it'll work as is... I'll post an update soon.

Another options are:

  1. duplicate projects/compiler-rt/test/sanitizer_common/TestCases/print-stack-trace.cc into UBSan-specific tests
  2. commit this change as-is
Dor1s added a comment.Apr 27 2017, 7:58 AM

No luck with enabling sanitizer-common tests for UBSan Standalone so far.

Dor1s added a comment.Apr 27 2017, 8:02 AM

Adding list(APPEND SUPPORTED_TOOLS ubsan) into compiler-rt/test/sanitizer_common/CMakeLists.txt works, but lots of tests are failing as I expected :(

vsk added inline comments.Apr 27 2017, 2:56 PM
lib/ubsan/ubsan_diag_standalone.cc
28

I think this requests the slow unwinder, which isn't available on Darwin. Maybe it would be better to use the fast unwinder? E.g: https://reviews.llvm.org/D32517

eugenis edited edge metadata.Apr 27 2017, 2:59 PM

There should be a test for this. It would catch this potential Mac problem, too.

lib/ubsan/ubsan_diag_standalone.cc
26

This may break on Mac, where the slow unwinder is disabled. See the code in MaybePrintStackTrace() in ubsan_diag.cc.

Dor1s updated this revision to Diff 97073.Apr 28 2017, 3:31 AM

Thanks for taking a look ! I added fast unwind call. It's still unclear for me how to add a test properly. If I add a separate UBSan-only test for __sanitizer_print_stack_trace, it doesn't work correctly as every UBSan test is being invoked in 4 configurations:

  • Standalone
  • ASan
  • MSan
  • TSan

and configurations other than 'Standalone' are using their __sanitizer_print_stack_trace implementations, so the test fails.

If I add UBSan to invoke sanitizer_common test suite, it fails too much, as nobody expected those tests to be passed.

I'd appreciate if you can share any hints what to do :)

Dor1s marked 2 inline comments as done.Apr 28 2017, 3:32 AM
Dor1s updated this revision to Diff 97079.Apr 28 2017, 4:41 AM

Ok, looks like I did it. The added test passes with all the configurations.

eugenis added inline comments.May 1 2017, 2:39 PM
lib/ubsan/ubsan_diag_standalone.cc
28

See https://reviews.llvm.org/D32517, this code needs to get stack limits for the fast unwinder, and also it should respect fast_unwind_on_fatal as the other sanitizers do.

Basically, the same as D32517 but without the check for flags()->print_stacktrace.

Wait until that lands and reuse the code?

test/ubsan/TestCases/Common/print-stack-trace.cc
1 ↗(On Diff #97079)

test with the fast unwinder, too

14 ↗(On Diff #97079)

AFAIR we usually do it like this:
FooBarBaz{{.*}}print-stack-trace.cc

No space and no argument list because symbolization works slightly differently on some platforms.

Dor1s marked 3 inline comments as done.May 3 2017, 2:59 AM

Thanks Evgenii for the review!

lib/ubsan/ubsan_diag_standalone.cc
28

Thanks Evgenii, that's a great suggestion! Done.

Dor1s updated this revision to Diff 97576.May 3 2017, 3:00 AM
Dor1s marked an inline comment as done.

Addressed the comments, re-used new implementation from https://reviews.llvm.org/D32517.

Dor1s updated this revision to Diff 97607.May 3 2017, 5:03 AM

Almost done. Still cannot make the test happy on Mac, but now only TSan config is failing:

Failing Tests (2):
    UBSan-TSan-x86_64 :: TestCases/Common/print-stack-trace.cc
    UBSan-TSan-x86_64h :: TestCases/Common/print-stack-trace.cc


  Expected Passes    : 279
  Expected Failures  : 10
  Unsupported Tests  : 13
  Unexpected Failures: 2

Because:

$ <..>/work/llvm/projects/compiler-rt/test/ubsan/ThreadSanitizer-x86_64/TestCases/Common/Output/print-stack-trace.cc.tmp
    #0 __sanitizer_print_stack_trace tsan_rtl_report.cc:727 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x5cefb)

It uses TSan's implementation, not UBSan. I'll compare this behavior with Linux to find the root cause.

Dor1s updated this revision to Diff 97615.May 3 2017, 5:39 AM
Dor1s added a comment.May 3 2017, 5:53 AM

As per summary of https://reviews.llvm.org/D32517, the slow unwinder is not supported on Darwin, but __sanitizer_print_stack_trace in Darwin is using slow unwind only (https://github.com/llvm-mirror/compiler-rt/blob/master/lib/tsan/rtl/tsan_rtl_report.cc#L726). This is why my test is failing with TSan.

I'll play with TSan as well.

Dor1s updated this revision to Diff 97642.May 3 2017, 7:42 AM

This is final version for now, please take a look.

Regarding TSan, I'll upload a patch to https://reviews.llvm.org/D32806, but it needs more thoughts probably.

Huge thanks to Alexander @glider for helping me with debugging and other issues!

Dor1s added a comment.May 4 2017, 7:54 AM

All the comments have been addressed. Can we land this?

Dor1s closed this revision.May 5 2017, 2:59 AM
Dor1s edited the summary of this revision. (Show Details)