[NFC][Sanitizer] Cleanup GetStackTrace implementations

[yln]

Summary

GetStackTrace implementations seem to have started out as exact copies
of the original implementation in ASan, but have diverged in subtle
ways. I left comments for these differences and will address them
according to your guidance in a follow-up patch.

[vitalybuka]

compiler-rt/lib/hwasan/hwasan.cc

247

It wold be nice to have this as multiple NFC patches:

1. rename arguments
2. move size=0 to top
3. move SymbolizerScope sym_scope

...

as-is I am not 100% sure that this patch is NFC
with tiny patches it would be easier to nail accidental regression

251

it must be either CHECK_EQ(0, stack->size) or = 0
So I am fine as-is where we just let to reuse instance of BufferedStackTrace by resetting size

259

we don't need these local variables

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

166

Can you please undo this macro? On first look I don't see value in them.
If you want to try convince me please try new patch with macro change only.

173

this UNREACHABLE can be a CHECK(SANITIZER_CAN_FAST_UNWIND) in UnwindFast

[yln]

compiler-rt/lib/hwasan/hwasan.cc

259

Agreed. If this would be the final version, I would inline them.
My goal here was to make all GetStackTrace look the same. Do you see the pattern?
It would be nice if we could remove it altogether or at least make it a private helper of the BufferedStackTrace class.

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

166

The only reason for these macros is that we can't directly do a call like stack->UnwindFast(..) from other code since it might not be available on a platform (not even a dummy symbol).

Let me know if you think that having dummy implementation that errors with "not supported" is preferred over having these macros.

173

I don't understand your comment.
UnwindFast is only available on SANITIZER_CAN_FAST_UNWIND platforms.

[vitalybuka]

compiler-rt/lib/hwasan/hwasan.cc

251

in other words, you can remove "investigate why this is needed" comment

259

acknowledged

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

166

dummy implementation as we do for the rest of sanitizers

173

if we have an implementation that
sanity check in the UnwindFast/UnwindSlow:
CHECK(SANITIZER_CAN_FAST_UNWIND/SANITIZER_CAN_SLOW_UNWIND)

if we have dummy function:
UNREACHABLE("Fast unwinder is unsupported")

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

108

this bug is fixed

166

Note that the macro approach is what's currently there. These macros just make it more explicit. I am happy to change this over to defining dummy symbols. Can I do this as a follow-up?

compiler-rt/lib/asan/asan_stack.cc

 
 u32 GetMallocContextSize() {
   return atomic_load(&malloc_context_size, memory_order_acquire);
 }
 
 }  // namespace __asan
 
 void __sanitizer::GetStackTrace(BufferedStackTrace *stack, uptr max_depth,
-                                uptr pc, uptr bp, void *context, bool fast) {
+                                uptr pc, uptr bp, void *context,
+                                bool request_fast_unwind) {
   using namespace __asan;
-#if SANITIZER_WINDOWS
-  stack->Unwind(max_depth, pc, 0, context, 0, 0, false);
-#else
-  AsanThread *t;
-  stack->size = 0;
-  if (LIKELY(asan_inited)) {
-    if ((t = GetCurrentThread()) && !t->isUnwinding()) {
-      uptr stack_top = t->stack_top();
-      uptr stack_bottom = t->stack_bottom();
-      ScopedUnwinding unwind_scope(t);
-      if (!SANITIZER_MIPS || IsValidFrame(bp, stack_top, stack_bottom)) {
-        if (StackTrace::WillUseFastUnwind(fast))
-          stack->Unwind(max_depth, pc, bp, nullptr, stack_top, stack_bottom,
-                      true);
-        else
-          stack->Unwind(max_depth, pc, 0, context, 0, 0, false);
-      }
-    } else if (!t && !fast) {
-      /* If GetCurrentThread() has failed, try to do slow unwind anyways. */
-      stack->Unwind(max_depth, pc, bp, context, 0, 0, false);
-    }
+  stack->size = 0; // TODO(yln): investigate why this is needed
+
+  if (UNLIKELY(!asan_inited)) return;
+
+  AsanThread *t = GetCurrentThread();
+  if (t && t->isUnwinding()) return;
+
+  // TODO(yln): no scope during "best effort" slow unwind without thread
+  // TODO(yln): use SymbolizerScope sym_scope; here instead of unwind_scope?
+  if (!t) {
+    MAYBE_UNWIND_SLOW(stack, pc, context, max_depth);
+    return;
+  }
+
+  if (StackTrace::WillUseFastUnwind(request_fast_unwind)) {
+    uptr top = t->stack_top();
+    uptr bottom = t->stack_bottom();
+    if (IsValidFrame(bp, top, bottom))
+      UNWIND_FAST(stack, pc, bp, top, bottom, max_depth);
+      MAYBE_UNWIND_SLOW(stack, pc, context, max_depth);
+  } else {
+    UNWIND_SLOW(stack, pc, context, max_depth);
   }
-#endif // SANITIZER_WINDOWS
 }
 
 // ------------------ Interface -------------- {{{1
 
 extern "C" {
 SANITIZER_INTERFACE_ATTRIBUTE
 void __sanitizer_print_stack_trace() {
   using namespace __asan;
   PRINT_CURRENT_STACK();
 }
 }  // extern "C"

compiler-rt/lib/hwasan/hwasan.cc

void InitInstrumentation() {
   InitThreads();
   hwasanThreadList().CreateCurrentThread();
 
   hwasan_instrumentation_inited = 1;
 }
 
 } // namespace __hwasan
 
