[DebugInfo] PR40628: Don't salvage load operations

[jmorse]

Summary

This patch disallows load insts from being salvaged. The reason why is PR40628 [0] -- LLVM is unable to determine where the validity of a debug expression containing DW_OP_deref ceases (i.e., where a store overwrites the intended data). It's also (I believe) unable to determine when hoisted/sunk memory instructions would invalidate a dbg.value. This has a large potential for misleading developers.

IMHO this isn't something that can be fixed without a lot of work, thus it's safer to turn off for now. If that's a contentious position though we can find a different way forwards. As mentioned in the PR, the effect on a clang-3.4 build is a 1.1% drop in variable location coverage and 1% drop in scope bytes coverage -- unfortunate, but IMHO not devastating. (I tend to put a lot more weight on avoiding inaccurate DebugInfo, as that diminishes trust in the accurate parts).

Summary of test changes:

• GVN test: -debugify option was added to test that instructions were salvaged in https://reviews.llvm.org/rL325063 . There's another file with -debugify checks added in that commit, so there's still coverage despite me deleting it in this test.
• DeadStoreElimination: tricky as memory operations are exactly what we're messing with. I've altered the test to have an intermediate addition that gets deleted by DSE as being redundant, and added a check that that's salvaged, which appears to have been the original aim of the test.
• InstCombine: this specifically tests for salvaging loads, so must be disabled. I've left a negative test there to fire if this feature is ever re-enabled in a safer condition.

The new test case is just a regression test so that there's a PR available if this behaviour comes back in the future.

[0] https://bugs.llvm.org/show_bug.cgi?id=40628

[aprantl]

This patch disallows load insts from being salvaged. The reason why is PR40628 [0] -- LLVM is unable to determine where the validity of a debug expression containing DW_OP_deref ceases (i.e., where a store overwrites the intended data).

Would it be feasible to inject a dbg_value(undef) at the end of the basic block or before the next store, whichever is earlier, instead?

[jmorse]

Hi,

In D57962#1391428, @aprantl wrote:

Would it be feasible to inject a dbg_value(undef) at the end of the basic block or before the next store, whichever is earlier, instead?

That'd totally work, and could even be done quite late in compilation. However I think the greater issue is that with DW_OP_derefs present it's difficult to determine when code movement in optimisation passes affects dbg.values. Consider this example, compiling "-O2 -g -fno-inline -fno-unroll-loops" with clang@r353515:

static int qux[] = { 1, 2, 3, 4 };

int
foo(int baz, int *out)
{
  int sum = 0;
  for (int i = 0; i < 4; i++) {
    int extra = *out;
    sum += qux[i] * baz;
    sum %= 4;
    *out = sum;
  }
  return sum;
}

int
main(int argc, char **argv)
{
  int out = 12;
  return foo(argc, &out);
}

(The assign to 'extra' is contrived but eliminating loads is something LLVM does all the time I believe). In this code, LICM promotes '*out' to an SSA register for the body of the loop. However, before LICM the dbg.value for 'extra' has its load operand folded into a DW_OP_deref, and points at the variable in 'main'. Stepping through this program in gdb, 'extra' always reads as '12', rather than any of the values computed in the loop. Invalidating the dbg.value at the next memory store wouldn't help, as the location specified by the dbg.value never holds the value we're interested in.

My main point here is that not only would LICM need to be taught that moving the store to '*out' could invalidate some dbg.values, determining which dbg.values are affected would involve digging into DIExpressions looking for dereferences, potentially through GEPs that were folded in too, possibly even requiring alias analysis on any dbg.value with a deref. Which (as far as I'm aware) is a reasonably large amount of code complexity and computation. DeadStoreElimination would need to behave similarly (a dbg.value might try to point at the memory of a store that's later eliminated). I'm not incredibly familiar with all of LLVMs passes, but there could be more to be taught.

There might be some kind of half-way house like inserting dbg.value(undef...) to terminate the range of dbg.values containing DW_OP_deref, at the point where any such pass changes visible stores, but I believe this would still involve teaching new things to passes.

[aprantl]

I think you convinced me. It doesn't make me happy that we have to drop this, but that's not your fault :-)

test/DebugInfo/Generic/pr40628.ll

13 ↗

(On Diff #185983)

Please add some kind of positive CHECK, too, otherwise this test would pass even if someone symlinked opt to /bin/true :-)

19 ↗

