[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:251
   using namespace __hwasan;
+  stack->size = 0; // TODO(yln): investigate why this is needed
+
----------------
vitalybuka wrote:
> 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
in other words, you can remove "investigate why this is needed" comment


================
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();
----------------
yln wrote:
> vitalybuka wrote:
> > we don't need these local variables
> 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.
> 
> 
acknowledged 


================
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) \
----------------
yln wrote:
> vitalybuka wrote:
> > 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.
> 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.
dummy implementation as we do for the rest of sanitizers


================
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)
----------------
yln wrote:
> vitalybuka wrote:
> > this UNREACHABLE can be a CHECK(SANITIZER_CAN_FAST_UNWIND) in UnwindFast
> I don't understand your comment.
> UnwindFast is only available on SANITIZER_CAN_FAST_UNWIND platforms.
> 
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")



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58696