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

[vsk]

Summary

Various parts of the LLVM code generator assume that the address
argument of a dbg.declare is not a ptrtoint-of-alloca. ASan breaks
this assumption, and this results in local variables sometimes being
unavailable at -O0.

GlobalISel, SelectionDAG, and FastISel all do not appear to expect
dbg.declares to have a ptrtoint as an operand. This means that they do
not place entry block allocas in the usual side table reserved for local
variables available in the whole function scope. This isn't always a
problem, as LLVM can try to lower the dbg.declare to a DBG_VALUE, but
those DBG_VALUEs can get dropped for all the usual reasons DBG_VALUEs
get dropped. In the ObjC test case I'm looking at, the cause happens to
be that replaceDbgDeclare has hoisted dbg.declares into the entry
block, causing LiveDebugValues to "kill" the DBG_VALUEs because the
lexical dominance check fails.

To address this, I propose:

1. Have ASan (always) pass an alloca to dbg.declares (this patch). This is a narrow bugfix for -O0 debugging.

2. Make replaceDbgDeclare not move dbg.declares around. This should be a generic improvement for optimized debug info, as it would prevent the lexical dominance check in LiveDebugValues from killing as many variables.
   
   This means reverting llvm/r227544, which fixed an assertion failure (llvm.org/PR22386) but no longer seems to be necessary. I was able to complete a stage2 build with the revert in place.

rdar://54688991

[echristo]

This all sounds good to me (other than "hey can we start getting rid of these intrinsics?" :)

[rnk]

I can think of several cases where non-alloca values are passed to dbg.declare, so if you want to make the verifier change, there's much more work to do. It might be better just to make the backends handle these cases.

Here is the most common case, passing non-trivially copyable objects by value:

struct Foo {
  Foo();
  Foo(const Foo &o);
  ~Foo();
  int x;
};
int receiveFoo(Foo o) { return o.x; }

In this case, the C++ ABI passes the argument by address. Arguably, in this case, clang should do what it does for references: store the pointer into a stack slot and adjust the DIExpression to indicate that the stack slot contains an address, not a value.

The next most common would be byval:

struct Foo {
  int x;
  int ys[6];
};
int receiveFoo(Foo o) { return o.x; }

In this case, o is already in memory, and it would be unreasonable for clang to copy it into a local alloca, since it could be large. The verifier should probably allow dbg.declare to point to a byval argument, since they are just stack objects for most targets.

The next case would be inalloca, which is like byval, but there is a GEP in the way. Compile example 1 with --target=i686-windows-msvc, and it looks like so:

define dso_local i32 @"?receiveFoo@@YAHUFoo@@@Z"(<{ %struct.Foo }>* inalloca %0) #0 !dbg !8 {
entry:
  %o = getelementptr inbounds <{ %struct.Foo }>, <{ %struct.Foo }>* %0, i32 0, i32 0
  call void @llvm.dbg.declare(metadata %struct.Foo* %o, metadata !25, metadata !DIExpression()), !dbg !26

This needs to keep working, although I have tried to come up with alternative designs: https://github.com/rnk/llvm-project/blob/call-setup-docs/llvm/docs/CallSetup.md

The next case I can think of would be this, ASan.

Next, safestack, which I believe also replaces static allocas.

[eugenis]

I'd rather createAllocaForLayout returned an alloca pointer and intptr was applied to it later.
Also, would not this new assert fire in the case of fake stack?

[vsk]

Apologies, it looks like the various ISel implementations do have some way to handle dbg.declares which point to arguments. In light of that it doesn't seem desirable to ban non-alloca dbg.declares outright. There might be a narrower condition that's worth enforcing, but that'll take more thought and analysis.

I'll work on incorporating feedback from @eugenis.

[aprantl]

This part of the patch LGTM.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

3133

// The ptrtoint operation is transparent to the debug info and only confuses later passes.

llvm/test/DebugInfo/X86/asan_debug_info.ll

4

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.

[vsk]

Address review feedback.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

3118

@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.

