[PATCH] D58741: [NFC][Sanitizer] Add new BufferedStackTrace::Unwind API

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/msan/msan.h:342
+    if (!SANITIZER_CAN_FAST_UNWIND || !fast)                              \
+      size = Min(size, 1);                                                \
+    stack.Unwind(pc, bp, nullptr, fast, size);                            \
----------------
do you need fast = false; here?


================
Comment at: compiler-rt/lib/msan/msan.h:352
   if (msan_inited)                                       \
-  GetStackTrace(&stack, kStackTraceMax, pc, bp, nullptr, \
-                common_flags()->fast_unwind_on_fatal)
+    stack.Unwind(pc, bp, nullptr, common_flags()->fast_unwind_on_fatal)
 
----------------
Can you add BufferedStackTrace::UnwindOnFatal and read fast_unwind_on_fatal there?
if possible in followup patch


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h:23
+// The bp may refer to the current frame or to the caller's frame.
+void GetStackTrace(BufferedStackTrace *stack, uptr max_depth, uptr pc, uptr bp,
+                   void *context, bool request_fast_unwind);
----------------
can you make it as private BufferedStackTrace::UnwindImpl
as  follow up patch



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_report.cc:104
   bool fast = common_flags()->fast_unwind_on_fatal;
-  if (StackTrace::WillUseFastUnwind(fast)) {
-    GetThreadStackTopAndBottom(false, &top, &bottom);
-    stack->Unwind(kStackTraceMax, pc, bp, nullptr, top, bottom, true);
-  } else
-    stack->Unwind(kStackTraceMax, pc, 0, nullptr, 0, 0, false);
+  stack->Unwind(pc, bp, nullptr, fast);
 
----------------
please inline fast_unwind_on_fatal


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_rtl.cc:331
                           BufferedStackTrace *stack) {
-  uptr top = 0;
-  uptr bottom = 0;
   bool fast = common_flags()->fast_unwind_on_fatal;
+  stack->Unwind(sig.pc, sig.bp, sig.context, fast);
----------------
please inline fast_unwind_on_fatal


================
Comment at: compiler-rt/lib/ubsan/ubsan_diag_standalone.cc:34
+  GET_CURRENT_PC_BP;
+  bool fast = common_flags()->fast_unwind_on_fatal;
   BufferedStackTrace stack;
----------------
it's better and consistent as
```
stack.Unwind(pc, bp, nullptr, common_flags()->fast_unwind_on_fatal);
```


================
Comment at: compiler-rt/lib/ubsan/ubsan_signals_standalone.cc:45
                           BufferedStackTrace *stack) {
-  GetStackTrace(stack, kStackTraceMax, sig.pc, sig.bp, sig.context,
+  stack->Unwind(sig.pc, sig.bp, sig.context,
                 common_flags()->fast_unwind_on_fatal);
----------------
maybe stack.UnwindOnSingnal(sig) in a separate patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58741