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

[Julian Lettner via Phabricator via llvm-commits]

yln marked 4 inline comments as done.
yln added inline comments.


================
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();
----------------
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.




================
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) \
----------------
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.


================
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)
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58696