3133

I've reworded this a bit, lmk if it's all right.

3137

@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.

llvm/test/DebugInfo/X86/asan_debug_info.ll

4

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.

[eugenis]

LGTM

[aprantl]

llvm/test/Instrumentation/AddressSanitizer/debug_info.ll

26

Ah here!

[vsk]

Looking into this some more, I no longer think it's all that important to stop replaceDbgDeclarefrom moving dbg.declares around. But I still think we should make the change as a cleanup, more context in: https://reviews.llvm.org/D74517

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

if (DoStackMalloc) {
     LocalStackBase = createPHI(IRB, NoFakeStack, AllocaValue, Term, FakeStack);
     IRB.CreateStore(LocalStackBase, LocalStackBaseAlloca);
     DIExprFlags |= DIExpression::DerefBefore;
   } else {
     // void *FakeStack = nullptr;
     // void *LocalStackBase = alloca(LocalStackSize);
     FakeStack = ConstantInt::get(IntptrTy, 0);
     LocalStackBase =
         DoDynamicAlloca ? createAllocaForLayout(IRB, L, true) : StaticAlloca;
     LocalStackBaseAlloca = LocalStackBase;
   }
 
+  // It shouldn't matter whether we pass an `alloca` or a `ptrtoint` as the
+  // dbg.declare address opereand, but passing a `ptrtoint` seems to confuse
+  // later passes and can result in dropped variable coverage in debug info.
+  Value *LocalStackBaseAllocaPtr =
+      isa<PtrToIntInst>(LocalStackBaseAlloca)
+          ? cast<PtrToIntInst>(LocalStackBaseAlloca)->getPointerOperand()
+          : LocalStackBaseAlloca;
+  assert(isa<AllocaInst>(LocalStackBaseAllocaPtr) &&
+         "Variable descriptions relative to ASan stack base will be dropped");
+
   // Replace Alloca instructions with base+offset.
   for (const auto &Desc : SVD) {
     AllocaInst *AI = Desc.AI;
-    replaceDbgDeclareForAlloca(AI, LocalStackBaseAlloca, DIB, DIExprFlags,
+    replaceDbgDeclareForAlloca(AI, LocalStackBaseAllocaPtr, DIB, DIExprFlags,
                                Desc.Offset);
     Value *NewAllocaPtr = IRB.CreateIntToPtr(
         IRB.CreateAdd(LocalStackBase, ConstantInt::get(IntptrTy, Desc.Offset)),
         AI->getType());
     AI->replaceAllUsesWith(NewAllocaPtr);
   }
 
   // The left-most redzone has enough space for at least 4 pointers.
   // Write the Magic value to redzone[0].
   Value *BasePlus0 = IRB.CreateIntToPtr(LocalStackBase, IntptrPtrTy);

llvm/test/DebugInfo/X86/asan_debug_info.ll

+; RUN: opt < %s -asan -asan-module -asan-use-after-return=0 -S | \
+; RUN:   llc -O0 -filetype=obj - -o - | \
+; RUN:   llvm-dwarfdump - | FileCheck %s
+
+; For this test case, ASan used to produce IR which resulted in the following
+; DWARF (at -O0):
+;
+;   DW_TAG_subprogram
+;     DW_AT_low_pc    (0x0000000000000000)
+;     DW_AT_high_pc   (0x00000000000000f1)
+;
+;     DW_TAG_variable
+;       DW_AT_location        (0x00000000
+;         [0x0000000000000014,  0x000000000000006d): DW_OP_breg0 RAX+32
+;         [0x000000000000006d,  0x00000000000000a4): DW_OP_breg7 RSP+16, DW_OP_deref, DW_OP_plus_uconst 0x20
+;         [0x00000000000000a6,  0x00000000000000ef): DW_OP_breg7 RSP+16, DW_OP_deref, DW_OP_plus_uconst 0x20)
+;
+; The DWARF produced for the original ObjC code that motivated this test case
+; was actually not as nice! In that example, the location list ranges didn't
+; intersect with the ranges of the parent lexical scope. But recreating that
+; exactly requires playing tricks to get LiveDebugValue's lexical dominance
+; check to kill a variable range early, and it isn't strictly necessary to show
+; the problem here.
+;
+; The problem is that we shouldn't get a location list at all. The instruction
+; selector should recognize that we have an "alloca" in the entry block, and
+; just make the fixed location available in the whole function. We now produce
+; the correct DWARF, namely:
+
+; CHECK: DW_TAG_variable
+; CHECK-NEXT:  DW_AT_location (DW_OP_breg7 RSP+32, DW_OP_plus_uconst 0x20)
+
+target triple = "x86_64-apple-macosx10.10.0"
+
+declare void @escape(i8**)
+
+; Function Attrs: sanitize_address
+define i8* @foo(i1 %cond) #0 !dbg !6 {
+entry:
+  %a1 = alloca i8*, !dbg !12
+  call void @escape(i8** %a1), !dbg !13
+  br i1 %cond, label %l1, label %l2, !dbg !14
+
+l1:                                               ; preds = %entry
+  ret i8* null, !dbg !15
+
+l2:                                               ; preds = %entry
+  call void @llvm.dbg.declare(metadata i8** %a1, metadata !11, metadata !DIExpression()), !dbg !16
+  %p = load i8*, i8** %a1, !dbg !16
+  ret i8* %p, !dbg !17
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+attributes #0 = { sanitize_address }
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!3, !4}
+!llvm.module.flags = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "redu.ll", directory: "/")
+!2 = !{}
+!3 = !{i32 6}
+!4 = !{i32 2}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+!7 = !DISubroutineType(types: !2)
+!8 = !{!11}
+!10 = !DIBasicType(name: "ty64", size: 64, encoding: DW_ATE_unsigned)
+!11 = !DILocalVariable(name: "2", scope: !6, file: !1, line: 5, type: !10)
+!12 = !DILocation(line: 1, column: 1, scope: !6)
+!13 = !DILocation(line: 2, column: 1, scope: !6)
+!14 = !DILocation(line: 3, column: 1, scope: !6)
+!15 = !DILocation(line: 4, column: 1, scope: !6)
+!16 = !DILocation(line: 5, column: 1, scope: !6)
+!17 = !DILocation(line: 6, column: 1, scope: !6)

