[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 ↗

(On Diff #170062)

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 ↗

(On Diff #170062)

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 ↗

(On Diff #170062)

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

test/CodeGen/X86/pr39187.ll

62 ↗

(On Diff #170062)

I have removed any unnecessary metadata.

[vsk]

lib/Transforms/Utils/SimplifyCFG.cpp

1379 ↗

(On Diff #171680)

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 ↗

(On Diff #171680)

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 ↗

(On Diff #171680)

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 ↗

(On Diff #171680)

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 ↗

(On Diff #171680)

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 ↗

(On Diff #171680)

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 ↗

(On Diff #171680)

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 ↗

(On Diff #171680)

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.

llvm/trunk/include/llvm/IR/Instruction.h

public:
   /// Return a pointer to the next non-debug instruction in the same basic
   /// block as 'this', or nullptr if no such instruction exists.
   const Instruction *getNextNonDebugInstruction() const;
   Instruction *getNextNonDebugInstruction() {
     return const_cast<Instruction *>(
         static_cast<const Instruction *>(this)->getNextNonDebugInstruction());
   }
 
+  /// Return a pointer to the previous non-debug instruction in the same basic
+  /// block as 'this', or nullptr if no such instruction exists.
+  const Instruction *getPrevNonDebugInstruction() const;
+  Instruction *getPrevNonDebugInstruction() {
+    return const_cast<Instruction *>(
+        static_cast<const Instruction *>(this)->getPrevNonDebugInstruction());
+  }
+
   /// Create a copy of 'this' instruction that is identical in all ways except
   /// the following:
   ///   * The instruction has no parent
   ///   * The instruction has no name
   ///
   Instruction *clone() const;
 
   /// Return true if the specified instruction is exactly identical to the

llvm/trunk/lib/IR/Instruction.cpp

 
 const Instruction *Instruction::getNextNonDebugInstruction() const {
   for (const Instruction *I = getNextNode(); I; I = I->getNextNode())
     if (!isa<DbgInfoIntrinsic>(I))
       return I;
   return nullptr;
 }
 
+const Instruction *Instruction::getPrevNonDebugInstruction() const {
+  for (const Instruction *I = getPrevNode(); I; I = I->getPrevNode())
+    if (!isa<DbgInfoIntrinsic>(I))
+      return I;
+  return nullptr;
+}
+
 bool Instruction::isAssociative() const {
   unsigned Opcode = getOpcode();
   if (isAssociative(Opcode))
     return true;
 
   switch (Opcode) {
   case FMul:
   case FAdd:

llvm/trunk/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
+  // non-debug instruction, as the reference insertion point, which will
+  // provide the debug location for the instruction being hoisted. For BBs
+  // with only debug instructions, use an empty debug location.
+  Instruction *InsertPt =
+      BIParent->getTerminator()->getPrevNonDebugInstruction();
+
   // 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);
   }
 
+  // The instruction NT being hoisted, is the terminator for the true branch,
+  // with debug location (DILocation) within that branch. We can't retain
+  // its original debug location value, otherwise 'select' instructions that
+  // are created from any PHI nodes, will take its debug location, giving
+  // the impression that those 'select' instructions are in the true branch,
+  // causing incorrect stepping, affecting the debug experience.
+  NT->setDebugLoc(InsertPt ? InsertPt->getDebugLoc() : DebugLoc());
+
   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()) {

llvm/trunk/test/CodeGen/X86/pr39187-g.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.
+
+; The insertion point is the previous non-debug instruction before the
+; terminator in the parent basic block of the hoisted instruction.
+
+; IR with '-g':
+;
+;  [...]
+;  %frombool = zext i1 %cmp to i8, !dbg !26
+;  call void @llvm.dbg.value(metadata i8 %frombool, metadata !15, metadata !DIExpression()), !dbg !26
+;  call void @llvm.dbg.value(metadata i32 0, metadata !17, metadata !DIExpression()), !dbg !27
+;  br i1 %cmp, label %if.then, label %if.else
+;  [...]
+;
+; Insertion point is: %frombool = zext i1 %cmp to i8, !dbg !26
+
+; IR generated with:
+; clang -S -g -gno-column-info -O2 -emit-llvm pr39187.cpp -o pr39187-g.ll -mllvm -opt-bisect-limit=10
+
+; // pr39187.cpp
+; 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;
+; }
+
+; CHECK-LABEL: entry
+; CHECK:  %foo = alloca i32, align 4
+; CHECK:  %face = alloca i8, align 1
+; 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, !dbg !16
+; CHECK:  %cmp = icmp eq i32 %foo.0., 4, !dbg !16
+; CHECK:  %frombool = zext i1 %cmp to i8, !dbg !16
+; CHECK:  call void @llvm.dbg.value(metadata i8 %frombool, metadata !13, metadata !DIExpression()), !dbg !16
+; CHECK:  call void @llvm.dbg.value(metadata i32 0, metadata !15, metadata !DIExpression()), !dbg !17
+; CHECK:  %. = select i1 %cmp, i32 8, i32 4, !dbg !16
+
+; 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
+  %face = alloca i8, align 1
+  %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, !dbg !26
+  %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 !15, metadata !DIExpression()), !dbg !26
+  call void @llvm.dbg.value(metadata i32 0, metadata !17, metadata !DIExpression()), !dbg !27
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:                                          ; preds = %entry
+  call void @llvm.dbg.value(metadata i32 8, metadata !14, metadata !DIExpression()), !dbg !25
+  br label %if.end
+
+if.else:                                          ; preds = %entry
+  call void @llvm.dbg.value(metadata i32 4, metadata !14, metadata !DIExpression()), !dbg !25
+  br label %if.end
+
+if.end:                                           ; preds = %if.else, %if.then
+  %beards.0 = phi i32 [ 8, %if.then ], [ 4, %if.else ]
+  store volatile i8 %frombool, i8* %face, align 1
+  %face.0. = load volatile i8, i8* %face, align 1
+  %0 = and i8 %face.0., 1
+  %tobool3 = icmp eq i8 %0, 0
+  %cond4 = select i1 %tobool3, i32 0, i32 %beards.0
+  ret i32 %cond4
+}
+
+; 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 346301)", 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 346301)"}
+!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, !15, !17}
+!14 = !DILocalVariable(name: "beards", scope: !7, file: !1, line: 4, type: !10)
+!15 = !DILocalVariable(name: "cond", scope: !7, file: !1, line: 5, type: !16)
+!16 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!17 = !DILocalVariable(name: "bar", scope: !7, file: !1, line: 6, type: !10)
+!25 = !DILocation(line: 4, scope: !7)
+!26 = !DILocation(line: 5, scope: !7)
+!27 = !DILocation(line: 6, scope: !7)