[DebugInfo] Fix incorrect handling of debug values with duplicate location operands

[StephenTozer]

Summary

This patch fixes several errors found by searching for code that does not correctly handle dbg.values with duplicate operands, such as DIArgList(i32 %a, i32 %a), following from the more specific errors found in D105129.

There are 2 types of errors fixed in this patch:

• The errors in llvm::salvageDebugInfoForDbgValues and HWAddressSanitizer::instrumentStack are caused by applying an update to the dbg.value for only the first instance of a duplicated operand, when the update needs to be applied for each instance.
• The errors in HWAddressSanitizer::sanitizeFunction and AArch64StackTagging::runOnFunction are caused by tracking values in the dbg.value that need to be replaced later, and then trying to replace the duplicated operand multiple times when it should only be replaced once.

In the longer term it will probably be easier to prevent duplicated operands from existing at all, but doing so requires further changes to prevent a DIArgList and its associated DIExpression from falling out of sync with each other. This can mostly be handled in DbgVariableIntrinsic and DIArgList::handleUpdatedOperand, but there are exceptions that may require a design change (such as joining the DIArgList and DIExpression in a single MDNode) to be handled in a generalized manner.

[djtodoro]

Basically LGTM.

llvm/lib/Transforms/Utils/Local.cpp

1741–1752

Can we add a comment here by explaining what is the intention of the code below?

llvm/test/CodeGen/AArch64/stack-tagging-dbg.ll

41

nit: This is unused.

[StephenTozer]

Addressed comments: Added comment to explain salvage changes, removed unused metadata entry.

llvm/lib/Target/AArch64/AArch64StackTagging.cpp

for (BasicBlock::iterator IT = BB.begin(); IT != BB.end(); ++IT) {
         Allocas[AI].AI = AI;
         Allocas[AI].OldAI = AI;
         continue;
       }
 
       if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(I)) {
         for (Value *V : DVI->location_ops())
           if (auto *AI = dyn_cast_or_null<AllocaInst>(V))
+            if (Allocas[AI].DbgVariableIntrinsics.empty() ||
+                Allocas[AI].DbgVariableIntrinsics.back() != DVI)
               Allocas[AI].DbgVariableIntrinsics.push_back(DVI);
         continue;
       }
 
       auto *II = dyn_cast<IntrinsicInst>(I);
       if (II && (II->getIntrinsicID() == Intrinsic::lifetime_start ||
                  II->getIntrinsicID() == Intrinsic::lifetime_end)) {
         AllocaInst *AI = findAllocaForValue(II->getArgOperand(1));
         if (!AI) {

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

AI->replaceUsesWithIf(Replacement,
                           [AILong](Use &U) { return U.getUser() != AILong; });
 
     for (auto *DDI : AllocaDbgMap.lookup(AI)) {
       // Prepend "tag_offset, N" to the dwarf expression.
       // Tag offset logically applies to the alloca pointer, and it makes sense
       // to put it at the beginning of the expression.
       SmallVector<uint64_t, 8> NewOps = {dwarf::DW_OP_LLVM_tag_offset,
                                          retagMask(N)};
-      auto Locations = DDI->location_ops();
-      unsigned LocNo = std::distance(Locations.begin(), find(Locations, AI));
-      DDI->setExpression(
-          DIExpression::appendOpsToArg(DDI->getExpression(), NewOps, LocNo));
+      for (size_t LocNo = 0; LocNo < DDI->getNumVariableLocationOps(); ++LocNo)
+        if (DDI->getVariableLocationOp(LocNo) == AI)
+          DDI->setExpression(DIExpression::appendOpsToArg(DDI->getExpression(),
+                                                          NewOps, LocNo));
     }
 
     size_t Size = getAllocaSizeInBytes(*AI);
     tagAlloca(IRB, AI, Tag, Size);
 
     for (auto RI : RetVec) {
       IRB.SetInsertPoint(RI);
 
for (auto &Inst : BB) {
             AllocasToInstrument.push_back(AI);
           continue;
         }
 
       if (isa<ReturnInst>(Inst) || isa<ResumeInst>(Inst) ||
           isa<CleanupReturnInst>(Inst))
         RetVec.push_back(&Inst);
 
-      if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&Inst))
-        for (Value *V : DVI->location_ops())
+      if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&Inst)) {
+        for (Value *V : DVI->location_ops()) {
           if (auto *Alloca = dyn_cast_or_null<AllocaInst>(V))
+            if (!AllocaDbgMap.count(Alloca) ||
+                AllocaDbgMap[Alloca].back() != DVI)
               AllocaDbgMap[Alloca].push_back(DVI);
+        }
+      }
 
       if (InstrumentLandingPads && isa<LandingPadInst>(Inst))
         LandingPadVec.push_back(&Inst);
 
       getInterestingMemoryOperands(&Inst, OperandsToInstrument);
 
       if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(&Inst))
         IntrinToInstrument.push_back(MI);