llvm/test/Instrumentation/AddressSanitizer/debug_info.ll

entry:
   store volatile i32 %add, i32* %r, align 4, !dbg !14
   %1 = load i32, i32* %r, align 4, !dbg !15
   ret i32 %1, !dbg !15
 }
 
 ;   CHECK: define i32 @_Z3zzzi
 ;   CHECK: entry:
 ; Verify that llvm.dbg.declare calls are in the entry basic block.
+;   CHECK-NEXT: [[MyAlloca:%.*]] = alloca i8, i64 64
 ;   CHECK-NOT: %entry
-;   CHECK: call void @llvm.dbg.declare(metadata {{.*}}, metadata ![[ARG_ID:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, 32))
+; Note: these dbg.declares used to contain `ptrtoint` operands. The instruction
+; selector would then decline to put the variable in the MachineFunction side
+; table. Check that the dbg.declares have `alloca` operands.
+;   CHECK: call void @llvm.dbg.declare(metadata i8* [[MyAlloca]], metadata ![[ARG_ID:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, 32))
 ;   CHECK-NOT: %entry
-;   CHECK: call void @llvm.dbg.declare(metadata {{.*}}, metadata ![[VAR_ID:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, 48))
+;   CHECK: call void @llvm.dbg.declare(metadata i8* [[MyAlloca]], metadata ![[VAR_ID:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, 48))
 
 declare void @llvm.dbg.declare(metadata, metadata, metadata) nounwind readnone
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!17}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, producer: "clang version 3.3 (trunk 169314)", isOptimized: true, emissionKind: FullDebug, file: !16, enums: !1, retainedTypes: !1, globals: !1)
 !1 = !{}