-void __sanitizer::GetStackTrace(BufferedStackTrace *stack, uptr max_s, uptr pc,
-                                uptr bp, void *context,
+void __sanitizer::GetStackTrace(BufferedStackTrace *stack, uptr max_depth,
+                                uptr pc, uptr bp, void *context,
                                 bool request_fast_unwind) {
   using namespace __hwasan;
+  stack->size = 0; // TODO(yln): investigate why this is needed
+
   Thread *t = GetCurrentThread();
-  if (!t) {
-    // the thread is still being created.
-    stack->size = 0;
-    return;
-  }
-  if (!StackTrace::WillUseFastUnwind(request_fast_unwind)) {
-    // Block reports from our interceptors during _Unwind_Backtrace.
-    SymbolizerScope sym_scope;
-    return stack->Unwind(max_s, pc, bp, context, 0, 0, request_fast_unwind);
+  if (!t) return;
+  // TODO(yln): no attempt to slow unwind if we can't get current thread?
+
+  if (StackTrace::WillUseFastUnwind(request_fast_unwind)) {
+    uptr top = t->stack_top();
+    uptr bottom = t->stack_bottom();
+    // TODO(yln): no check for IsValidFrame ???
+    UNWIND_FAST(stack, pc, bp, top, bottom, max_depth);
+  } else {
+    UNWIND_SLOW(stack, pc, context, max_depth);
   }
-  if (StackTrace::WillUseFastUnwind(request_fast_unwind))
-    stack->Unwind(max_s, pc, bp, nullptr, t->stack_top(), t->stack_bottom(),
-                  true);
-  else
-    stack->Unwind(max_s, pc, 0, context, 0, 0, false);
 }
 
 // Interface.
 
 using namespace __hwasan;
 
 uptr __hwasan_shadow_memory_dynamic_address;  // Global interface symbol.
 

compiler-rt/lib/lsan/lsan.cc

 ///// Interface to the common LSan module. /////
 bool WordIsPoisoned(uptr addr) {
   return false;
 }
 
 }  // namespace __lsan
 
 void __sanitizer::GetStackTrace(BufferedStackTrace *stack, uptr max_depth,
-                                uptr pc, uptr bp, void *context, bool fast) {
+                                uptr pc, uptr bp, void *context,
+                                bool request_fast_unwind) {
   using namespace __lsan;
-  uptr stack_top = 0, stack_bottom = 0;
-  ThreadContext *t;
-  if (StackTrace::WillUseFastUnwind(fast) &&
-      (t = CurrentThreadContext())) {
-    stack_top = t->stack_end();
-    stack_bottom = t->stack_begin();
-  }
-  if (!SANITIZER_MIPS || IsValidFrame(bp, stack_top, stack_bottom)) {
-    if (StackTrace::WillUseFastUnwind(fast))
-      stack->Unwind(max_depth, pc, bp, nullptr, stack_top, stack_bottom, true);
+
+  ThreadContext *t = CurrentThreadContext();
+  if (!t) {
+    MAYBE_UNWIND_SLOW(stack, pc, context, max_depth);
+    return;
+  }
+
+  // TODO(yln): No unwinding scope?
+  if (StackTrace::WillUseFastUnwind(request_fast_unwind)) {
+    uptr top = t->stack_end();  // TODO(yln): naming is end/begin
+    uptr bottom = t->stack_begin(); // instead of top/bottom
+    if (IsValidFrame(bp, top, bottom))
+      UNWIND_FAST(stack, pc, bp, top, bottom, max_depth);
     else
-      stack->Unwind(max_depth, pc, 0, context, 0, 0, false);
+      MAYBE_UNWIND_SLOW(stack, pc, context, max_depth);
+  } else {
+    UNWIND_SLOW(stack, pc, context, max_depth);
   }
 }
 
 using namespace __lsan;  // NOLINT
 
 static void InitializeFlags() {
   // Set all the default values.
   SetCommonFlagsDefaults();

compiler-rt/lib/msan/msan.cc

u32 ChainOrigin(u32 id, StackTrace *stack) {
   Origin o = Origin::FromRawId(id);
   stack->tag = StackTrace::TAG_UNKNOWN;
   Origin chained = Origin::CreateChainedOrigin(o, stack);
   return chained.raw_id();
 }
 
 } // namespace __msan
 
-void __sanitizer::GetStackTrace(BufferedStackTrace *stack, uptr max_s, uptr pc,
-                                uptr bp, void *context,
+void __sanitizer::GetStackTrace(BufferedStackTrace *stack, uptr max_depth,
+                                uptr pc, uptr bp, void *context,
                                 bool request_fast_unwind) {
   using namespace __msan;
-  MsanThread *t = GetCurrentThread();
-  if (!t || !StackTrace::WillUseFastUnwind(request_fast_unwind)) {
-    // Block reports from our interceptors during _Unwind_Backtrace.
-    SymbolizerScope sym_scope;
-    return stack->Unwind(max_s, pc, bp, context, 0, 0, false);
+
+  MsanThread *t = GetCurrentThread();
+  if (!t) {
+    MAYBE_UNWIND_SLOW(stack, pc, context, max_depth);
+    return;
+  }
+
+  if (StackTrace::WillUseFastUnwind(request_fast_unwind)) {
+    uptr top = t->stack_top();
+    uptr bottom = t->stack_bottom();
+    // TODO(yln): no check for IsValidFrame ???
+    UNWIND_FAST(stack, pc, bp, top, bottom, max_depth);
+  } else {
+    UNWIND_SLOW(stack, pc, context, max_depth);
   }
-  if (StackTrace::WillUseFastUnwind(request_fast_unwind))
-    stack->Unwind(max_s, pc, bp, nullptr, t->stack_top(), t->stack_bottom(),
-                  true);
-  else
-    stack->Unwind(max_s, pc, 0, context, 0, 0, false);
 }
 
 // Interface.
 
 using namespace __msan;
 
 #define MSAN_MAYBE_WARNING(type, size)              \
   void __msan_maybe_warning_##size(type s, u32 o) { \

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

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);
   void Unwind(u32 max_depth, uptr pc, uptr bp, void *context, uptr stack_top,
               uptr stack_bottom, bool request_fast_unwind);
+  void UnwindFast(uptr pc, uptr bp, uptr stack_top, uptr stack_bottom,
+                  u32 max_depth);
+
+  void UnwindSlowWithOptionalContext(uptr pc, void *context, u32 max_depth) {
+    if (context)
+      UnwindSlow(pc, max_depth);
+    else
+      UnwindSlow(pc, context, max_depth);
+  }
 
   void Reset() {
     *static_cast<StackTrace *>(this) = StackTrace(trace_buffer, 0);
     top_frame_bp = 0;
   }
 
  private:
-  void UnwindFast(uptr pc, uptr bp, uptr stack_top, uptr stack_bottom,
-                  u32 max_depth);
   void UnwindSlow(uptr pc, u32 max_depth);
   void UnwindSlow(uptr pc, void *context, u32 max_depth);
   void PopStackFrames(uptr count);
   uptr LocatePcInTrace(uptr pc);
 
   BufferedStackTrace(const BufferedStackTrace &) = delete;
   void operator=(const BufferedStackTrace &) = delete;
 };
#define GET_CURRENT_PC_BP_SP \
   uptr local_stack;                           \
   uptr sp = (uptr)&local_stack
 
 #define GET_CURRENT_PC_BP \
   uptr bp = GET_CURRENT_FRAME();              \
   uptr pc = StackTrace::GetCurrentPc();
 
 
+#if SANITIZER_CAN_FAST_UNWIND
+  #define UNWIND_FAST(stack, pc, bp, top, bottom, max_depth) \
+    (stack)->UnwindFast((pc), (bp), (top), (bottom), (max_depth))
+  #define MAYBE_UNWIND_FAST(stack, pc, bp, top, bottom, max_depth) \
+    UNWIND_FAST(stack, pc, bp, top, bottom, max_depth)
+#else
+  #define UNWIND_FAST(stack, pc, bp, top, bottom, max_depth) \
+    UNREACHABLE("Fast unwinder is unsupported")
+  #define MAYBE_UNWIND_FAST(stack, pc, bp, top, bottom, max_depth)
+#endif
+
+#if SANITIZER_CAN_SLOW_UNWIND
+  #define UNWIND_SLOW(stack, pc, context, max_depth) \
+    (stack)->UnwindSlowWithOptionalContext((pc), (context), (max_depth));
+  #define MAYBE_UNWIND_SLOW(stack, pc, context, max_depth) \
+    UNWIND_SLOW(stack, pc, context, max_depth)
+#else
+  #define UNWIND_SLOW(stack, pc, context, max_depth) \
+    UNREACHABLE("Slow unwinder is unsupported")
+  #define MAYBE_UNWIND_SLOW(stack, pc, context, max_depth)
+#endif
+
 #endif  // SANITIZER_STACKTRACE_H