[NFC][Sanitizer] Cleanup ASan's GetStackTrace implementation

[yln]

Summary

Cleanup ASan's __sanitizer::BufferedStackTrace::UnwindImpl (formerly
GetStackTrace) implementation. Start with ASan because it is the most
complex implementation.

GetStackTrace implementations seem to have started out as exact copies
of the original implementation in ASan, but have diverged in subtle
ways. My goal is to parameterize this algorithm (via templating or
callbacks) so we can share the implementation and get rid of the
inversed dependency (sanitizer_common depends on concrete
implementations in asan, ubsan, etc.). This should also help us to avoid
those pesky linker errors caused by undefined, duplicate, and weak
symbols on Windows.

[vitalybuka]

ways. My goal is to parameterize this algorithm (via templating or callbacks) so we can share the implementation and get rid of the inversed dependency (sanitizer_common depends on

concrete implementations in asan, ubsan, etc.). This should also help us to avoid those pesky linker errors caused by undefined, duplicate, and weak symbols on Windows.

I like what you have already done, but I don't think we are looking to remove inverse deps. Historically it's how compiler-rt was made, and fixing just unwinder will not make a difference.
Spreading this to other parts seems is a lot of work.

compiler-rt/lib/asan/asan_stack.cc

77

Originally the patch was reverted when IsValidFrame was added like this
https://reviews.llvm.org/D18690?vs=on&id=52346&whitespace=ignore-most#toc

Also given that you've commented out top/bottom checks r354718 I don't want to enabled IsValidFrame everywhere.
Could you please keep it for MIPS, and return to this later, if you still want this, after resolving r354718

81

misaligned

[yln]

Put back special case for MIPS.

[vitalybuka]

compiler-rt/lib/asan/asan_stack.cc

73

UNREACHABLE makes it not NFC

What about:

  if (t && !t->isUnwinding() && WillUseFastUnwind(request_fast)) {
    ScopedUnwinding unwind_scope(t);
    uptr top = t->stack_top();
    uptr bottom = t->stack_bottom();
    if (!SANITIZER_MIPS || IsValidFrame(bp, top, bottom)) {
      UnwindFast(pc, bp, top, bottom, max_depth);
      return;
    } 
  }

#if SANITIZER_CAN_SLOW_UNWIND
    UnwindSlowWithOptionalContext(pc, context, max_depth);
#endif

[yln]

compiler-rt/lib/asan/asan_stack.cc

73

You are right, the patch is not strictly NFC:

1. Scope was moved
2. t->isUnwinding() also skips when slow unwinding

I think these are issues with the current implementation.
I will remove those changes to make this patch strictly NFC and provide separate patches with explanations.

Note that, UNREACHABLE is required for NFC. We are effectively inlining the old Unwind overload which has this code.

[yln]

Adopting Vitaly's suggestion

[vitalybuka]

Reverted with r355369

[vitalybuka]

rebase

[vitalybuka]

lib/sanitizer_common/sanitizer_stacktrace.h

138 ↗

(On Diff #189230)

Please use clang-format --style=file

compiler-rt/lib/asan/asan_stack.cc

void __sanitizer::BufferedStackTrace::UnwindImpl(
   if (UNLIKELY(!asan_inited))
     return;
   request_fast = StackTrace::WillUseFastUnwind(request_fast);
   AsanThread *t = GetCurrentThread();
   ScopedUnwinding unwind_scope(t);
   if (!unwind_scope.CanUnwind())
     return;
   if (request_fast) {
-    if (t) {
-      Unwind(max_depth, pc, bp, nullptr, t->stack_top(), t->stack_bottom(),
-             true);
-    }
+    if (t)
+      UnwindFast(pc, bp, t->stack_top(), t->stack_bottom(), max_depth);
     return;
   }
   if (SANITIZER_MIPS && t &&
       !IsValidFrame(bp, t->stack_top(), t->stack_bottom()))
     return;
-  Unwind(max_depth, pc, bp, context, 0, 0, false);
+#if SANITIZER_CAN_SLOW_UNWIND
+  UnwindSlowWithOptionalContext(pc, context, max_depth);
+#endif
 }
 
 // ------------------ Interface -------------- {{{1
 
 extern "C" {
 SANITIZER_INTERFACE_ATTRIBUTE
 void __sanitizer_print_stack_trace() {
   using namespace __asan;
   PRINT_CURRENT_STACK();
 }
 }  // extern "C"

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h

void UnwindImpl(uptr pc, uptr bp, void *context, bool request_fast,
                   u32 max_depth);
 
   // UnwindFast/Slow have platform-specific implementations
   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 UnwindSlowWithOptionalContext(uptr pc, void *context, u32 max_depth) {
+    if (context)
+      UnwindSlow(pc, context, max_depth);
+    else
+      UnwindSlow(pc, max_depth);
+  }
+
   void PopStackFrames(uptr count);
   uptr LocatePcInTrace(uptr pc);
 
   BufferedStackTrace(const BufferedStackTrace &) = delete;
   void operator=(const BufferedStackTrace &) = delete;
 
   friend class FastUnwindTest;
 };