(On Diff #185983)

Is the tbaa data needed?

20 ↗

(On Diff #185983)

Could you get by with fewer distinct DILocations? i.e., would just attaching !23 everywhere work, too?

[aprantl]

I think you convinced me. It doesn't make me happy that we have to drop this, but that's not your fault :-)

lib/Transforms/Utils/Local.cpp

1704 ↗

(On Diff #185983)

Could you please add a comment here explaining why we don't salvage loads, so no-one comes back later with a great idea and adds it back in?

[jmorse]

Folded last comments into commit, but phab doesn't show those comments inline because of path differences it seems:

• Added comment warning against salvaging loads in salvageDebugInfoImpl
• Removed TBBA metadata and un-necessary DILocations
• New test case positively checks for an undef dbg.value corresponding to the optimised-out load.

llvm/trunk/lib/Transforms/Utils/Local.cpp

if (auto *CI = dyn_cast<CastInst>(&I)) {
     case Instruction::LShr:
       return applyOps({dwarf::DW_OP_constu, Val, dwarf::DW_OP_shr});
     case Instruction::AShr:
       return applyOps({dwarf::DW_OP_constu, Val, dwarf::DW_OP_shra});
     default:
       // TODO: Salvage constants from each kind of binop we know about.
       return nullptr;
     }
-  } else if (isa<LoadInst>(&I)) {
-    // Rewrite the load into DW_OP_deref.
-    return DIExpression::prepend(SrcDIExpr, DIExpression::WithDeref);
+    // *Not* to do: we should not attempt to salvage load instructions,
+    // because the validity and lifetime of a dbg.value containing
+    // DW_OP_deref becomes difficult to analyze. See PR40628 for examples.
   }
   return nullptr;
 }
 
 /// A replacement for a dbg.value expression.
 using DbgValReplacement = Optional<DIExpression *>;
 
 /// Point debug users of \p From to \p To using exprs given by \p RewriteExpr,

llvm/trunk/test/DebugInfo/Generic/pr40628.ll

