[DebugInfo][Dexter] Unreachable line stepped onto after SimplifyCFG.

[CarlosAlbertoEnciso]

Summary

For the below test:

int main() {
  volatile int foo = 0;

  int beards = 0;
  bool cond = foo == 4;
  int bar = 0;
  if (cond)
    beards = 8;
  else
    beards = 4;

  volatile bool face = cond;

  return face ? beards : 0;
}

When in the debugger, on the line "if (cond)", and we step from the
'if' condition, it steps onto the unreached branch ("beards = 8;"), which is
misleading, as described by:

PR39187

[CarlosAlbertoEnciso]

Ping.

Thanks

[aprantl]

lib/Transforms/Utils/SimplifyCFG.cpp

1393

This comment explains *what* the code is doing but not *why*. Could you explain why this is being done in the comment?

test/CodeGen/X86/pr39187.ll

62

Since this test is only about DILocations, stripping out the dbg.value would make the the metadata a lot shorter.

[CarlosAlbertoEnciso]

@aprantl,

Thanks for your comments. I have updated the patch to address them.

lib/Transforms/Utils/SimplifyCFG.cpp

1393

Very good point. I have changed the comment to include the reason for that change.

test/CodeGen/X86/pr39187.ll

62

I have removed any unnecessary metadata.

[vsk]

lib/Transforms/Utils/SimplifyCFG.cpp

1379

A bit nitty-gritty, but: Can the instruction before the terminator be a debug info intrinsic?

If so, I think the line location you pick be different if you switch from -gline-tables-only versus -g.

[CarlosAlbertoEnciso]

lib/Transforms/Utils/SimplifyCFG.cpp

1379

Thanks for your review.

Changing the test case to:

int main() {
  volatile int foo = 0;

  int beards = 0;
  bool cond = foo == 4;
  if (cond)
    beards = 8;
  else
    beards = 4;

  volatile bool tmp;
  volatile bool *face = &tmp;
  *face = cond;
 
  return beards;
}

The IR will look like:

  ...
  %foo.0. = load volatile i32, i32* %foo, align 4, !dbg !28, !tbaa !23
  %cmp = icmp eq i32 %foo.0., 4, !dbg !28
  %frombool = zext i1 %cmp to i8, !dbg !28
  call void @llvm.dbg.value(metadata i8 %frombool, metadata !16, metadata !DIExpression()), !dbg !28
  br i1 %cmp, label %if.then, label %if.else, !dbg !29

if.then:                                          ; preds = %entry
  call void @llvm.dbg.value(metadata i32 8, metadata !15, metadata !DIExpression()), !dbg !27
  br label %if.end, !dbg !30

if.else:                                          ; preds = %entry
  call void @llvm.dbg.value(metadata i32 4, metadata !15, metadata !DIExpression()), !dbg !27
  br label %if.end

if.end:                                           ; preds = %if.else, %if.then
...

The instruction before the terminator is a debug info intrinsic.

[CarlosAlbertoEnciso]

lib/Transforms/Utils/SimplifyCFG.cpp

1379

If so, I think the line location you pick be different if you switch from -gline-tables-only versus -g.

IR for '-g'

...
%cmp = icmp eq i32 %foo.0., 4, !dbg !26
%frombool = zext i1 %cmp to i8, !dbg !26
call void @llvm.dbg.value(metadata i8 %frombool, metadata !16, metadata !DIExpression()), !dbg !26
br i1 %cmp, label %if.then, label %if.else, !dbg !27
...
!26 = !DILocation(line: 5, scope: !8)

IR for '-gline-tables-only'

...
%cmp = icmp eq i32 %foo.0., 4, !dbg !15
%frombool = zext i1 %cmp to i8, !dbg !15
br i1 %cmp, label %if.then, label %if.else, !dbg !16
...
!15 = !DILocation(line: 5, scope: !8)
...

InsertPts

  call void @llvm.dbg.value(metadata i8 %frombool, metadata !16, metadata !DIExpression()), !dbg !26

&&

  %frombool = zext i1 %cmp to i8, !dbg !15

For both options, the line location of the insertion point is the same, debug location !DILocation(line: 5, scope: !8).

[vsk]

lib/Transforms/Utils/SimplifyCFG.cpp

1379

I think it's still possible to construct cases where -g vs. -gline-tables-only produces different locations. What if you add int bar = 0 on the line after bool cond = foo == 4? I see this:

