[PATCH] D58696: [NFC][Sanitizer] Cleanup GetStackTrace implementations
Vitaly Buka via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 26 13:59:23 PST 2019
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
More information about the llvm-commits
mailing list