[DebugInfo] Prevent error when updating location operands for a dbg.value

[StephenTozer]

Summary

This patch fixes the issue observed at https://bugs.chromium.org/p/chromium/issues/detail?id=1224338, mentioned in D91722.

The above issue occurs in CodeGenPrepare::fixupDbgValue, where we attempt to update the location operands of a dbg.value. The error occurs when the dbg.value uses a DIArgList that contains the same value multiple times; currently the update is performed by iterating over the location operands, and updating them within that loop by calling replaceVariableLocationOp, which invalidates the iterator; specifically, it continues to point to the old list of values, and so the loop attempts to replace the same value again when it isn't present in the list anymore, resulting in the observed error.

This has been fixed by first collecting the dbg.value's location operands into a set and then iterating over that, ensuring that we don't invalidate our loop iterator or attempt to replace a non-existing operand. Upon investigating, I discovered that the same issue exists in HWAddressSanitizer::sanitizeFunction, and have added the same fix as part of this patch.

[thakis]

I don't know this code so I can't comment on the patch, but it'd be good to add a test :)

[StephenTozer]

Add simple test to check for crashes in CodeGenPrepare.

[djtodoro]

This LGTM.

Do we need a test for the HWAddressSanitizer as well?

[StephenTozer]

In D105129#2855297, @djtodoro wrote:

Do we need a test for the HWAddressSanitizer as well?

Probably, I checked to see if there's an existing test that can be adapted but it looks as though this block for handling debug info specifically is untested at the moment.

[djtodoro]

In D105129#2855356, @StephenTozer wrote:

In D105129#2855297, @djtodoro wrote:

Do we need a test for the HWAddressSanitizer as well?

Probably, I checked to see if there's an existing test that can be adapted but it looks as though this block for handling debug info specifically is untested at the moment.

Oh, I see... If you don't mind, I'd vote for adding a test for it as well. :)

[StephenTozer]

Added test for the HWAddressSanitizer side of the fix.

[djtodoro]

Thanks!

