[asan] Invalid debug info for promotable allocas

[kubamracek]

Summary

Hi everyone,

Since r230724 ("Skip promotable allocas to improve performance at -O0"), there is a regression in the generated debug info for those non-instrumented variables. When inspecting such a variable's value in LLDB, you often get garbage instead of the actual value. Fred was able to track this down:

What happens is quite sad: The variable doesn’t get instrumented and thus should behave like a standard stack variable. However, ASAN’s instrumentation is inserted before the creation of the non-instrumented alloca. The only allocas that are considered standard stack variables are the ones declared in the first basic-block, but the initial instrumentation setup in the function breaks that invariant. Indeed, if moving the alloca to the first BB, things work. (This shows that the llvm.dbg.declare intrinsic is badly defined. Depending on whether the alloca it refers to is in the first BB or not, it means something quite different…)

The simplest way that I can think of preventing this issue is to make sure that the uninstrumented allocas stay in the first BB. I think this is safe, because static promotable allocas shouldn’t depend on any other value, and moving them up they will still dominate their uses, thus the SSA form should be maintained. I might very well be missing some corner cases though. The following patch does that.

[kcc]

Two style comments.
Also, please add a test.

lib/Transforms/Instrumentation/AddressSanitizer.cpp

528 ↗

(On Diff #29649)

NonInstrumentedStaticAllocaVec

1741 ↗

(On Diff #29649)

This code tends to not have {} in such case.

[kubamracek]

Added test. Addressed style comments.

[kcc]

test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca.ll

13 ↗

(On Diff #29760)

Please comment why %non_instrumented will not get instrumented.

21 ↗

(On Diff #29760)

Please make sure the test works in a non-asserts build.
I remember there were issues with non-asserts build removing the var names.

[samsonov]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

1741 ↗

(On Diff #29760)

According to logic, this should be InsBefore, not AllocaVec[0].

[kubamracek]

Addressing review comments. I checked that the test passes in a release (RelWithDebInfo) build.

[glider]

LGTM as soon as others are happy.

[kcc]

LGTM

llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp

struct FunctionStackPoisoner : public InstVisitor<FunctionStackPoisoner> {
   AddressSanitizer &ASan;
   DIBuilder DIB;
   LLVMContext *C;
   Type *IntptrTy;
   Type *IntptrPtrTy;
   ShadowMapping Mapping;
 
   SmallVector<AllocaInst *, 16> AllocaVec;
+  SmallVector<AllocaInst *, 16> NonInstrumentedStaticAllocaVec;
   SmallVector<Instruction *, 8> RetVec;
   unsigned StackAlignment;
 
   Function *AsanStackMallocFunc[kMaxAsanStackMallocSizeClass + 1],
       *AsanStackFreeFunc[kMaxAsanStackMallocSizeClass + 1];
   Function *AsanPoisonStackMemoryFunc, *AsanUnpoisonStackMemoryFunc;
   Function *AsanAllocaPoisonFunc, *AsanAllocasUnpoisonFunc;
 
struct FunctionStackPoisoner : public InstVisitor<FunctionStackPoisoner> {
   //   tmp = alloca i8, new_size, max(align, 32)
   //   addr = tmp + 32 (first 32 bytes are for the left redzone).
   // Additional_size is added to make new memory allocation contain not only
   // requested memory, but also left, partial and right redzones.
   void handleDynamicAllocaCall(AllocaInst *AI);
 
   /// \brief Collect Alloca instructions we want (and can) handle.
   void visitAllocaInst(AllocaInst &AI) {
-    if (!ASan.isInterestingAlloca(AI)) return;
+    if (!ASan.isInterestingAlloca(AI)) {
+      if (AI.isStaticAlloca()) NonInstrumentedStaticAllocaVec.push_back(&AI);
+      return;
+    }
 
     StackAlignment = std::max(StackAlignment, AI.getAlignment());
     if (ASan.isDynamicAlloca(AI))
       DynamicAllocaVec.push_back(&AI);
     else
       AllocaVec.push_back(&AI);
   }
 
void FunctionStackPoisoner::poisonStack() {
   DebugLoc EntryDebugLocation;
   if (auto SP = getDISubprogram(&F))
     EntryDebugLocation = DebugLoc::get(SP->getScopeLine(), 0, SP);
 
   Instruction *InsBefore = AllocaVec[0];
   IRBuilder<> IRB(InsBefore);
   IRB.SetCurrentDebugLocation(EntryDebugLocation);
 
+  for (auto *AI : NonInstrumentedStaticAllocaVec) AI->moveBefore(InsBefore);
+
   SmallVector<ASanStackVariableDescription, 16> SVD;
   SVD.reserve(AllocaVec.size());
   for (AllocaInst *AI : AllocaVec) {
     ASanStackVariableDescription D = {AI->getName().data(),
                                       ASan.getAllocaSizeInBytes(AI),
                                       AI->getAlignment(), AI, 0};
     SVD.push_back(D);
   }

llvm/trunk/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca.ll

+; This test checks that non-instrumented allocas stay in the first basic block.
+; Only first-basic-block allocas are considered stack slots, and moving them
+; breaks debug info.
+
+; RUN: opt < %s -asan -asan-module -S | FileCheck %s
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.10.0"
+
+define i32 @foo() sanitize_address {
+entry:
+  ; Regular alloca, will get instrumented (forced by the ptrtoint below).
+  %instrumented = alloca i32, align 4
+
+  ; Won't be instrumented because of asan-skip-promotable-allocas.
+  %non_instrumented = alloca i32, align 4
+  store i32 0, i32* %non_instrumented, align 4
+  %value = load i32, i32* %non_instrumented, align 4
+
+  %ptr = ptrtoint i32* %instrumented to i64
+  ret i32 %value
+}
+
+; CHECK: entry:
+; CHECK: %non_instrumented = alloca i32, align 4
+; CHECK: load i32, i32* @__asan_option_detect_stack_use_after_return