This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by CarlosAlbertoEnciso on Oct 18 2018, 3:34 AM.

Details

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

Diff Detail

Event Timeline

aprantl added inline comments.Oct 29 2018, 8:41 AM
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.

@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.

CarlosAlbertoEnciso marked 2 inline comments as done.Oct 30 2018, 1:26 PM
aprantl accepted this revision.Oct 31 2018, 1:39 PM
This revision is now accepted and ready to land.Oct 31 2018, 1:39 PM
vsk added inline comments.Oct 31 2018, 1:49 PM
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.

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.

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 added inline comments.Nov 5 2018, 8:43 AM
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

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)
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 added inline comments.Nov 6 2018, 7:26 AM
lib/Transforms/Utils/SimplifyCFG.cpp
1379

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

CarlosAlbertoEnciso edited the summary of this revision. (Show Details)

@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'.

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 accepted this revision.Nov 7 2018, 9:42 AM

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).

@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.

Looks good, thanks!

aprantl accepted this revision.Nov 8 2018, 4:50 PM
This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Nov 27 2018, 6:19 AM

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.

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.