[PATCH] D58861: [NFC][Sanitizer] Cleanup ASan's GetStackTrace implementation

[Julian Lettner via Phabricator via llvm-commits]

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


================
Comment at: compiler-rt/lib/asan/asan_stack.cc:48
+
+  ScopedUnwinding unwind_scope(t);
+  if (WillUseFastUnwind(request_fast)) {
----------------
vitalybuka wrote:
> UNREACHABLE makes it not NFC
> 
> What about:
> ```
> 
>   if (t && !t->isUnwinding() && WillUseFastUnwind(request_fast)) {
>     ScopedUnwinding unwind_scope(t);
>     uptr top = t->stack_top();
>     uptr bottom = t->stack_bottom();
>     if (!SANITIZER_MIPS || IsValidFrame(bp, top, bottom)) {
>       UnwindFast(pc, bp, top, bottom, max_depth);
>       return;
>     } 
>   }
> 
> #if SANITIZER_CAN_SLOW_UNWIND
>     UnwindSlowWithOptionalContext(pc, context, max_depth);
> #endif
> ```
> 
> 
You are right, the patch is not strictly NFC:
1) Scope was moved
2) t->isUnwinding() also skips when slow unwinding

I think these are issues with the current implementation.
I will remove those changes to make this patch strictly NFC and provide separate patches with explanations.

Note that, UNREACHABLE is required for NFC. We are effectively inlining the old Unwind overload which has this code.



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

https://reviews.llvm.org/D58861