[Local] Do not move around dbg.declares during replaceDbgDeclare

[vsk]

Summary

replaceDbgDeclare is used to update the descriptions of stack variables
when they are moved (e.g. by ASan or SafeStack). A side effect of
replaceDbgDeclare is that it moves dbg.declares around in the
instruction stream (typically by hoisting them into the entry block).
This behavior was introduced in llvm/r227544 to fix an assertion failure
(llvm.org/PR22386), but no longer appears to be necessary.

Hoisting a dbg.declare generally does not create problems. Usually,
dbg.declare either describes an argument or an alloca in the entry
block, and backends have special handling to emit locations for these.
In optimized builds, LowerDbgDeclare places dbg.values in the right
spots regardless of where the dbg.declare is. And no one uses
replaceDbgDeclare to handle things like VLAs.

However, there doesn't seem to be a positive case for moving
dbg.declares around anymore, and this reordering can get in the way of
understanding other bugs. I propose getting rid of it.

Testing: stage2 RelWithDebInfo sanitized build, check-llvm

rdar://59397340

[eugenis]

I think the point of moving dbg.declare was to make sure it is dominated by the new expression. Ex. in ASan we place it immediately after the base pointer definition.
If you are sure this condition holds in all call sites, then LGTM.

[aprantl]

This looks reasonable with @eugenis caveat taken care of.

[vsk]

In D74517#1873432, @eugenis wrote:

I think the point of moving dbg.declare was to make sure it is dominated by the new expression. Ex. in ASan we place it immediately after the base pointer definition.
If you are sure this condition holds in all call sites, then LGTM.

@eugenis IIUC, post-ASan, each dbg.declare(<alloca>) should be dominated by <alloca>, even if none of the dbg.declares are hoisted. I added the following assert to test this while building a stage2 llc:

   // Replace Alloca instructions with base+offset.
