tsan: make obtaining current PC faster

[dvyukov]

Summary

We obtain the current PC is all interceptors and collectively
common interceptor code contributes to overall slowdown
(in particular cheaper str/mem* functions).

The current way to obtain the current PC involves:

4493e1:       e8 3a f3 fe ff          callq  438720 <_ZN11__sanitizer10StackTrace12GetCurrentPcEv>
4493e9:       48 89 c6                mov    %rax,%rsi

and the called function is:

uptr StackTrace::GetCurrentPc() {

438720:       48 8b 04 24             mov    (%rsp),%rax
438724:       c3                      retq

The new way uses address of a local label and involves just:

44a888:       48 8d 35 fa ff ff ff    lea    -0x6(%rip),%rsi

I am not switching all uses of StackTrace::GetCurrentPc to GET_CURRENT_PC
because it may lead some differences in produced reports and break tests.
The difference comes from the fact that currently we have PC pointing
to the CALL instruction, but the new way does not yield any code on its own
so the PC points to a random instruction in the function and symbolizing
that instruction can produce additional inlined frames (if the random
instruction happen to relate to some inlined function).

[melver]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

199

Previously the PC was obtained by a call and we sort of were guaranteed that the compiler wouldn't move code around.

Should this now include compiler barriers before/after?

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

199

I am not sure what a compiler barrier will give us here. It can pessimize codegen and the PC still points to a random instruction. So I would wait until we have a reason to add the barrier.
And in the end compiler can move code around calls as well.

[thakis]

This seems to break building on arm macs: http://45.33.8.238/macm1/14037/step_4.txt

Please take a look.

[dvyukov]

Looking. Thanks for the report.

[thakis]

Looks like it's Mac builds on general, my Intel Mac is just slow to cycle: http://45.33.8.238/mac/33533/step_4.txt

[dvyukov]

I've mailed https://reviews.llvm.org/D106078 that fixes it.

[dvyukov]

FTR another fix for issues reported by bots:
https://reviews.llvm.org/D106081

https://lab.llvm.org/buildbot#builders/94/builds/4914
https://lab.llvm.org/buildbot#builders/100/builds/7355

[thakis]

The tree's been broken for over 6h by now. Can we revert for now? (Or, ideally, like 5h ago?)

[thakis]

Reverted this (and follow-up https://reviews.llvm.org/D106081) in 557855e047aea53023165756586697c0f1cbc3ce.

[dvyukov]

Rework the change after numerious bot breakages and reverts.
As some bot breakages shown new inlined frames can also affect
stack printing in sanitizer_common (for some malloc-related reports),
so removing the inlined frames only in tsan does not work.
Also relying on hardcoded function names is fragile.
Additionally with reduced debug info compiler may strip namespaces
and class names from debug info, so that e.g.
"__tsan::ScopedInterceptor::Foo" becomes just "Foo". Stripping such
frames is especially problematic and can collide with user frames.
I've also tried a different approach: for some stacks throw away
inlined functions for the top frame. This does not rely on matching
function names. It almost worked except that for tsan racing stacks
we don't really know if top PC is a user PC (need inline frames)
or it comes from an str/mem interceptor (don't need inline frames).
The current version gives up on introducing no code and adds a NOP,
which becomes the carrier of the proper debug info. This makes
everything much simpler and hopefully more reliable. But unfortunately
it makes the code arch-dependent, so for now it's x86-only.

Benchmarking shows it's still ~4x faster:

#include <stdio.h>

attribute((noinline)) unsigned long get_current_pc1() {
return (unsigned long)__builtin_return_address(0);
}

#define get_current_pc2() ({ here: asm("nop"); ((unsigned long) && here) + 1; })

int main(int argc, char** argv) {
const unsigned long iters = 1e10;
volatile unsigned long pc = 0;
if (argc == 1) {

		for (unsigned long i = 0; i < iters; i++)
			pc = get_current_pc1();

} else {

		for (unsigned long i = 0; i < iters; i++)
			pc = get_current_pc2();

}
}

pc1: 10.38
pc2: 2.58
pc1: 10.87
pc2: 2.56
pc1: 10.99
pc2: 2.59

[dvyukov]

PTAL

[pcc]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

209

Maybe since this has to be arch-dependent anyway, instead of the NOP, you could put the LEA instruction here?

[dvyukov]

Switch to using LEA RIP directly.

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

209

Right!
If we are x86-only and use asm anyway, that's reasonable.
This removes nop byte and avoids pedantic warnings re taking address of a label.
Unfortunately RIP is not supported in MOV instructions (?) (we need 0 offset anyway), so we still inject 4 byte offset.
Also, an asm block can be inlined now, so we could use this to simply speed up StackTrace::GetCurrentPc() for everybody. But StackTrace::GetCurrentPc() would appear on top of most stacks, and again unfortunately attribute artificial does not do what it claims to do. Well, maybe it marks the code with some fancy artificial attribute in later versions of DWARF, but it does not help, I still see it in our stacks. Maybe we could improve llvm-symbolizer to ignore artificial functions, we there does not seem to be a way to do this with addr2line symbolizer.
There is also attribute((no_debug)) (which I learnt about from D43259), but it's clang-only. I am also not sure if we it will work reliably, or it's merely a coincidence that it works. If we simply leave an instruction w/o debug info, I think some DWARF users will use line info from the previous instructions, which is not necessary the caller function, but may be some other inlined function in the caller.
So for now I stayed with a macro.

And now the test started failing because 2 subsequent GET_CURRENT_PC() invocations started to yield the same PC. Compiler figured out that asm blocks can be merged. I guess we could avoid that with volatile asm, but I think it's fine, we don't need specifically different PCs.

PTAL

[melver]

Given this is x86-only now, hopefully there won't be any new surprises.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

#define GET_CURRENT_PC_BP \
   uptr bp = GET_CURRENT_FRAME();              \
   uptr pc = StackTrace::GetCurrentPc()
 
 #define GET_CURRENT_PC_BP_SP \
   GET_CURRENT_PC_BP;                          \
   uptr local_stack;                           \
   uptr sp = (uptr)&local_stack
 
+// GET_CURRENT_PC() is equivalent to StackTrace::GetCurrentPc().
+// Optimized x86 version is faster than GetCurrentPc because
+// it does not involve a function call, instead it reads RIP register.
+// Reads of RIP by an instruction return RIP pointing to the next
+// instruction, which is exactly what we want here, thus 0 offset.
+// It needs to be a macro because otherwise we will get the name
+// of this function on the top of most stacks. Attribute artificial
+// does not do what it claims to do, unfortunatley. And attribute
+// __nodebug__ is clang-only. If we would have an attribute that
+// would remove this function from debug info, we could simply make
+// StackTrace::GetCurrentPc() faster.
+#if defined(__x86_64__)
+#  define GET_CURRENT_PC()                \
+    ({                                    \
+      uptr pc;                            \
+      asm("lea 0(%%rip), %0" : "=r"(pc)); \
+      pc;                                 \
+    })
+#else
+#  define GET_CURRENT_PC() StackTrace::GetCurrentPc()
+#endif
 
 #endif  // SANITIZER_STACKTRACE_H

compiler-rt/lib/sanitizer_common/tests/sanitizer_stacktrace_test.cpp

TEST(SlowUnwindTest, ShortStackTrace) {
   EXPECT_EQ(0U, stack.size);
   EXPECT_EQ(0U, stack.top_frame_bp);
   stack.Unwind(pc, bp, nullptr, false, /*max_depth=*/1);
   EXPECT_EQ(1U, stack.size);
   EXPECT_EQ(pc, stack.trace[0]);
   EXPECT_EQ(bp, stack.top_frame_bp);
 }
 
+TEST(GetCurrentPc, Basic) {
+  // Test that PCs obtained via GET_CURRENT_PC()
+  // and StackTrace::GetCurrentPc() are all different
+  // and are close to the function start.
+  struct Local {
+    static NOINLINE void Test() {
+      const uptr pcs[] = {
+          (uptr)&Local::Test,
+          GET_CURRENT_PC(),
+          StackTrace::GetCurrentPc(),
+          StackTrace::GetCurrentPc(),
+      };
+      for (uptr i = 0; i < ARRAY_SIZE(pcs); i++)
+        Printf("pc%zu: %p\n", i, pcs[i]);
+      for (uptr i = 1; i < ARRAY_SIZE(pcs); i++) {
+        EXPECT_GT(pcs[i], pcs[0]);
+        EXPECT_LT(pcs[i], pcs[0] + 1000);
+        for (uptr j = 0; j < i; j++) EXPECT_NE(pcs[i], pcs[j]);
+      }
+    }
+  };
+  Local::Test();
+}
+
 // Dummy implementation. This should never be called, but is required to link
 // non-optimized builds of this test.
 void BufferedStackTrace::UnwindImpl(uptr pc, uptr bp, void *context,
                                     bool request_fast, u32 max_depth) {
   UNIMPLEMENTED();
 }
 
 }  // namespace __sanitizer

compiler-rt/lib/tsan/rtl/tsan_interceptors.h

 inline bool in_symbolizer() {
   cur_thread_init();
   return UNLIKELY(cur_thread()->in_symbolizer);
 }
 #endif
 
 }  // 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 = StackTrace::GetCurrentPc(); \
-    (void)pc; \
+#define SCOPED_INTERCEPTOR_RAW(func, ...)      \
+  cur_thread_init();                           \
+  ThreadState *thr = cur_thread();             \
+  const uptr caller_pc = GET_CALLER_PC();      \
+  const uptr pc = GET_CURRENT_PC();            \
+  (void)pc;                                    \
   /**/
 
 #define SCOPED_TSAN_INTERCEPTOR(func, ...) \
     SCOPED_INTERCEPTOR_RAW(func, __VA_ARGS__); \
     if (REAL(func) == 0) { \
       Report("FATAL: ThreadSanitizer: failed to intercept %s\n", #func); \
       Die(); \
     }                                                    \
     if (!thr->is_inited || thr->ignore_interceptors || thr->in_ignored_lib) \