%6 = zext i1 %5 to i8, !dbg !31
call void @llvm.dbg.value(metadata i8 %6, metadata !15, metadata !DIExpression()), !dbg !31
call void @llvm.dbg.value(metadata i32 0, metadata !17, metadata !DIExpression()), !dbg !32
br i1 %5, label %7, label %8, !dbg !33

Where !32 and !31 are on different lines. Link in godbolt: https://godbolt.org/z/VswzLx

[CarlosAlbertoEnciso]

lib/Transforms/Utils/SimplifyCFG.cpp

1379

You are correct. By adding that line, the insertion points have different debug locations:

  call void @llvm.dbg.value(metadata i32 0, metadata !18, metadata !DIExpression()), !dbg !28
!28 = !DILocation(line: 6, scope: !8)

vs

  %frombool = zext i1 %cmp to i8, !dbg !15
!15 = !DILocation(line: 5, scope: !8)

[CarlosAlbertoEnciso]

lib/Transforms/Utils/SimplifyCFG.cpp

1379

These are the IRs for both options:

  ...
  %cmp = icmp eq i32 %foo.0., 4, !dbg !27
  %frombool = zext i1 %cmp to i8, !dbg !27
  call void @llvm.dbg.value(metadata i8 %frombool, metadata !16, metadata !DIExpression()), !dbg !27
  call void @llvm.dbg.value(metadata i32 0, metadata !18, metadata !DIExpression()), !dbg !28
  br i1 %cmp, label %if.then, label %if.else, !dbg !29
  ...
!27 = !DILocation(line: 5, scope: !8)
!28 = !DILocation(line: 6, scope: !8)

&

  ...
  %cmp = icmp eq i32 %foo.0., 4, !dbg !15
  %frombool = zext i1 %cmp to i8, !dbg !15
  br i1 %cmp, label %if.then, label %if.else, !dbg !16
  ...
!15 = !DILocation(line: 5, scope: !8)

What I can propose to have the same behavior for both options (-g vs -gline-tables-only), is for the case when the insertion point is a debug intrinsic, to find the previous instruction that is not a debug intrinsic (within the same BB), to be used as insertion point.

Create a function Instruction::getPrevNonDebugInstruction() to get that instruction. We have already Instruction::getNextNonDebugInstruction().

Doing that, then for the above IRs, for both cases, the insertion point will be:

%frombool = zext i1 %cmp to i8, !dbg !27

or

%frombool = zext i1 %cmp to i8, !dbg !15

that have the same debug location.

What to do if the BB contains only debug instructions?

[vsk]

lib/Transforms/Utils/SimplifyCFG.cpp

1379

Sgtm. Maybe use an empty location when no previous one can be found.

[CarlosAlbertoEnciso]

@vsk,

Thanks for your comments.

I have updated the patch to address the issue of using -g vs -gline-tables-only.
Included a test case for '-g' and a test case for '-gline-tables-only'.

[CarlosAlbertoEnciso]

lib/Transforms/Utils/SimplifyCFG.cpp

1379

Uploaded a new patch, that implements what we have discussed in order to cover both cases: -g and -gline-tables-only.

[vsk]

LGTM, although I don't think the 'pr39187-gline-tables-only.ll' adds any additional test coverage, so I'd suggest deleting it. The other test already tests the interesting case (dbg intrinsic prior to terminator with a different location).

[CarlosAlbertoEnciso]

@vsk: I have updated the patch to remove the test case pr39187-gline-tables-only.ll, as per your suggestion.

@aprantl: Do you any additional comments on the new patch?

Thanks very much to both of you for your invaluable feedback and reviews.

[aprantl]

Looks good, thanks!

[nikic]

This revision has caused https://bugs.llvm.org/show_bug.cgi?id=39807. One of the LLVM IR invariants is that inlinable calls between functions with debug info must have debugloc. However, this change may cause debugloc on invokes to be dropped.

[aprantl]

Ouch. Yeah it should use a line-0 location iff the instruction is a call. We can probably re-use code from getMergedLocation & friends for this if there isn't already a helper for it.

lib/Transforms/Utils/SimplifyCFG.cpp

for (PHINode &PN : Succ->phis()) {
 
       if (isa<ConstantExpr>(BB1V) && !isSafeToSpeculativelyExecute(BB1V))
         return Changed;
       if (isa<ConstantExpr>(BB2V) && !isSafeToSpeculativelyExecute(BB2V))
         return Changed;
     }
   }
 