+; RUN: opt -early-cse -S %s -o - | FileCheck %s
+
+; PR40628: The first load below is determined to be redundant by EarlyCSE.
+; During salvaging, the corresponding dbg.value could have a DW_OP_deref used
+; in its DIExpression to represent the redundant operation -- however LLVM
+; cannot currently determine that the subsequent store should terminate the
+; variables location range. A debugger would display zero for the "redundant"
+; variable after stepping onto the return instruction.
+
+; Test that the load being removed results in the corresponding dbg.value
+; being assigned the 'undef' value.
+
+; CHECK:      @foo
+; CHECK-NEXT: dbg.value(metadata i32 undef, metadata ![[DEADVAR:[0-9]+]],
+; CHECK-NEXT: load
+; CHECK-NEXT: dbg.value(metadata i32 %{{[0-9]+}}, metadata ![[LIVEVAR:[0-9]+]],
+; CHECK-NEXT: store
+; CHECK-NEXT: ret
+
+; CHECK:      ![[DEADVAR]] = !DILocalVariable(name: "redundant",
+; CHECK:      ![[LIVEVAR]] = !DILocalVariable(name: "loaded",
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define dso_local i32 @foo(i32*) !dbg !7 {
+  %2 = load i32, i32* %0, align 4, !dbg !23
+  call void @llvm.dbg.value(metadata i32 %2, metadata !16, metadata !DIExpression()), !dbg !23
+  %3 = load i32, i32* %0, align 4, !dbg !23
+  call void @llvm.dbg.value(metadata i32 %3, metadata !17, metadata !DIExpression()), !dbg !23
+  store i32 0, i32* %0, align 4, !dbg !23
+  ret i32 %3, !dbg !23
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "pr40628.c", directory: ".")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 2, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !11}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !10, size: 64)
+!12 = !{!16, !17}
+!16 = !DILocalVariable(name: "redundant", scope: !7, file: !1, line: 4, type: !10)
+!17 = !DILocalVariable(name: "loaded", scope: !7, file: !1, line: 5, type: !10)
+!23 = !DILocation(line: 4, column: 7, scope: !7)

llvm/trunk/test/Transforms/DeadStoreElimination/debuginfo.ll

 ; RUN: opt < %s -debugify -basicaa -dse -S | FileCheck %s
 
 target datalayout = "E-p:64:64:64-a0:0:8-f32:32:32-f64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-v64:64:64-v128:128:128"
 
 declare noalias i8* @malloc(i32)
 
 declare void @test_f()
 
-define i32* @test_salvage() {
+define i32* @test_salvage(i32 %arg) {
 ; Check that all four original local variables have their values preserved.
-; CHECK-LABEL: @test_salvage()
+; CHECK-LABEL: @test_salvage(
 ; CHECK-NEXT: malloc
 ; CHECK-NEXT: @llvm.dbg.value(metadata i8* %p, metadata ![[p:.*]], metadata !DIExpression())
 ; CHECK-NEXT: bitcast
 ; CHECK-NEXT: @llvm.dbg.value(metadata i32* %P, metadata ![[P:.*]], metadata !DIExpression())
-; CHECK-NEXT: @llvm.dbg.value(metadata i32* %P, metadata ![[DEAD:.*]], metadata !DIExpression(DW_OP_deref))
-; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %P, metadata ![[DEAD2:.*]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 1, DW_OP_stack_value))
+; CHECK-NEXT: @llvm.dbg.value(metadata i32 %arg, metadata ![[DEAD:.*]], metadata !DIExpression(DW_OP_plus_uconst, 1, DW_OP_stack_value))
 ; CHECK-NEXT: call void @test_f()
 ; CHECK-NEXT: store i32 0, i32* %P
 
   %p = tail call i8* @malloc(i32 4)
   %P = bitcast i8* %p to i32*
-  %DEAD = load i32, i32* %P
-  %DEAD2 = add i32 %DEAD, 1
-  store i32 %DEAD2, i32* %P
+  %DEAD = add i32 %arg, 1
+  store i32 %DEAD, i32* %P
   call void @test_f()
   store i32 0, i32* %P
   ret i32* %P
 }
 
 ; CHECK: ![[p]] = !DILocalVariable(name: "1"
 ; CHECK: ![[P]] = !DILocalVariable(name: "2"
 ; CHECK: ![[DEAD]] = !DILocalVariable(name: "3"
-; CHECK: ![[DEAD2]] = !DILocalVariable(name: "4"

llvm/trunk/test/Transforms/GVN/fence.ll

-; RUN: opt -S -debugify -basicaa -gvn < %s | FileCheck %s
+; RUN: opt -S -basicaa -gvn < %s | FileCheck %s
 
 @a = external constant i32
 ; We can value forward across the fence since we can (semantically) 
 ; reorder the following load before the fence.
 define i32 @test(i32* %addr.i) {
 ; CHECK-LABEL: @test
 ; CHECK: store
 ; CHECK: fence
 ; CHECK-NOT: load
 ; CHECK: ret
   store i32 5, i32* %addr.i, align 4
   fence release
   %a = load i32, i32* %addr.i, align 4
   ret i32 %a
 }
 
 ; Same as above
 define i32 @test2(i32* %addr.i) {
 ; CHECK-LABEL: @test2
-; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %addr.i, metadata [[var_a:![0-9]+]], metadata !DIExpression(DW_OP_deref))
 ; CHECK-NEXT: fence
 ; CHECK-NOT: load
-; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %addr.i, metadata [[var_a2:![0-9]+]], metadata !DIExpression(DW_OP_deref))
 ; CHECK: ret
   %a = load i32, i32* %addr.i, align 4
   fence release
   %a2 = load i32, i32* %addr.i, align 4
   %res = sub i32 %a, %a2
   ret i32 %res
 }
 

 ; lock() { 
 ;   do {
 ;     %res = cmpxchg %p, 0, 1, monotonic monotonic
 ;   } while(!%res.success)
 ;   fence acquire;
 ; }
 ; Given we chose to forward across the release fence, we clearly can't forward
 ; across the acquire fence as well.
-
-; CHECK: [[var_a]] = !DILocalVariable
-; CHECK-NEXT: [[var_a2]] = !DILocalVariable

llvm/trunk/test/Transforms/InstCombine/debuginfo-dce.ll

 
 ; ModuleID = '<stdin>'
 source_filename = "test.c"
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.12.0"
 
 %struct.entry = type { %struct.entry* }
 
+; This salvage can't currently occur safely (PR40628), however if/when that's
+; ever fixed, then this is definitely a piece of test coverage that should
+; be maintained.
 define void @salvage_load(%struct.entry** %queue) local_unnamed_addr #0 !dbg !14 {
 entry:
   %im_not_dead = alloca %struct.entry*
   %0 = load %struct.entry*, %struct.entry** %queue, align 8, !dbg !19
   %1 = load %struct.entry*, %struct.entry** %queue, align 8, !dbg !19
   call void @llvm.dbg.value(metadata %struct.entry* %1, metadata !18, metadata !20), !dbg !19
 ; CHECK: define void @salvage_load
 ; CHECK-NEXT: entry:
-; CHECK-NEXT: call void @llvm.dbg.value(metadata %struct.entry** %queue,
-; CHECK-SAME:                           metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 0))
+; CHECK-NOT: dbg.value
   store %struct.entry* %1, %struct.entry** %im_not_dead, align 8
   ret void, !dbg !21
 }
 
 define void @salvage_bitcast(%struct.entry* %queue) local_unnamed_addr #0 !dbg !22 {
 entry:
   %im_not_dead = alloca i8*
   %0 = bitcast %struct.entry* %queue to i8*, !dbg !23