llvm/lib/Transforms/Utils/Local.cpp

for (auto *DII : DbgUsers) {
     // Do not add DW_OP_stack_value for DbgDeclare and DbgAddr, because they
     // are implicitly pointing out the value as a DWARF memory location
     // description.
     bool StackValue = isa<DbgValueInst>(DII);
     auto DIILocation = DII->location_ops();
     assert(
         is_contained(DIILocation, &I) &&
         "DbgVariableIntrinsic must use salvaged instruction as its location");
-    unsigned LocNo = std::distance(DIILocation.begin(), find(DIILocation, &I));
     SmallVector<Value *, 4> AdditionalValues;
-    DIExpression *SalvagedExpr = salvageDebugInfoImpl(
-        I, DII->getExpression(), StackValue, LocNo, AdditionalValues);
-
+    // `I` may appear more than once in DII's location ops, and each use of `I`
+    // must be updated in the DIExpression and potentially have additional
+    // values added; thus we call salvageDebugInfoImpl for each `I` instance in
+    // DIILocation.
+    DIExpression *SalvagedExpr = DII->getExpression();
+    auto LocItr = find(DIILocation, &I);
+    while (SalvagedExpr && LocItr != DIILocation.end()) {
+      unsigned LocNo = std::distance(DIILocation.begin(), LocItr);
+      SalvagedExpr = salvageDebugInfoImpl(I, SalvagedExpr, StackValue, LocNo,
+                                          AdditionalValues);
+      LocItr = std::find(++LocItr, DIILocation.end(), &I);
+    }
     // salvageDebugInfoImpl should fail on examining the first element of
     // DbgUsers, or none of them.
     if (!SalvagedExpr)
       break;
 
     DII->replaceVariableLocationOp(&I, I.getOperand(0));
     if (AdditionalValues.empty()) {
       DII->setExpression(SalvagedExpr);

llvm/test/CodeGen/AArch64/stack-tagging-dbg.ll

 ; RUN: opt < %s -aarch64-stack-tagging -S -o - | FileCheck %s
 
 target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
 target triple = "aarch64--linux-android"
 
 declare void @use32(i32*)
 declare void @llvm.dbg.declare(metadata, metadata, metadata) nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) nounwind readnone speculatable
 
 ; Debug intrinsics use the new alloca directly, not through a GEP or a tagp.
 define void @DbgIntrinsics() sanitize_memtag {
 entry:
   %x = alloca i32, align 4
   call void @llvm.dbg.declare(metadata i32* %x, metadata !6, metadata !DIExpression()), !dbg !10
+  call void @llvm.dbg.value(metadata !DIArgList(i32* %x, i32* %x), metadata !6, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus)), !dbg !10
   store i32 42, i32* %x, align 4
   call void @use32(i32* %x)
   ret void
 }
 
 ; CHECK-LABEL: define void @DbgIntrinsics(
 ; CHECK:  [[X:%.*]] = alloca { i32, [12 x i8] }, align 16
 ; CHECK:  call void @llvm.dbg.declare(metadata { i32, [12 x i8] }* [[X]],
+; CHECK:  call void @llvm.dbg.value(metadata !DIArgList({ i32, [12 x i8] }* [[X]], { i32, [12 x i8] }* [[X]])
 
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!8, !9}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 9.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
 !1 = !DIFile(filename: "stack-tagging.cc", directory: "/tmp")
 !2 = !{}
 !3 = distinct !DISubprogram(name: "DbgIntrinsics", linkageName: "DbgIntrinsics", scope: !1, file: !1, line: 3, type: !4, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
 !4 = !DISubroutineType(types: !5)
 !5 = !{null}
 !6 = !DILocalVariable(name: "x", scope: !3, file: !1, line: 4, type: !7)
 !7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
 !8 = !{i32 2, !"Dwarf Version", i32 4}
 !9 = !{i32 2, !"Debug Info Version", i32 3}
 !10 = !DILocation(line: 1, column: 2, scope: !3)

llvm/test/DebugInfo/salvage-duplicate-values.ll

+; XFAIL: *
+; RUN: opt %s -dce -S | FileCheck %s
+
+; Tests the results of salvaging variadic dbg.values that use the same SSA value
+; multiple times.
+
+; CHECK: call void @llvm.dbg.value(metadata !DIArgList(i32 %a, i32 %a),
+; CHECK-SAME: ![[VAR_C:[0-9]+]],
+; CHECK-SAME: !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_plus_uconst, 5, DW_OP_LLVM_arg, 1, DW_OP_plus_uconst, 5, DW_OP_plus, DW_OP_stack_value))
+
+; CHECK: call void @llvm.dbg.value(metadata !DIArgList(i32 %a, i32 %a, i32 %b, i32 %b),
+; CHECK-SAME: ![[VAR_C]],
+; CHECK-SAME: !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 2, DW_OP_plus, DW_OP_LLVM_arg, 1, DW_OP_LLVM_arg, 3, DW_OP_plus, DW_OP_plus, DW_OP_stack_value))
+
+; CHECK: ![[VAR_C]] = !DILocalVariable(name: "c"
+
+define i32 @"?multiply@@YAHHH@Z"(i32 %a, i32 %b) !dbg !8 {
+entry:
+  %add1 = add nsw i32 %a, 5, !dbg !15
+  %add2 = add nsw i32 %a, %b, !dbg !15
+  call void @llvm.dbg.value(metadata !DIArgList(i32 %add1, i32 %add1), metadata !16, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus, DW_OP_stack_value)), !dbg !13
+  call void @llvm.dbg.value(metadata !DIArgList(i32 %add2, i32 %add2), metadata !16, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus, DW_OP_stack_value)), !dbg !13
+  %mul = mul nsw i32 %a, %b, !dbg !17
+  ret i32 %mul, !dbg !17
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 11.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "/")
+!2 = !{}
+!3 = !{i32 2, !"CodeView", i32 1}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 2}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = !{!"clang version 11.0.0"}
+!8 = distinct !DISubprogram(name: "multiply", linkageName: "?multiply@@YAHHH@Z", scope: !1, file: !1, line: 1, type: !9, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11, !11, !11}
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !DILocalVariable(name: "b", arg: 2, scope: !8, file: !1, line: 1, type: !11)
+!13 = !DILocation(line: 0, scope: !8)
+!14 = !DILocalVariable(name: "a", arg: 1, scope: !8, file: !1, line: 1, type: !11)
+!15 = !DILocation(line: 2, scope: !8)
+!16 = !DILocalVariable(name: "c", scope: !8, file: !1, line: 2, type: !11)
+!17 = !DILocation(line: 3, scope: !8)

