sanitizer_common: allow multiple GET_CURRENT_PC() in a function

[dvyukov]

Summary

Currently if GET_CURRENT_PC() is used multiple times
in a single function, it will cause duplicate this_pc
label errors.
Append line number to the this_pc label to this fix.

This fixes existing build breakages on mac:
http://45.33.8.238/macm1/14037/step_4.txt

[melver]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

234

This is fine, given it's internal and is simple.

But, just wondering, wouldn't an always_inline function not have this problem? Or would it not work for this usecase?

[dvyukov]

actually append LINE
the previous version resulted in this_pcLINE

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

234

It's actually not that simple. The previous version was broken and resulted in this_pcLINE. This new version works.

An inline function may increase chances of additional inline frames. But it seems that we will need to filter them out anyway (I already see 2 other bot failures related to inline functions). And if we filter them out, it will be easy to filter out an inline function used in GET_CURRENT_PC as well.

[dvyukov]

change GET_CURRENT_PC to StackTrace::GetCurrentPcFast

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

234

Switched to a function. It's indeed nicer.

But the function indeed shows up on top of most frames, so we need https://reviews.llvm.org/D106081 first or all tests fail.

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

234

Wait, no, I rebased this on top of https://reviews.llvm.org/D106081 and it works with gcc now.
But all tests still fail with clang because... drumroll... clang fails to inline ALWAYS_INLINE function and inserts a call:

4493f1:       e8 fa f9 ff ff          callq  448df0 <_ZN11__sanitizer10StackTrace16GetCurrentPcFastEv>
4493f9:       48 89 c6                mov    %rax,%rsi

and this top frame is completely removed by D106081, so we end up without top frame at all...

[melver]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

79

Perhaps this is missing an ALWAYS_INLINE to make Clang play nice?

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

79

No, it doesn't help.
I suspect clang does not want to inline functions with addresses of labels taken, otherwise why wouldn't it inline a function that results in a single instruction even w/o inline?

I don't have any other ideas besides reverting to macro...

[vitalybuka]

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

268

Should you rather duplicate the test for Fast version?

[dvyukov]

revert back to GET_CURRENT_PC() macro

[dvyukov]

duplicated test for GET_CURRENT_PC

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

268

Done. PTAL.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

145

Why not __builtin_return_address(0) ?
Is this because it's wrong if inlined?

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

272

can you also add a test like

a= GetCurrentPcFast()
b= GetCurrentPcFast()
c= GetCurrentPcFast()
EXPECT_NE(a,b);
EXPECT_NE(b,c);

[dvyukov]

The new version in https://reviews.llvm.org/D106046 amends this as well.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

145

__builtin_return_address(0) is _caller_ PC and we want _current_ PC.

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

272

The new version in https://reviews.llvm.org/D106046 includes this test.

compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h

 
 extern "C" void __ud2(void);
 # pragma intrinsic(__ud2)
 inline void Trap() {
   __ud2();
 }
 #endif
 
+#define CONCAT2(x, y) x##y
+#define CONCAT(x, y) CONCAT2(x, y)
+
 #define HANDLE_EINTR(res, f)                                       \
   {                                                                \
     int rverrno;                                                   \
     do {                                                           \
       res = (f);                                                   \
     } while (internal_iserror(res, &rverrno) && rverrno == EINTR); \
   }
 

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

static bool WillUseFastUnwind(bool request_fast_unwind) {
     if (!SANITIZER_CAN_FAST_UNWIND)
       return false;
     if (!SANITIZER_CAN_SLOW_UNWIND)
       return true;
     return request_fast_unwind;
   }
 
   static uptr GetCurrentPc();
   static inline uptr GetPreviousInstructionPc(uptr pc);
   static uptr GetNextInstructionPc(uptr pc);
 };
 
 // Performance-critical, must be in the header.
 ALWAYS_INLINE
 uptr StackTrace::GetPreviousInstructionPc(uptr pc) {
 #if defined(__arm__)
   // T32 (Thumb) branch instructions might be 16 or 32 bit long,

 #else
   return pc + 1;
 #endif
 }
 
 // StackTrace that owns the buffer used to store the addresses.
 struct BufferedStackTrace : public StackTrace {
   uptr trace_buffer[kStackTraceMax];
   uptr top_frame_bp;  // Optional bp of a top frame.
 
   BufferedStackTrace() : StackTrace(trace_buffer, 0), top_frame_bp(0) {}
 
   void Init(const uptr *pcs, uptr cnt, uptr extra_top_pc = 0);
 
   // Get the stack trace with the given pc and bp.
   // The pc will be in the position 0 of the resulting stack trace.
   // The bp may refer to the current frame or to the caller's frame.
#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
 
-#define GET_CURRENT_PC()                                   \
-  ({                                                       \
-    this_pc:                                               \
-      StackTrace::GetNextInstructionPc((uptr) && this_pc); \
+#define GET_CURRENT_PC()                                               \
+  ({                                                                   \
+    CONCAT(this_pc, __LINE__)                                          \
+        : StackTrace::GetNextInstructionPc((uptr) &&                   \
+                                           CONCAT(this_pc, __LINE__)); \
   })
 
 #endif  // SANITIZER_STACKTRACE_H

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

 //===-- sanitizer_stacktrace_test.cpp -------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of ThreadSanitizer/AddressSanitizer runtime.
 //
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_common/sanitizer_stacktrace.h"
 
 #include <string.h>
 
 #include <algorithm>
 #include <string>
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_internal_defs.h"
 
 using testing::ContainsRegex;

 
 TEST(SlowUnwindTest, ShortStackTrace) {
   BufferedStackTrace stack;
   uptr pc = StackTrace::GetCurrentPc();
   uptr bp = GET_CURRENT_FRAME();
   stack.Unwind(pc, bp, nullptr, false, /*max_depth=*/0);
   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(SlowUnwindTest, GET_CURRENT_PC) {
+  BufferedStackTrace stack;
+  uptr pc = GET_CURRENT_PC();
+  uptr bp = GET_CURRENT_FRAME();
+  stack.Unwind(pc, bp, nullptr, false, /*max_depth=*/0);
+  EXPECT_EQ(0U, stack.size);
+  EXPECT_EQ(0U, stack.top_frame_bp);
+  // Ensure that GET_CURRENT_PC() macro can be used
+  // multiple times in a single function.
+  pc = GET_CURRENT_PC();
+  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);
+}
+
 // 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