llvm/lib/CodeGen/CodeGenPrepare.cpp

 // Some CGP optimizations may move or alter what's computed in a block. Check
 // whether a dbg.value intrinsic could be pointed at a more appropriate operand.
 bool CodeGenPrepare::fixupDbgValue(Instruction *I) {
   assert(isa<DbgValueInst>(I));
   DbgValueInst &DVI = *cast<DbgValueInst>(I);
 
   // Does this dbg.value refer to a sunk address calculation?
   bool AnyChange = false;
-  for (Value *Location : DVI.getValues()) {
+  SmallDenseSet<Value *> LocationOps(DVI.location_ops().begin(),
+                                     DVI.location_ops().end());
+  for (Value *Location : LocationOps) {
     WeakTrackingVH SunkAddrVH = SunkAddrs[Location];
     Value *SunkAddr = SunkAddrVH.pointsToAliveValue() ? SunkAddrVH : nullptr;
     if (SunkAddr) {
       // Point dbg.value at locally computed address, which should give the best
       // opportunity to be accurately lowered. This update may change the type
       // of pointer being referred to; however this makes no difference to
       // debugging information, and we can't generate bitcasts that may affect
       // codegen.

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

if (Size != AlignedSize) {
       AllocaToPaddedAllocaMap[AI] = NewAI;
     }
   }
 
   if (!AllocaToPaddedAllocaMap.empty()) {
     for (auto &BB : F) {
       for (auto &Inst : BB) {
         if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&Inst)) {
-          for (Value *V : DVI->location_ops()) {
+          SmallDenseSet<Value *> LocationOps(DVI->location_ops().begin(),
+                                             DVI->location_ops().end());
+          for (Value *V : LocationOps) {
             if (auto *AI = dyn_cast_or_null<AllocaInst>(V)) {
               if (auto *NewAI = AllocaToPaddedAllocaMap.lookup(AI))
                 DVI->replaceVariableLocationOp(V, NewAI);
             }
           }
         }
       }
     }

llvm/test/DebugInfo/X86/codegenprep-addrsink.ll

 ; CHECK:       dbg.value(metadata i8* %arith, metadata ![[DIVAR:[0-9]+]],
 ; CHECK-SAME:    metadata !DIExpression()
 ; CHECK-NEXT:  %[[CASTVAR:[0-9a-zA-Z]+]] = bitcast i32* %p to i8*
 ; CHECK-NEXT:  %[[GEPVAR:[0-9a-zA-Z]+]] = getelementptr i8, i8* %[[CASTVAR]],
 ; CHECK-SAME:                             i64 3
 ; CHECK-NEXT:  %loaded = load i8, i8* %[[GEPVAR]]
 ; CHECK-NEXT:  call void @llvm.dbg.value(metadata i8* %[[GEPVAR]],
 ; CHECK-SAME:                            metadata ![[DIVAR]],
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata !DIArgList(i8* %[[GEPVAR]],
+; CHECK-SAME:                            i8* %[[GEPVAR]]), metadata ![[DIVAR]],
   call void @llvm.dbg.value(metadata i8 *%arith, metadata !12, metadata !DIExpression()), !dbg !14
   %loaded = load i8, i8 *%arith
   call void @llvm.dbg.value(metadata i8 *%arith, metadata !12, metadata !DIExpression()), !dbg !14
+  call void @llvm.dbg.value(metadata !DIArgList(i8 *%arith, i8 *%arith), metadata !12, metadata !DIExpression()), !dbg !14
   ret i8 %loaded
 
 ret:
   ret i8 0
 }
 
 ; CHECK: ![[DIVAR]] = !DILocalVariable(name: "p",
 

llvm/test/Instrumentation/HWAddressSanitizer/alloca.ll

 ; Test alloca instrumentation.
 ;
 ; RUN: opt < %s -hwasan -hwasan-with-ifunc=1 -S | FileCheck %s --check-prefixes=CHECK,DYNAMIC-SHADOW,NO-UAR-TAGS
 ; RUN: opt < %s -hwasan -hwasan-mapping-offset=0 -S | FileCheck %s --check-prefixes=CHECK,ZERO-BASED-SHADOW,NO-UAR-TAGS
 ; RUN: opt < %s -hwasan -hwasan-with-ifunc=1 -hwasan-uar-retag-to-zero=0 -S | FileCheck %s --check-prefixes=CHECK,DYNAMIC-SHADOW,UAR-TAGS
 
 target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
 target triple = "aarch64--linux-android10000"
 
 declare void @use32(i32*)
 
-define void @test_alloca() sanitize_hwaddress {
+define void @test_alloca() sanitize_hwaddress !dbg !15 {
 ; CHECK-LABEL: @test_alloca(
 ; CHECK: %[[FP:[^ ]*]] = call i8* @llvm.frameaddress.p0i8(i32 0)
 ; CHECK: %[[A:[^ ]*]] = ptrtoint i8* %[[FP]] to i64
 ; CHECK: %[[B:[^ ]*]] = lshr i64 %[[A]], 20
 ; CHECK: %[[BASE_TAG:[^ ]*]] = xor i64 %[[A]], %[[B]]
 
 ; CHECK: %[[X:[^ ]*]] = alloca { i32, [12 x i8] }, align 16
 ; CHECK: %[[X_BC:[^ ]*]] = bitcast { i32, [12 x i8] }* %[[X]] to i32*
 ; CHECK: %[[X_TAG:[^ ]*]] = xor i64 %[[BASE_TAG]], 0
 ; CHECK: %[[X1:[^ ]*]] = ptrtoint i32* %[[X_BC]] to i64
 ; CHECK: %[[C:[^ ]*]] = shl i64 %[[X_TAG]], 56
 ; CHECK: %[[D:[^ ]*]] = or i64 %[[X1]], %[[C]]
 ; CHECK: %[[X_HWASAN:[^ ]*]] = inttoptr i64 %[[D]] to i32*
 
 ; CHECK: %[[X_TAG2:[^ ]*]] = trunc i64 %[[X_TAG]] to i8
 ; CHECK: %[[E:[^ ]*]] = ptrtoint i32* %[[X_BC]] to i64
 ; CHECK: %[[F:[^ ]*]] = lshr i64 %[[E]], 4
 ; DYNAMIC-SHADOW: %[[X_SHADOW:[^ ]*]] = getelementptr i8, i8* %.hwasan.shadow, i64 %[[F]]
 ; 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: 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*
 ; NO-UAR-TAGS: call void @llvm.memset.p0i8.i64(i8* align 1 %[[X_SHADOW2]], i8 0, i64 1, i1 false)
 ; UAR-TAGS: call void @llvm.memset.p0i8.i64(i8* align 1 %[[X_SHADOW2]], i8 %[[X_TAG_UAR]], i64 1, i1 false)
 ; CHECK: ret void
 
 
 entry:
   %x = alloca i32, align 4
-  call void @use32(i32* nonnull %x)
-  ret void
+  call void @llvm.dbg.value(metadata !DIArgList(i32* %x, i32* %x), metadata !22, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus, DW_OP_deref)), !dbg !21
+  call void @use32(i32* nonnull %x), !dbg !23
+  ret void, !dbg !24
 }
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!14}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 13.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "alloca.cpp", directory: "/")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!14 = !{!"clang version 13.0.0"}
+!15 = distinct !DISubprogram(name: "test_alloca", linkageName: "_Z11test_allocav", scope: !1, file: !1, line: 4, type: !16, scopeLine: 4, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{null}
+!19 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !20, size: 64)
+!20 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!21 = !DILocation(line: 0, scope: !15)
+!22 = !DILocalVariable(name: "x", scope: !15, file: !1, line: 5, type: !20)
+!23 = !DILocation(line: 7, column: 5, scope: !15)
+!24 = !DILocation(line: 8, column: 1, scope: !15)