From 557855e047aea53023165756586697c0f1cbc3ce Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Thu, 15 Jul 2021 19:29:04 -0400 Subject: [PATCH] Revert "tsan: make obtaining current PC faster" This reverts commit e33446ea58b8357dd8b79eb39140a1de2baff1ae. Doesn't build on mac, and causes other problems. See reports on https://reviews.llvm.org/D106046 and https://reviews.llvm.org/D106081 Also revert follow-up "tsan: strip top inlined internal frames" This reverts commit 7b302fc9b04c7991cdb869b65316e0d72e41042e. --- .../lib/sanitizer_common/sanitizer_stacktrace.cpp | 32 ++++++++++++++++++ .../lib/sanitizer_common/sanitizer_stacktrace.h | 38 ---------------------- compiler-rt/lib/tsan/rtl/tsan_interceptors.h | 16 ++++----- .../lib/tsan/rtl/tsan_interceptors_posix.cpp | 6 ++-- compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp | 29 ++++------------- 5 files changed, 49 insertions(+), 72 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp index 49672eb..07e4409 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp @@ -19,6 +19,38 @@ namespace __sanitizer { +uptr StackTrace::GetNextInstructionPc(uptr pc) { +#if defined(__sparc__) || defined(__mips__) + return pc + 8; +#elif defined(__powerpc__) || defined(__arm__) || defined(__aarch64__) + return pc + 4; +#elif SANITIZER_RISCV64 + // Current check order is 4 -> 2 -> 6 -> 8 + u8 InsnByte = *(u8 *)(pc); + if (((InsnByte & 0x3) == 0x3) && ((InsnByte & 0x1c) != 0x1c)) { + // xxxxxxxxxxxbbb11 | 32 bit | bbb != 111 + return pc + 4; + } + if ((InsnByte & 0x3) != 0x3) { + // xxxxxxxxxxxxxxaa | 16 bit | aa != 11 + return pc + 2; + } + // RISC-V encoding allows instructions to be up to 8 bytes long + if ((InsnByte & 0x3f) == 0x1f) { + // xxxxxxxxxx011111 | 48 bit | + return pc + 6; + } + if ((InsnByte & 0x7f) == 0x3f) { + // xxxxxxxxx0111111 | 64 bit | + return pc + 8; + } + // bail-out if could not figure out the instruction size + return 0; +#else + return pc + 1; +#endif +} + uptr StackTrace::GetCurrentPc() { return GET_CALLER_PC(); } diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h index 03137f8..6b2a687 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h +++ b/compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h @@ -106,39 +106,6 @@ uptr StackTrace::GetPreviousInstructionPc(uptr pc) { #endif } -ALWAYS_INLINE -uptr StackTrace::GetNextInstructionPc(uptr pc) { -#if defined(__sparc__) || defined(__mips__) - return pc + 8; -#elif defined(__powerpc__) || defined(__arm__) || defined(__aarch64__) - return pc + 4; -#elif SANITIZER_RISCV64 - // Current check order is 4 -> 2 -> 6 -> 8 - u8 InsnByte = *(u8 *)(pc); - if (((InsnByte & 0x3) == 0x3) && ((InsnByte & 0x1c) != 0x1c)) { - // xxxxxxxxxxxbbb11 | 32 bit | bbb != 111 - return pc + 4; - } - if ((InsnByte & 0x3) != 0x3) { - // xxxxxxxxxxxxxxaa | 16 bit | aa != 11 - return pc + 2; - } - // RISC-V encoding allows instructions to be up to 8 bytes long - if ((InsnByte & 0x3f) == 0x1f) { - // xxxxxxxxxx011111 | 48 bit | - return pc + 6; - } - if ((InsnByte & 0x7f) == 0x3f) { - // xxxxxxxxx0111111 | 64 bit | - return pc + 8; - } - // bail-out if could not figure out the instruction size - return 0; -#else - return pc + 1; -#endif -} - // StackTrace that owns the buffer used to store the addresses. struct BufferedStackTrace : public StackTrace { uptr trace_buffer[kStackTraceMax]; @@ -229,10 +196,5 @@ static inline bool IsValidFrame(uptr frame, uptr stack_top, uptr stack_bottom) { uptr local_stack; \ uptr sp = (uptr)&local_stack -#define GET_CURRENT_PC() \ - ({ \ - this_pc: \ - StackTrace::GetNextInstructionPc((uptr) && this_pc); \ - }) #endif // SANITIZER_STACKTRACE_H diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors.h b/compiler-rt/lib/tsan/rtl/tsan_interceptors.h index c5716f5..29576ea 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors.h +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors.h @@ -30,14 +30,14 @@ inline bool in_symbolizer() { } // namespace __tsan -#define SCOPED_INTERCEPTOR_RAW(func, ...) \ - cur_thread_init(); \ - ThreadState *thr = cur_thread(); \ - const uptr caller_pc = GET_CALLER_PC(); \ - ScopedInterceptor si(thr, #func, caller_pc); \ - const uptr pc = GET_CURRENT_PC(); \ - (void)pc; \ - /**/ +#define SCOPED_INTERCEPTOR_RAW(func, ...) \ + cur_thread_init(); \ + ThreadState *thr = cur_thread(); \ + const uptr caller_pc = GET_CALLER_PC(); \ + ScopedInterceptor si(thr, #func, caller_pc); \ + const uptr pc = StackTrace::GetCurrentPc(); \ + (void)pc; \ +/**/ #define SCOPED_TSAN_INTERCEPTOR(func, ...) \ SCOPED_INTERCEPTOR_RAW(func, __VA_ARGS__); \ diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp index 86accfe..6808f2e 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp @@ -2157,7 +2157,7 @@ void atfork_prepare() { if (in_symbolizer()) return; ThreadState *thr = cur_thread(); - const uptr pc = GET_CURRENT_PC(); + const uptr pc = StackTrace::GetCurrentPc(); ForkBefore(thr, pc); } @@ -2165,7 +2165,7 @@ void atfork_parent() { if (in_symbolizer()) return; ThreadState *thr = cur_thread(); - const uptr pc = GET_CURRENT_PC(); + const uptr pc = StackTrace::GetCurrentPc(); ForkParentAfter(thr, pc); } @@ -2173,7 +2173,7 @@ void atfork_child() { if (in_symbolizer()) return; ThreadState *thr = cur_thread(); - const uptr pc = GET_CURRENT_PC(); + const uptr pc = StackTrace::GetCurrentPc(); ForkChildAfter(thr, pc); FdOnFork(thr, pc); } diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp index 6c2d9a7..706794f 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp @@ -47,24 +47,7 @@ void __tsan_on_report(const ReportDesc *rep) { (void)rep; } -bool InternalFrame(const char *func) { - static const char *frames[] = { - "__tsan::ScopedInterceptor", - "__sanitizer::StackTrace", - }; - for (auto frame : frames) { - if (!internal_strncmp(func, frame, internal_strlen(frame))) - return true; - } - return false; -} - -static SymbolizedStack *StackStripMain(SymbolizedStack *frames) { - for (; frames && frames->info.function; frames = frames->next) { - // Remove top inlined frames from our interceptors. - if (!InternalFrame(frames->info.function)) - break; - } +static void StackStripMain(SymbolizedStack *frames) { SymbolizedStack *last_frame = nullptr; SymbolizedStack *last_frame2 = nullptr; for (SymbolizedStack *cur = frames; cur; cur = cur->next) { @@ -73,7 +56,7 @@ static SymbolizedStack *StackStripMain(SymbolizedStack *frames) { } if (last_frame2 == 0) - return frames; + return; #if !SANITIZER_GO const char *last = last_frame->info.function; const char *last2 = last_frame2->info.function; @@ -86,8 +69,7 @@ static SymbolizedStack *StackStripMain(SymbolizedStack *frames) { last_frame->ClearAll(); last_frame2->next = nullptr; // Strip global ctors init. - } else if (last && (0 == internal_strcmp(last, "__do_global_ctors_aux") || - 0 == internal_strcmp(last, "__libc_csu_init"))) { + } else if (last && 0 == internal_strcmp(last, "__do_global_ctors_aux")) { last_frame->ClearAll(); last_frame2->next = nullptr; // If both are 0, then we probably just failed to symbolize. @@ -103,7 +85,6 @@ static SymbolizedStack *StackStripMain(SymbolizedStack *frames) { last_frame->ClearAll(); last_frame2->next = nullptr; #endif - return frames; } ReportStack *SymbolizeStackId(u32 stack_id) { @@ -137,8 +118,10 @@ static ReportStack *SymbolizeStack(StackTrace trace) { last->next = top; top = ent; } + StackStripMain(top); + ReportStack *stack = ReportStack::New(); - stack->frames = StackStripMain(top); + stack->frames = top; return stack; } -- 2.7.4