[PATCH] D58696: [NFC][Sanitizer] Cleanup GetStackTrace implementations

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/hwasan/hwasan.cc:247
 
-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,
----------------
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


================
Comment at: compiler-rt/lib/hwasan/hwasan.cc:251
   using namespace __hwasan;
+  stack->size = 0; // TODO(yln): investigate why this is needed
+
----------------
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


================
Comment at: compiler-rt/lib/hwasan/hwasan.cc:259
+  if (StackTrace::WillUseFastUnwind(request_fast_unwind)) {
+    uptr top = t->stack_top();
+    uptr bottom = t->stack_bottom();
----------------
we don't need these local variables


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h:166
 
+#if SANITIZER_CAN_FAST_UNWIND
+  #define UNWIND_FAST(stack, pc, bp, top, bottom, max_depth) \
----------------
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.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h:173
+  #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)
----------------
this UNREACHABLE can be a CHECK(SANITIZER_CAN_FAST_UNWIND) in UnwindFast


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58696/new/

https://reviews.llvm.org/D58696