[PATCH] D74369: [AddressSanitizer] Ensure only AllocaInst is passed to dbg.declare

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:3118
     LocalStackBase =
         DoDynamicAlloca ? createAllocaForLayout(IRB, L, true) : StaticAlloca;
     LocalStackBaseAlloca = LocalStackBase;
----------------
@eugenis I tried to change createAllocaForLayout to return an alloca, but I couldn't manage it. This required changes all over this pass. I tried tackling this twice, but the resulting IR from the pass was broken in ways I didn't quite understand.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:3123
   // Replace Alloca instructions with base+offset.
+  Value *LocalStackBaseAllocaPtr =
+      isa<PtrToIntInst>(LocalStackBaseAlloca)
----------------
aprantl wrote:
> `// The ptrtoint operation is transparent to the debug info and only confuses later passes.`
I've reworded this a bit, lmk if it's all right.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:3127
+          : LocalStackBaseAlloca;
+  assert(isa<AllocaInst>(LocalStackBaseAllocaPtr) &&
+         "Variable descriptions relative to ASan stack base will be dropped");
----------------
@eugenis Re:

> Also, would not this new assert fire in the case of fake stack?

I think LocalStackBaseAlloca is either an alloca, or is the result of createAllocaForLayout, so the assert should not fire. Testing with an ASan-enabled stage2 build and with check-llvm was clean.


================
Comment at: llvm/test/DebugInfo/X86/asan_debug_info.ll:3
+; RUN:   llc -O0 -filetype=obj - -o - | \
+; RUN:   llvm-dwarfdump - | FileCheck %s
+
----------------
aprantl wrote:
> While having an end-to-end test is great, I wonder if we should also have an IR->IR test that just specifies the ASAN transform's expected behavior.
The existing IR->IR tests do cover what the ASan transform produces. I'll leave a note about this in one of the tests: they used to match the `ptrtoint`, I've changed one of them to match an alloca.


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

https://reviews.llvm.org/D74369