[DebugInfo][Dexter] Speculated BB presents illegal variable value to debugger

[CarlosAlbertoEnciso]

Summary

For the below test:

int main() {
  volatile int foo = 4;
  int read = foo;
  int read1 = foo;

  int result = 0;
  if (read == 4) {
    result = read1 + 2;
  } else {
    result = read1 - 2;
  }

  return result;
}

Debug location information for 'result' is wrong with -O2.

When in the debugger, on the line "if (read == 4)", the value of "result"
is reported as '2', where it should be zero, as described by:

PR38763

[TWeaver]

couple of nitpicks.

lib/Transforms/Utils/Local.cpp

2528 ↗

(On Diff #165055)

I'd use for (auto &DII : DbgUsers) here. The type is obvious from the declaration of DbgUsers. This should make this line a little easier on the eyes.

2528 ↗

(On Diff #165055)

is DII the correct name for the iterator here? should this be DVI inline with the name of the type used in the declaration of DbgUsers?

That is : DVI for DbgVariableIntrinsic.

[aprantl]

test/CodeGen/X86/pr38763.ll

1 ↗

(On Diff #165055)

Sine you are testing SimplifyCFG, it would be much better to just run that one pass via opt and check the result immediately after it.

7 ↗

(On Diff #165055)

This is not a helpful description of the test. It would be easier to understand what the test is testing for if it said something like: "When SimplifyCFG folds two PHI instructions into a conditional instruction, the debug info becomes ambiguous..." (not sure if that is what actually happens but you get the idea.)

[aprantl]

This test confuses me. The function you are modifying is called FoldTwoEntryPHINode, but there is not a single PHI node in the input?

[aprantl]

It still isn't clear to me why you need to drop the debug info in the first place (instead of e.g., making it point to the select, or leave it on its operands) but I hope that that will become obvious once I see the actual testcase.

[probinson]

lib/Transforms/Utils/Local.cpp

2525 ↗

(On Diff #165055)

The function name and the comment in the .h file made me think this is removing the !dbg metadata from the specified instruction. But that's not at all what it does.

People generally don't use the term 'metadata' to refer to the debug intrinsic instructions. Maybe dropDebugUsers would be a better name.

[CarlosAlbertoEnciso]

lib/Transforms/Utils/Local.cpp

2525 ↗

(On Diff #165055)

Good observation. Changing the name to dropDebugUsers.

2528 ↗

(On Diff #165055)

Changing the loop to for (auto &DII : DbgUsers).

2528 ↗

(On Diff #165055)

I can see your point.

The SmallVector<DbgVariableIntrinsic *, 1> is used in quite few other places and it seems the DII is the standard name used for the iterator.

I would prefer to follow that name convention.

[dblaikie]

lib/Transforms/Utils/Local.cpp

2528 ↗

(On Diff #165055)

One tweak on this - if you're deducing a pointer with 'auto', LLVM style prefers to be explicit about that, so write "auto *X" rather than "auto &X" (assuming a reference isn't actually needed here, which it seems not)

[CarlosAlbertoEnciso]

In D51976#1232030, @aprantl wrote:

It still isn't clear to me why you need to drop the debug info in the first place (instead of e.g., making it point to the select, or leave it on its operands) but I hope that that will become obvious once I see the actual testcase.

Thanks for your feedback. I will review the test case and upload a new patch.

[CarlosAlbertoEnciso]

Address issues raised by the reviewers.

• Change the test case to use opt and the specific 'SimplifyCFG' pass.
• Rename 'dropDebugMetadata' as 'dropDebugUsers'.
• Use range-based for loop.

[CarlosAlbertoEnciso]

lib/Transforms/Utils/Local.cpp

2528 ↗

(On Diff #165055)

@dblaikie,

I am sorry I missed your tweak. I am uploading a patch that includes it.

test/CodeGen/X86/pr38763.ll

1 ↗

(On Diff #165055)

The new test case, run only the SimplifyCFG pass via opt.

7 ↗

(On Diff #165055)

Updated test description.

[CarlosAlbertoEnciso]

Address issues raised by the reviewers.

• Change the test case to use opt and the specific 'SimplifyCFG' pass.
• Rename 'dropDebugMetadata' as 'dropDebugUsers'.
• Use range-based for loop.
• Use pointer for the iterator.

[vsk]

Thanks, this is looking pretty close. A few comments inline.

include/llvm/Transforms/Utils/Local.h

446 ↗

(On Diff #165505)

Please omit 'machine' here, as MachineInstr and Instruction are distinct.

test/CodeGen/X86/pr38763.ll

36 ↗

(On Diff #165505)

For the test to be stringent, it needs to check that there these dbg.value's don't appear:

dbg.value(%add, !16, ...)
dbg.value(%sub, !16, ...)

One way to do that is to require exact next-line matches. E.g:

CHECK: %add = ...
CHECK-NOT: dbg.value(metadata i32 %add
CHECK-NEXT: %sub = ...
CHECK-NOT: dbg.value(metadata i32 %sub
CHECK-NEXT: %result.0 = select ...
CHECK-NEXT: d.v(%result.0, ...)

86 ↗

(On Diff #165505)

Are these lifetime markers needed to reproduce the issue?

93 ↗

(On Diff #165505)

Is the test still effective if you strip out all these attributes (i.e, remove '#0' etc from the file)? If so, doing that is a good way to make the test more maintainable.

[aprantl]

Having read the real testcase now, I understand why you are dropping the debug info now. I would like to point out that we could do better. If we extended llvm.dbg.value to take more than one LLVM SSA Value, then we could produce a DIExpression that selects either %add or %sub depending on the value of %cmp. Let me know if you are interested in implementing this, it should be relatively straight forward to do and would come in useful in all sorts of other situations, too.

[aprantl]

test/CodeGen/X86/pr38763.ll

113 ↗

(On Diff #165505)

You can strip out all variables that aren't necessary for the test, it makes it easier to read and maintain in the future.

122 ↗

(On Diff #165505)

You can further simplify the testcase by just using single (or just fewer) DILocation for everything.

[CarlosAlbertoEnciso]

In D51976#1235508, @aprantl wrote:

Having read the real testcase now, I understand why you are dropping the debug info now. I would like to point out that we could do better. If we extended llvm.dbg.value to take more than one LLVM SSA Value, then we could produce a DIExpression that selects either %add or %sub depending on the value of %cmp. Let me know if you are interested in implementing this, it should be relatively straight forward to do and would come in useful in all sorts of other situations, too.

That is a very good idea and I am interested in implementing it. A good opportunity to improve my knowledge in that area. I can see the associated benefits.

I would suggest if that is OK with you, to finish with the current issue. And then as a general improvement, to revisit the 'select' case as an independent issue, to implement what you have described. If you feel otherwise, I would proceed with the changes proposed to the 'llvm.dbg.value'.

In the meantime, I am updating the patch.

include/llvm/Transforms/Utils/Local.h

446 ↗

(On Diff #165505)

Removed 'machine'.

test/CodeGen/X86/pr38763.ll

36 ↗

(On Diff #165505)

Thanks for showing me how to improve the test. Adding the 'CHECK-NOT' entries.

86 ↗

(On Diff #165505)

Those markers are not needed. Removing them.

93 ↗

(On Diff #165505)

Able to reproduce the issue without those attributes.

113 ↗

(On Diff #165505)

Stripped out variables that are not necessary.

122 ↗

(On Diff #165505)

Remove unnecessary DILocation entries.

[CarlosAlbertoEnciso]

Address issues raised by the reviewers.

• Improve the test case by removing unnecessary variables and DILocation entries.
• Depending on @aprantl decision, a second step would be extending 'llvm.dbg.value' to deal with the 'select' case.

[aprantl]

In D51976#1236852, @CarlosAlbertoEnciso wrote:

In D51976#1235508, @aprantl wrote:

Having read the real testcase now, I understand why you are dropping the debug info now. I would like to point out that we could do better. If we extended llvm.dbg.value to take more than one LLVM SSA Value, then we could produce a DIExpression that selects either %add or %sub depending on the value of %cmp. Let me know if you are interested in implementing this, it should be relatively straight forward to do and would come in useful in all sorts of other situations, too.

That is a very good idea and I am interested in implementing it. A good opportunity to improve my knowledge in that area. I can see the associated benefits.

I would suggest if that is OK with you, to finish with the current issue. And then as a general improvement, to revisit the 'select' case as an independent issue, to implement what you have described. If you feel otherwise, I would proceed with the changes proposed to the 'llvm.dbg.value'.

Sure. Dropping the debug info is always the safe/correct option and will improve the status quo. It would be good to leave a comment in the that explains why we are dropping it and that properly handling it needs a missing feature in LLVM IR.

In the meantime, I am updating the patch.

[probinson]

In D51976#1236852, @CarlosAlbertoEnciso wrote:

In D51976#1235508, @aprantl wrote:

Having read the real testcase now, I understand why you are dropping the debug info now. I would like to point out that we could do better. If we extended llvm.dbg.value to take more than one LLVM SSA Value, then we could produce a DIExpression that selects either %add or %sub depending on the value of %cmp. Let me know if you are interested in implementing this, it should be relatively straight forward to do and would come in useful in all sorts of other situations, too.

That is a very good idea and I am interested in implementing it. A good opportunity to improve my knowledge in that area. I can see the associated benefits.

Hi Carlos, I had this exact conversation with Tom Weaver last week. Extending llvm.dbg.value to handle more complex expressions will allow huge steps forward in making variable values visible (if not always modifiable). He was thinking more in terms of loop induction variables however. Maybe you two can collaborate on the IR part of this.

[aprantl]

I also sketched an implementation over in https://bugs.llvm.org/show_bug.cgi?id=38763.

[CarlosAlbertoEnciso]

@aprantl and @probinson,

Based on your views and comments, I am happy to "change" the current fix towards implementing the extension of 'llvm.dbg.value', as it seems it would be more beneficial for a better debug information.

The added comments (https://bugs.llvm.org/show_bug.cgi?id=38763) by @aprantl, are a good starting point.

@vsk, how do you feel about moving the current proposed patch, to be in terms of extending the 'llvm.dbg.value'?

[vsk]

In D51976#1237210, @CarlosAlbertoEnciso wrote:

@aprantl and @probinson,

Based on your views and comments, I am happy to "change" the current fix towards implementing the extension of 'llvm.dbg.value', as it seems it would be more beneficial for a better debug information.

The added comments (https://bugs.llvm.org/show_bug.cgi?id=38763) by @aprantl, are a good starting point.

@vsk, how do you feel about moving the current proposed patch, to be in terms of extending the 'llvm.dbg.value'?

I expect that extending dbg.value in this way will be fairly involved. There are a couple questions worth thinking about, imo:

• Would the extra SSA values need to be constants (or live in registers)? If not, how will their locations be represented in a TAG_variable entry?
• What's the expected memory size overhead of adding an extra operand to dbg.value? Does it need to be counterbalanced by introducing dbg.value.simple() [for the case where there is an empty DIExpression/extended SSA stack]?
• How will LiveDebugVariables determine where to insert DBG_VALUEs into the MI stream when there are extra SSA values? How do you ensure that the live intervals of those values aren't disjoint?

I think we should separate out the task of investigating these questions (& maybe collecting some numbers with a prototype) from driving this patch forward.

My preference would be to land this patch as-is.

test/CodeGen/X86/pr38763.ll

1 ↗

(On Diff #165765)

Do you need to pipe in the input ('< %s') here? I assumed that opt would write its output to %s.ll otherwise.

[probinson]

In D51976#1237392, @vsk wrote:

I expect that extending dbg.value in this way will be fairly involved. There are a couple questions worth thinking about, imo:

• Would the extra SSA values need to be constants (or live in registers)? If not, how will their locations be represented in a TAG_variable entry?
• What's the expected memory size overhead of adding an extra operand to dbg.value? Does it need to be counterbalanced by introducing dbg.value.simple() [for the case where there is an empty DIExpression/extended SSA stack]?

Metadata already knows how to do lists, I'd suggest that rather than a variable-length argument list to the intrinsic (if there aren't other considerations that make this problematic).

• How will LiveDebugVariables determine where to insert DBG_VALUEs into the MI stream when there are extra SSA values? How do you ensure that the live intervals of those values aren't disjoint?

The values that feed into a DIExpression must dominate the instruction referencing the DIExpression (he said casually waving his hands).
Whether that's easy or hard to enforce is a different question.

I think we should separate out the task of investigating these questions (& maybe collecting some numbers with a prototype) from driving this patch forward.

And the design discussion ought to be on llvm-dev rather than buried in a code review....

My preference would be to land this patch as-is.

+1

[CarlosAlbertoEnciso]

test/CodeGen/X86/pr38763.ll

1 ↗

(On Diff #165765)

The pipe is not required. opt write its output to the standard output.
I had look at the test cases that use 'opt' and there are 2 ways to invoke it:

(1) RUN: opt <options> < %s

and

(2) RUN: opt <options> %s

The most common format is (1). Changing to use that format.

[CarlosAlbertoEnciso]

The general preference seems to land this patch as-is.

This patch addresses the outstanding issues raised by the reviewers.

• Use pipe in the opt command line.

[probinson]

test/CodeGen/X86/pr38763.ll

1 ↗

(On Diff #165765)

FTR we prefer < %s because that way the input path doesn't appear in the output. The input path can end up causing incorrect CHECK matches, depending on how the checks are written.

[CarlosAlbertoEnciso]

test/CodeGen/X86/pr38763.ll

1 ↗

(On Diff #165765)

Hi Paul,

Thanks for the explanation about the preference on < %s.

[vsk]

Thanks, LGTM. @aprantl?

[CarlosAlbertoEnciso]

Thanks very much to all the reviewers for your valuable comments and suggestions.

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

 
 /// Copy a range metadata node to a new load instruction.
 ///
 /// This handles mapping it to nonnull metadata if the new load is a pointer
 /// load instead of an integer load and the range doesn't cover null.
 void copyRangeMetadata(const DataLayout &DL, const LoadInst &OldLI, MDNode *N,
                        LoadInst &NewLI);
 
+/// Remove the debug intrinsic instructions for the given instruction.
+void dropDebugUsers(Instruction &I);
+
 //===----------------------------------------------------------------------===//
 //  Intrinsic pattern matching
 //
 
 /// Try to match a bswap or bitreverse idiom.
 ///
 /// If an idiom is matched, an intrinsic call is inserted before \c I. Any added
 /// instructions are returned in \c InsertedInsts. They will all have been added

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

void llvm::copyRangeMetadata(const DataLayout &DL, const LoadInst &OldLI,
 
   unsigned BitWidth = DL.getIndexTypeSizeInBits(NewTy);
   if (!getConstantRangeFromMetadata(*N).contains(APInt(BitWidth, 0))) {
     MDNode *NN = MDNode::get(OldLI.getContext(), None);
     NewLI.setMetadata(LLVMContext::MD_nonnull, NN);
   }
 }
 
+void llvm::dropDebugUsers(Instruction &I) {
+  SmallVector<DbgVariableIntrinsic *, 1> DbgUsers;
+  findDbgUsers(DbgUsers, &I);
+  for (auto *DII : DbgUsers)
+    DII->eraseFromParent();
+}
+
 namespace {
 
 /// A potential constituent of a bitreverse or bswap expression. See
 /// collectBitParts for a fuller explanation.
 struct BitPart {
   BitPart(Value *P, unsigned BW) : Provider(P) {
     Provenance.resize(BW);
   }

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

static bool FoldTwoEntryPHINode(PHINode *PN, const TargetTransformInfo &TTI,
   // If we can still promote the PHI nodes after this gauntlet of tests,
   // do all of the PHI's now.
   Instruction *InsertPt = DomBlock->getTerminator();
   IRBuilder<NoFolder> Builder(InsertPt);
 
   // Move all 'aggressive' instructions, which are defined in the
   // conditional parts of the if's up to the dominating block.
   if (IfBlock1) {
-    for (auto &I : *IfBlock1)
+    for (auto &I : *IfBlock1) {
       I.dropUnknownNonDebugMetadata();
+      dropDebugUsers(I);
+    }
     DomBlock->getInstList().splice(InsertPt->getIterator(),
                                    IfBlock1->getInstList(), IfBlock1->begin(),
                                    IfBlock1->getTerminator()->getIterator());
   }
   if (IfBlock2) {
-    for (auto &I : *IfBlock2)
+    for (auto &I : *IfBlock2) {
       I.dropUnknownNonDebugMetadata();
+      dropDebugUsers(I);
+    }
     DomBlock->getInstList().splice(InsertPt->getIterator(),
                                    IfBlock2->getInstList(), IfBlock2->begin(),
                                    IfBlock2->getTerminator()->getIterator());
   }
 
   while (PHINode *PN = dyn_cast<PHINode>(BB->begin())) {
     // Change the PHI node into a select instruction.
     Value *TrueVal = PN->getIncomingValue(PN->getIncomingBlock(0) == IfFalse);

llvm/trunk/test/CodeGen/X86/pr38763.ll

+; RUN: opt < %s -S -simplifycfg | FileCheck %s
+
+; When SimplifyCFG changes the PHI node into a select instruction, the debug
+; information becomes ambiguous. It causes the debugger to display wrong
+; variable value.
+;
+; When in the debugger, on the line "if (read == 4)", the value of "result"
+; is reported as '2', where it should be zero.
+
+; IR generated with:
+; clang -S -g -O2 -emit-llvm pr38763.cpp -o pr38763.ll -mllvm -opt-bisect-limit=10
+
+; // pr38763.cpp
+; int main() {
+;   volatile int foo = 4;
+;   int read = foo;
+;   int read1 = foo;
+; 
+;   int result = 0;
+;   if (read == 4) {
+;     result = read1 + 2;
+;   } else {
+;     result = read1 - 2;
+;   }
+; 
+;   return result;
+; }
+
+; Remove the '@llvm.dbg.value' associated with 'result' for the true/false
+; branches, as they becomes ambiguous.
+
+; CHECK-LABEL: entry
+; CHECK:  %cmp = icmp eq i32 %foo.0., 4
+; CHECK:  %add = add nsw i32 %foo.0.4, 2, !dbg !18
+; CHECK-NOT: @llvm.dbg.value(metadata i32 %add
+; CHECK:  %sub = add nsw i32 %foo.0.4, -2, !dbg !21
+; CHECK-NOT: @llvm.dbg.value(metadata i32 %sub
+; CHECK:  %result.0 = select i1 %cmp, i32 %add, i32 %sub
+; CHECK:  call void @llvm.dbg.value(metadata i32 %result.0, metadata !12, metadata !DIExpression()), !dbg !17
+
+; ModuleID = 'pr38763.cpp'
+source_filename = "pr38763.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+; Function Attrs: norecurse nounwind uwtable
+define dso_local i32 @main() local_unnamed_addr #0 !dbg !7 {
+entry:
+  %foo = alloca i32, align 4
+  %foo.0..sroa_cast = bitcast i32* %foo to i8*
+  store volatile i32 4, i32* %foo, align 4, !tbaa !19
+  %foo.0. = load volatile i32, i32* %foo, align 4
+  %foo.0.4 = load volatile i32, i32* %foo, align 4
+  call void @llvm.dbg.value(metadata i32 0, metadata !16, metadata !DIExpression()), !dbg !27
+  %cmp = icmp eq i32 %foo.0., 4
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:                                          ; preds = %entry
+  %add = add nsw i32 %foo.0.4, 2, !dbg !31
+  call void @llvm.dbg.value(metadata i32 %add, metadata !16, metadata !DIExpression()), !dbg !27
+  br label %if.end
+
+if.else:                                          ; preds = %entry
+  %sub = add nsw i32 %foo.0.4, -2, !dbg !34
+  call void @llvm.dbg.value(metadata i32 %sub, metadata !16, metadata !DIExpression()), !dbg !27
+  br label %if.end
+
+if.end:                                           ; preds = %if.else, %if.then
+  %result.0 = phi i32 [ %add, %if.then ], [ %sub, %if.else ]
+  call void @llvm.dbg.value(metadata i32 %result.0, metadata !16, metadata !DIExpression()), !dbg !27
+  ret i32 %result.0
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #2
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 8.0.0 (trunk 342209)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "pr38763.cpp", 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 version 8.0.0 (trunk 342209)"}
+!7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!16}
+!16 = !DILocalVariable(name: "result", scope: !7, file: !1, line: 6, type: !10)
+!19 = !{!20, !20, i64 0}
+!20 = !{!"int", !21, i64 0}
+!21 = !{!"omnipotent char", !22, i64 0}
+!22 = !{!"Simple C++ TBAA"}
+!27 = !DILocation(line: 6, column: 7, scope: !7)
+!29 = distinct !DILexicalBlock(scope: !7, file: !1, line: 7, column: 7)
+!31 = !DILocation(line: 8, column: 20, scope: !32)
+!32 = distinct !DILexicalBlock(scope: !29, file: !1, line: 7, column: 18)
+!34 = !DILocation(line: 10, column: 20, scope: !35)
+!35 = distinct !DILexicalBlock(scope: !29, file: !1, line: 9, column: 10)