llvm/test/Instrumentation/HWAddressSanitizer/alloca.ll

 ; ZERO-BASED-SHADOW: %[[X_SHADOW:[^ ]*]] = inttoptr i64 %[[F]] to i8*
 ; CHECK: %[[X_SHADOW_GEP:[^ ]*]] = getelementptr i8, i8* %[[X_SHADOW]], i32 0
 ; CHECK: store i8 4, i8* %[[X_SHADOW_GEP]]
 ; CHECK: %[[X_I8:[^ ]*]] = bitcast i32* %[[X_BC]] to i8*
 ; CHECK: %[[X_I8_GEP:[^ ]*]] = getelementptr i8, i8* %[[X_I8]], i32 15
 ; CHECK: store i8 %[[X_TAG2]], i8* %[[X_I8_GEP]]
 ; CHECK: call void @llvm.dbg.value(
 ; CHECK-SAME: metadata !DIArgList(i32* %[[X_BC]], i32* %[[X_BC]])
+; CHECK-SAME: metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_tag_offset, 0, DW_OP_LLVM_arg, 1, DW_OP_LLVM_tag_offset, 0,
 ; CHECK: call void @use32(i32* nonnull %[[X_HWASAN]])
 
 ; UAR-TAGS: %[[BASE_TAG_COMPL:[^ ]*]] = xor i64 %[[BASE_TAG]], 255
 ; UAR-TAGS: %[[X_TAG_UAR:[^ ]*]] = trunc i64 %[[BASE_TAG_COMPL]] to i8
 ; CHECK: %[[E2:[^ ]*]] = ptrtoint i32* %[[X_BC]] to i64
 ; CHECK: %[[F2:[^ ]*]] = lshr i64 %[[E2]], 4
 ; DYNAMIC-SHADOW: %[[X_SHADOW2:[^ ]*]] = getelementptr i8, i8* %.hwasan.shadow, i64 %[[F2]]
 ; ZERO-BASED-SHADOW: %[[X_SHADOW2:[^ ]*]] = inttoptr i64 %[[F2]] to i8*