+  DominatorTree DT(F);
   for (const auto &Desc : SVD) {
     AllocaInst *AI = Desc.AI;
+    assert(DT.dominates(cast<Instruction>(LocalStackBaseAllocaPtr), AI));
     replaceDbgDeclare(AI, LocalStackBaseAllocaPtr, DIB, DIExprFlags,
                       Desc.Offset);

If the input IR has no debug use-before-def, the post-ASan IR should look good as well, because ASan either does not move AI or it hoists it (from StaticAllocasToMoveUp). Does that answer your question?

[eugenis]

OK, sounds good.

llvm/include/llvm/Transforms/Utils/Local.h

 /// Finds the debug info intrinsics describing a value.
 void findDbgUsers(SmallVectorImpl<DbgVariableIntrinsic *> &DbgInsts, Value *V);
 
 /// Replaces llvm.dbg.declare instruction when the address it
 /// describes is replaced with a new value. If Deref is true, an
 /// additional DW_OP_deref is prepended to the expression. If Offset
 /// is non-zero, a constant displacement is added to the expression
 /// (between the optional Deref operations). Offset can be negative.
-bool replaceDbgDeclare(Value *Address, Value *NewAddress,
-                       Instruction *InsertBefore, DIBuilder &Builder,
+bool replaceDbgDeclare(Value *Address, Value *NewAddress, DIBuilder &Builder,
                        uint8_t DIExprFlags, int Offset);
 
-/// Replaces llvm.dbg.declare instruction when the alloca it describes
-/// is replaced with a new value. If Deref is true, an additional
-/// DW_OP_deref is prepended to the expression. If Offset is non-zero,
-/// a constant displacement is added to the expression (between the
-/// optional Deref operations). Offset can be negative. The new
-/// llvm.dbg.declare is inserted immediately after AI.
-bool replaceDbgDeclareForAlloca(AllocaInst *AI, Value *NewAllocaAddress,
-                                DIBuilder &Builder, uint8_t DIExprFlags,
-                                int Offset);
-
 /// Replaces multiple llvm.dbg.value instructions when the alloca it describes
 /// is replaced with a new value. If Offset is non-zero, a constant displacement
 /// is added to the expression (after the mandatory Deref). Offset can be
 /// negative. New llvm.dbg.value instructions are inserted at the locations of
 /// the instructions they replace.
 void replaceDbgValueForAlloca(AllocaInst *AI, Value *NewAllocaAddress,
                               DIBuilder &Builder, int Offset = 0);
 

llvm/lib/CodeGen/SafeStack.cpp

if (Size == 0)
       Size = 1; // Don't create zero-sized stack objects.
 
     Value *Off = IRB.CreateGEP(Int8Ty, BasePointer, // BasePointer is i8*
                                ConstantInt::get(Int32Ty, -Offset));
     Value *NewArg = IRB.CreateBitCast(Off, Arg->getType(),
                                      Arg->getName() + ".unsafe-byval");
 
     // Replace alloc with the new location.
-    replaceDbgDeclare(Arg, BasePointer, BasePointer->getNextNode(), DIB,
-                      DIExpression::ApplyOffset, -Offset);
+    replaceDbgDeclare(Arg, BasePointer, DIB, DIExpression::ApplyOffset,
+                      -Offset);
     Arg->replaceAllUsesWith(NewArg);
     IRB.SetInsertPoint(cast<Instruction>(NewArg)->getNextNode());
     IRB.CreateMemCpy(Off, Align, Arg, Arg->getParamAlign(), Size);
   }
 
   // Allocate space for every unsafe static AllocaInst on the unsafe stack.
   for (AllocaInst *AI : StaticAllocas) {
     IRB.SetInsertPoint(AI);
     unsigned Offset = SSL.getObjectOffset(AI);
 
-    replaceDbgDeclareForAlloca(AI, BasePointer, DIB, DIExpression::ApplyOffset,
-                               -Offset);
+    replaceDbgDeclare(AI, BasePointer, DIB, DIExpression::ApplyOffset, -Offset);
     replaceDbgValueForAlloca(AI, BasePointer, DIB, -Offset);
 
     // Replace uses of the alloca with the new location.
     // Insert address calculation close to each use to work around PR27844.
     std::string Name = std::string(AI->getName()) + ".unsafe";
     while (!AI->use_empty()) {
       Use &U = *AI->use_begin();
       Instruction *User = cast<Instruction>(U.getUser());
for (AllocaInst *AI : DynamicAllocas) {
     IRB.CreateStore(NewTop, UnsafeStackPtr);
     if (DynamicTop)
       IRB.CreateStore(NewTop, DynamicTop);
 
     Value *NewAI = IRB.CreatePointerCast(NewTop, AI->getType());
     if (AI->hasName() && isa<Instruction>(NewAI))
       NewAI->takeName(AI);
 
-    replaceDbgDeclareForAlloca(AI, NewAI, DIB, DIExpression::ApplyOffset, 0);
+    replaceDbgDeclare(AI, NewAI, DIB, DIExpression::ApplyOffset, 0);
     AI->replaceAllUsesWith(NewAI);
     AI->eraseFromParent();
   }
 
   if (!DynamicAllocas.empty()) {
     // Now go through the instructions again, replacing stacksave/stackrestore.
     for (inst_iterator It = inst_begin(&F), Ie = inst_end(&F); It != Ie;) {
       Instruction *I = &*(It++);

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

Value *LocalStackBaseAllocaPtr =
           ? 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, LocalStackBaseAllocaPtr, DIB, DIExprFlags,
-                               Desc.Offset);
+    replaceDbgDeclare(AI, LocalStackBaseAllocaPtr, DIB, DIExprFlags,
     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].

llvm/lib/Transforms/Utils/InlineFunction.cpp

for (BasicBlock::iterator I = FirstNewBlock->begin(),
       }
 
       // Transfer all of the allocas over in a block.  Using splice means
       // that the instructions aren't removed from the symbol table, then
       // reinserted.
       Caller->getEntryBlock().getInstList().splice(
           InsertPoint, FirstNewBlock->getInstList(), AI->getIterator(), I);
     }
-    // Move any dbg.declares describing the allocas into the entry basic block.
-    DIBuilder DIB(*Caller->getParent());
-    for (auto &AI : IFI.StaticAllocas)
-      replaceDbgDeclareForAlloca(AI, AI, DIB, DIExpression::ApplyOffset, 0);
   }
 
   SmallVector<Value*,4> VarArgsToForward;
   SmallVector<AttributeSet, 4> VarArgsAttrs;
   for (unsigned i = CalledFunc->getFunctionType()->getNumParams();
        i < CS.getNumArgOperands(); i++) {
     VarArgsToForward.push_back(CS.getArgOperand(i));
     VarArgsAttrs.push_back(CS.getAttributes().getParamAttributes(i));

llvm/lib/Transforms/Utils/Local.cpp

void llvm::findDbgUsers(SmallVectorImpl<DbgVariableIntrinsic *> &DbgUsers,
   if (auto *L = LocalAsMetadata::getIfExists(V))
     if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L))
       for (User *U : MDV->users())
         if (DbgVariableIntrinsic *DII = dyn_cast<DbgVariableIntrinsic>(U))
           DbgUsers.push_back(DII);
 }
 
 bool llvm::replaceDbgDeclare(Value *Address, Value *NewAddress,
-                             Instruction *InsertBefore, DIBuilder &Builder,
-                             uint8_t DIExprFlags, int Offset) {
+                             DIBuilder &Builder, uint8_t DIExprFlags,
+                             int Offset) {
   auto DbgAddrs = FindDbgAddrUses(Address);
   for (DbgVariableIntrinsic *DII : DbgAddrs) {
     DebugLoc Loc = DII->getDebugLoc();
     auto *DIVar = DII->getVariable();
     auto *DIExpr = DII->getExpression();
     assert(DIVar && "Missing variable");
     DIExpr = DIExpression::prepend(DIExpr, DIExprFlags, Offset);
-    // Insert llvm.dbg.declare immediately before InsertBefore, and remove old
+    // Insert llvm.dbg.declare immediately before DII, and remove old
     // llvm.dbg.declare.
-    Builder.insertDeclare(NewAddress, DIVar, DIExpr, Loc, InsertBefore);
-    if (DII == InsertBefore)
-      InsertBefore = InsertBefore->getNextNode();
+    Builder.insertDeclare(NewAddress, DIVar, DIExpr, Loc, DII);
     DII->eraseFromParent();
   }
   return !DbgAddrs.empty();
 }
 
-bool llvm::replaceDbgDeclareForAlloca(AllocaInst *AI, Value *NewAllocaAddress,
-                                      DIBuilder &Builder, uint8_t DIExprFlags,
-                                      int Offset) {
-  return replaceDbgDeclare(AI, NewAllocaAddress, AI->getNextNode(), Builder,
-                           DIExprFlags, Offset);
-}
-
 static void replaceOneDbgValueForAlloca(DbgValueInst *DVI, Value *NewAddress,
                                         DIBuilder &Builder, int Offset) {
   DebugLoc Loc = DVI->getDebugLoc();
   auto *DIVar = DVI->getVariable();
   auto *DIExpr = DVI->getExpression();
   assert(DIVar && "Missing variable");
 
   // This is an alloca-based llvm.dbg.value. The first thing it should do with

llvm/test/Instrumentation/AddressSanitizer/debug_info.ll

entry:
   %0 = load i32, i32* %p.addr, align 4, !dbg !14
   %add = add nsw i32 %0, 1, !dbg !14
   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: [[MyAlloca:%.*]] = alloca i8, i64 64
 ; 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 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)

llvm/test/Instrumentation/AddressSanitizer/local_stack_base.ll

entry:
   %i.addr = alloca i32, align 4
   store i32 %i, i32* %i.addr, align 4
   call void @llvm.dbg.declare(metadata i32* %i.addr, metadata !12, metadata !DIExpression()), !dbg !13
 
   ; CHECK: %asan_local_stack_base = alloca i64
   ; CHECK: %[[ALLOCA:.*]] = ptrtoint i8* %MyAlloca to i64
   ; CHECK: %[[PHI:.*]] = phi i64 {{.*}} %[[ALLOCA]],
   ; CHECK: store i64 %[[PHI]], i64* %asan_local_stack_base
-  ; CHECK: call void @llvm.dbg.declare(metadata i64* %asan_local_stack_base, metadata !12, metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 32)), !dbg !13
+; CHECK: call void @llvm.dbg.declare(metadata i64* %asan_local_stack_base, metadata [[VAR_I:![0-9]+]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 32)), !dbg [[LOC_I:![0-9]+]]
   %0 = load i32, i32* %i.addr, align 4, !dbg !14
   %add = add nsw i32 %0, 2, !dbg !15
   ret i32 %add, !dbg !16
 }
 
