This is an archive of the discontinued LLVM Phabricator instance.

Differential D32806

[tsan] Implement fast unwinding in __sanitizer_print_stack_trace + cleanup.
Needs ReviewPublic

Authored by Dor1s on May 3 2017, 7:22 AM.

Details

Reviewers
eugenis
dvyukov
glider
Summary

Please take a look.

Diff Detail

Event Timeline

Dor1s created this revision.May 3 2017, 7:22 AM
Herald added a subscriber: kubamracek. · View Herald TranscriptMay 3 2017, 7:22 AM
Dor1s retitled this revision from [tsan] Implement fast unwinding in __sanitizer_print_stack_trace. to [tsan] Implement fast unwinding in __sanitizer_print_stack_trace + cleanup..May 3 2017, 9:05 AM
Dor1s updated this revision to Diff 97661.May 3 2017, 9:12 AM
Dor1s updated this revision to Diff 97666.May 3 2017, 9:20 AM
Dor1s updated this revision to Diff 97669.May 3 2017, 9:33 AM
Dor1s edited the summary of this revision. (Show Details)May 3 2017, 9:43 AM
Dor1s added reviewers: eugenis, dvyukov, glider.
Dor1s added inline comments.
test/tsan/print_stack_trace.cc
4 ↗(On Diff #97669)

Self-nit: no need to recompile with the same options.

Dor1s added inline comments.May 3 2017, 10:13 AM
lib/sanitizer_common/sanitizer_stacktrace.h
33 ↗(On Diff #97669)

It looks like I got confused and removing this SANITIZER_CAN_SLOW_UNWIND stuff is wrong. Though maybe I can implement it as well?

eugenis added inline comments.May 3 2017, 10:29 AM
lib/sanitizer_common/sanitizer_stacktrace.h
33 ↗(On Diff #97669)

Yeah, sorry I've missed this. I assume this is here for a reason.

lib/tsan/rtl/tsan_rtl_report.cc
698

why??

700

just use std::swap()

Dor1s updated this revision to Diff 97687.May 3 2017, 10:46 AM

This version doesn't break anything.

Dor1s updated this revision to Diff 97706.May 3 2017, 12:26 PM
Dor1s marked 2 inline comments as done.
Dor1s marked an inline comment as done.
Dor1s added inline comments.
lib/tsan/rtl/tsan_rtl_report.cc
698

Good catch! My previous patchset used this function twice, but now there is not need to have that. Removed.

700

Looks like this code does not use STL intentionally. If I add it, the following error happens, for example:

In file included from <...>/llvm/src/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cc:29:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/algorithm:628:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:603:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/new:158:40: error: redefinition of 'operator new'
inline _LIBCPP_INLINE_VISIBILITY void* operator new  (std::size_t, void* __p) _NOEXCEPT {return __p;}
                                       ^
Dor1s marked 2 inline comments as done.May 5 2017, 7:18 AM

Could you please take a look or suggest other reviewers?

eugenis added inline comments.May 8 2017, 1:13 PM
lib/tsan/rtl/tsan_rtl_report.cc
731

Why do you need this case at all? PrintStackTrace call below should take care of it. See the implementation of the same function in ubsan.

737

ThreadState::stk_addr
ThreadState::stk_size

Revision Contents

PathSize
lib/
tsan/
rtl/
1 line

Diff 97640

lib/tsan/rtl/tsan_rtl_report.cc

Show First 20 Lines • Show All 689 Lines • ▼ Show 20 Lines
void PrintCurrentStack(ThreadState *thr, uptr pc) { void PrintCurrentStack(ThreadState *thr, uptr pc) {
VarSizeStackTrace trace; VarSizeStackTrace trace;
ObtainCurrentStack(thr, pc, &trace); ObtainCurrentStack(thr, pc, &trace);
PrintStack(SymbolizeStack(trace)); PrintStack(SymbolizeStack(trace));
} }
// Always inlining PrintCurrentStackSlow, because LocatePcInTrace assumes // Always inlining PrintCurrentStackSlow, because LocatePcInTrace assumes
// __sanitizer_print_stack_trace exists in the actual unwinded stack, but // __sanitizer_print_stack_trace exists in the actual unwinded stack, but
eugenisUnsubmitted
Done
ReplyInline Actions

why??

eugenis: why??
Dor1sAuthorUnsubmitted
Done
ReplyInline Actions

Good catch! My previous patchset used this function twice, but now there is not need to have that. Removed.

Dor1s: Good catch! My previous patchset used this function twice, but now there is not need to have…
// tail-call to PrintCurrentStackSlow breaks this assumption because // tail-call to PrintCurrentStackSlow breaks this assumption because
// __sanitizer_print_stack_trace disappears after tail-call. // __sanitizer_print_stack_trace disappears after tail-call.
eugenisUnsubmitted
Done
ReplyInline Actions

just use std::swap()

eugenis: just use std::swap()
Dor1sAuthorUnsubmitted
Done
ReplyInline Actions

Looks like this code does not use STL intentionally. If I add it, the following error happens, for example:

In file included from <...>/llvm/src/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cc:29:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/algorithm:628:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:603:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/new:158:40: error: redefinition of 'operator new'
inline _LIBCPP_INLINE_VISIBILITY void* operator new  (std::size_t, void* __p) _NOEXCEPT {return __p;}
                                       ^
Dor1s: Looks like this code does not use STL intentionally. If I add it, the following error happens…
// However, this solution is not reliable enough, please see dvyukov's comment // However, this solution is not reliable enough, please see dvyukov's comment
// http://reviews.llvm.org/D19148#406208 // http://reviews.llvm.org/D19148#406208
// Also see PR27280 comment 2 and 3 for breaking examples and analysis. // Also see PR27280 comment 2 and 3 for breaking examples and analysis.
ALWAYS_INLINE ALWAYS_INLINE
void PrintCurrentStackSlow(uptr pc) { void PrintCurrentStackSlow(uptr pc) {
#if !SANITIZER_GO #if !SANITIZER_GO
BufferedStackTrace *ptrace = BufferedStackTrace *ptrace =
new(internal_alloc(MBlockStackTrace, sizeof(BufferedStackTrace))) new(internal_alloc(MBlockStackTrace, sizeof(BufferedStackTrace)))
Show All 11 Lines
} // namespace __tsan } // namespace __tsan
using namespace __tsan; using namespace __tsan;
extern "C" { extern "C" {
SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_INTERFACE_ATTRIBUTE
void __sanitizer_print_stack_trace() { void __sanitizer_print_stack_trace() {
PrintCurrentStackSlow(StackTrace::GetCurrentPc()); PrintCurrentStackSlow(StackTrace::GetCurrentPc());
} }
} // extern "C" } // extern "C"
eugenisUnsubmitted
Not Done
ReplyInline Actions

Why do you need this case at all? PrintStackTrace call below should take care of it. See the implementation of the same function in ubsan.

eugenis: Why do you need this case at all? PrintStackTrace call below should take care of it. See the…
eugenisUnsubmitted
Not Done
ReplyInline Actions

ThreadState::stk_addr
ThreadState::stk_size

eugenis: ThreadState::stk_addr ThreadState::stk_size