+  // As the parent basic block terminator is a branch instruction which is
+  // removed at the end of the current transformation, use its previous
+  // instruction, as the reference insertion point.
+  Instruction *InsertPt = BIParent->size() > 1
+                              ? BIParent->getTerminator()->getPrevNode()
+                              : BIParent->getTerminator();
+
   // Okay, it is safe to hoist the terminator.
   Instruction *NT = I1->clone();
   BIParent->getInstList().insert(BI->getIterator(), NT);
   if (!NT->getType()->isVoidTy()) {
     I1->replaceAllUsesWith(NT);
     I2->replaceAllUsesWith(NT);
     NT->takeName(I1);
   }
 
+  // For the instruction being hoisted, we do not retain its original debug
+  // location (DILocation); instead use the value from the reference insertion
+  // point.
+  NT->setDebugLoc(InsertPt->getDebugLoc());
+
   IRBuilder<NoFolder> Builder(NT);
   // Hoisting one of the terminators from our successor is a great thing.
   // Unfortunately, the successors of the if/else blocks may have PHI nodes in
   // them.  If they do, all PHI entries for BB1/BB2 must agree for all PHI
   // nodes, so we insert select instruction to compute the final result.
   std::map<std::pair<Value *, Value *>, SelectInst *> InsertedSelects;
   for (BasicBlock *Succ : successors(BB1)) {
     for (PHINode &PN : Succ->phis()) {

test/CodeGen/X86/pr39187.ll

+; RUN: opt < %s -S -simplifycfg | FileCheck %s
+
+; SimplifyCFG can hoist any common code in the 'then' and 'else' blocks to
+; the 'if' basic block.
+;
+; For the special case, when hoisting the terminator instruction, its debug
+; location keep references to its basic block, causing the debug information
+; to become ambiguous. It causes the debugger to display unreached lines.
+
+; Change the debug location associated with the hoisted instruction, to
+; the debug location from the insertion point in the 'if' block.
+
+; IR generated with:
+; clang -S -g -gno-column-info -O2 -emit-llvm pr39187.cpp -o pr39187.ll -mllvm -opt-bisect-limit=10
+
+; // pr39187.cpp
+; int main() {
+;   volatile int foo = 0;
+;
+;   int beards = 0;
+;   if (foo == 4)
+;     beards = 8;
+;   else
+;     beards = 4;
+;
+;   return beards;
+; }
+
+; CHECK-LABEL: entry
+; CHECK:  %foo = alloca i32, align 4
+; CHECK:  %foo.0..sroa_cast = bitcast i32* %foo to i8*
+; CHECK:  store volatile i32 0, i32* %foo, align 4
+; CHECK:  %foo.0. = load volatile i32, i32* %foo, align 4
+; CHECK:  %cmp = icmp eq i32 %foo.0., 4, !dbg !13
+; CHECK:  %. = select i1 %cmp, i32 8, i32 4, !dbg !13
+
+; ModuleID = 'pr39187.cpp'
+source_filename = "pr39187.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 0, i32* %foo, align 4
+  %foo.0. = load volatile i32, i32* %foo, align 4
+  %cmp = icmp eq i32 %foo.0., 4, !dbg !21
+  br i1 %cmp, label %if.then, label %if.else, !dbg !23
+
+if.then:                                          ; preds = %entry
+  call void @llvm.dbg.value(metadata i32 8, metadata !14, metadata !DIExpression()), !dbg !20
+  br label %if.end, !dbg !24
+
+if.else:                                          ; preds = %entry
+  call void @llvm.dbg.value(metadata i32 4, metadata !14, metadata !DIExpression()), !dbg !20
+  br label %if.end
+
+if.end:                                           ; preds = %if.else, %if.then!19
+  %beards.0 = phi i32 [ 8, %if.then ], [ 4, %if.else ], !dbg !25
+  call void @llvm.dbg.value(metadata i32 %beards.0, metadata !14, metadata !DIExpression()), !dbg !20
+  ret i32 %beards.0
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #2
+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 344693)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "pr39187.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 344693)"}
+!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 = !{!14}
+!13 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !10)
+!14 = !DILocalVariable(name: "beards", scope: !7, file: !1, line: 4, type: !10)
+!20 = !DILocation(line: 4, scope: !7)
+!21 = !DILocation(line: 5, scope: !22)
+!22 = distinct !DILexicalBlock(scope: !7, file: !1, line: 5)
+!23 = !DILocation(line: 5, scope: !7)
+!24 = !DILocation(line: 6, scope: !22)
+!25 = !DILocation(line: 0, scope: !22)
+!26 = !DILocation(line: 11, scope: !7)