+; CHECK: [[SCOPE:![0-9]+]] = distinct !DISubprogram(name: "foo"
+; CHECK: [[VAR_I]] = !DILocalVariable(name: "i", arg: 1, scope: [[SCOPE]]
+; CHECK: [[LOC_I]] = !DILocation(line: 1, column: 13, scope: [[SCOPE]]
+
 ; Function Attrs: nounwind readnone speculatable
 declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
 
 attributes #0 = { noinline nounwind optnone sanitize_address ssp uwtable }
 attributes #1 = { nounwind readnone speculatable }
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4, !5, !6}

llvm/test/Transforms/Inline/alloca-dbgdeclare-merge.ll

-; Test that alloca merging in the inliner places dbg.declare calls immediately
-; after the merged alloca. Not at the end of the entry BB, and definitely not
-; before the alloca.
+; Test that alloca merging in the inliner places dbg.declare calls after the
+; merged alloca, but does not otherwise reorder them.
 ;
 ; clang -g -S -emit-llvm -Xclang -disable-llvm-optzns
 ;
 ;__attribute__((always_inline)) void f() {
 ;  char aaa[100];
 ;  aaa[10] = 1;
 ;}
 ;
 ;__attribute__((always_inline)) void g() {
 ;  char bbb[100];
 ;  bbb[20] = 1;
 ;}
 ;
 ;void h() {
 ;  f();
 ;  g();
 ;}
 ;
 ; RUN: opt -always-inline -S < %s | FileCheck %s
+
+; FIXME: Why does the dbg.declare for "aaa" occur later in @h than the
+; dbg.declare for "bbb"? I'd expect the opposite, given @f is inlined earlier.
 ;
 ; CHECK:      define void @h()
 ; CHECK-NEXT: entry:
 ; CHECK-NEXT:   %[[AI:.*]] = alloca [100 x i8]
-; CHECK-NEXT:   call void @llvm.dbg.declare(metadata [100 x i8]* %[[AI]],
-; CHECK-NEXT:   call void @llvm.dbg.declare(metadata [100 x i8]* %[[AI]],
+; CHECK-NEXT:   call void @llvm.dbg.declare(metadata [100 x i8]* %[[AI]], metadata [[BBB:![0-9]+]]
+; CHECK-NEXT:   bitcast
+; CHECK-NEXT:   llvm.lifetime.start
+; CHECK-NEXT:   call void @llvm.dbg.declare(metadata [100 x i8]* %[[AI]], metadata [[AAA:![0-9]+]]
 
+; CHECK: [[AAA]] = !DILocalVariable(name: "aaa"
+; CHECK: [[BBB]] = !DILocalVariable(name: "bbb"
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; Function Attrs: alwaysinline nounwind uwtable
 define void @f() #0 !dbg !4 {
 entry:
   %aaa = alloca [100 x i8], align 16

llvm/test/Transforms/Inline/alloca-dbgdeclare.ll

 
 ; Function Attrs: nounwind
 define void @_Z3fn4v() #0 !dbg !22 {
 entry:
 ; Test that the dbg.declare is moved together with the alloca.
 ; CHECK: define void @_Z3fn5v()
 ; CHECK-NEXT: entry:
 ; CHECK-NEXT:   %agg.tmp.sroa.3.i = alloca [20 x i8], align 4
+; CHECK-NEXT:   br label %while.body
+; CHECK:      while.body:
+; CHECK-NEXT:   bitcast
+; CHECK-NEXT:   llvm.lifetime.start
 ; CHECK-NEXT:   call void @llvm.dbg.declare(metadata [20 x i8]* %agg.tmp.sroa.3.i,
   %agg.tmp.sroa.3 = alloca [20 x i8], align 4
   tail call void @llvm.dbg.declare(metadata [20 x i8]* %agg.tmp.sroa.3, metadata !25, metadata !30), !dbg !31
   %agg.tmp.sroa.0.0.copyload = load i32, i32* getelementptr inbounds (%struct.A, %struct.A* @b, i64 0, i32 0), align 8, !dbg !33
   tail call void @llvm.dbg.value(metadata i32 %agg.tmp.sroa.0.0.copyload, metadata !25, metadata !34), !dbg !31
   %agg.tmp.sroa.3.0..sroa_idx = getelementptr inbounds [20 x i8], [20 x i8]* %agg.tmp.sroa.3, i64 0, i64 0, !dbg !33
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %agg.tmp.sroa.3.0..sroa_idx, i8* align 4 getelementptr (i8, i8* bitcast (%struct.A* @b to i8*), i64 4), i64 20, i1 false), !dbg !33
   tail call void @llvm.dbg.declare(metadata %struct.A* undef, metadata !25, metadata !35) #0, !dbg !31

llvm/test/Transforms/Inline/inline_dbg_declare.ll

 declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
 
 ; CHECK: define void @bar
 
 ; Function Attrs: nounwind
 define void @bar(float* %dst) #0 !dbg !9 {
 entry:
 
-; CHECK: [[x_addr_i:%[a-zA-Z0-9.]+]] = alloca float, align 4
+; CHECK: [[x_addr_i:%.+]] = alloca float, align 4
+; CHECK: store float {{.*}}, float* [[x_addr_i]]
 ; CHECK-NEXT: void @llvm.dbg.declare(metadata float* [[x_addr_i]], metadata [[m23:![0-9]+]], metadata !DIExpression()), !dbg [[m24:![0-9]+]]
 
   %dst.addr = alloca float*, align 4
   store float* %dst, float** %dst.addr, align 4
   call void @llvm.dbg.declare(metadata float** %dst.addr, metadata !20, metadata !17), !dbg !21
   %0 = load float*, float** %dst.addr, align 4, !dbg !22
   %arrayidx = getelementptr inbounds float, float* %0, i32 0, !dbg !22
   %1 = load float, float* %arrayidx, align 4, !dbg !22

llvm/unittests/Transforms/Utils/LocalTest.cpp

TEST(Local, ReplaceDbgDeclare) {
   auto *AI = dyn_cast<AllocaInst>(Inst);
   ASSERT_TRUE(AI);
   Inst = Inst->getNextNode()->getNextNode();
   ASSERT_TRUE(Inst);
   auto *DII = dyn_cast<DbgDeclareInst>(Inst);
   ASSERT_TRUE(DII);
   Value *NewBase = Constant::getNullValue(Type::getInt32PtrTy(C));
   DIBuilder DIB(*M);
-  replaceDbgDeclare(AI, NewBase, DII, DIB, DIExpression::ApplyOffset, 0);
+  replaceDbgDeclare(AI, NewBase, DIB, DIExpression::ApplyOffset, 0);
 
   // There should be exactly two dbg.declares.
   int Declares = 0;
   for (const Instruction &I : F->front())
     if (isa<DbgDeclareInst>(I))
       Declares++;
   EXPECT_EQ(2, Declares);
 }