Page MenuHomePhabricator

[SimplifyCFG] Remap rewritten debug intrinsic operands.
ClosedPublic

Authored by rickyz on Thu, May 7, 2:55 PM.

Details

Summary

FoldBranchToCommonDest clones instructions to a different basic block,
but handles debug intrinsics in a separate path. Previously, when
cloning debug intrinsics, their operands were not updated to reference
the correct cloned values. As a result, we would emit debug.value
intrinsics with broken operand references which are discarded in later
passes. This leads to incorrect debuginfo that reports incorrect values
for variables.

Fix this by remapping debug intrinsic operands when cloning them.

Fixes https://bugs.llvm.org/show_bug.cgi?id=45667.

Diff Detail

Event Timeline

rickyz created this revision.Thu, May 7, 2:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, May 7, 2:55 PM
rickyz updated this revision to Diff 262774.Thu, May 7, 3:11 PM

Oops, forgot to write a commit message.

rickyz edited the summary of this revision. (Show Details)Thu, May 7, 3:12 PM
rickyz updated this revision to Diff 262775.Thu, May 7, 3:12 PM

Typo fix.

rickyz edited the summary of this revision. (Show Details)Thu, May 7, 3:13 PM
rickyz updated this revision to Diff 262777.Thu, May 7, 3:14 PM

One more commit message typo, sorry for the noise!

Harbormaster completed remote builds in B56111: Diff 262775.
Harbormaster completed remote builds in B56109: Diff 262770.
vsk accepted this revision.Thu, May 7, 5:24 PM
vsk added reviewers: jmorse, Orlando, aprantl.
vsk added a subscriber: debug-info.

This looks correct to me. Please wait for another +1 before committing this.

llvm/test/DebugInfo/simplify-cfg-preserve-dbg-values.ll
116

It's probably safe to strip all (or most) of these attributes out, as they're unlikely to affect the test.

This revision is now accepted and ready to land.Thu, May 7, 5:24 PM
rickyz updated this revision to Diff 262794.Thu, May 7, 5:27 PM
rickyz marked an inline comment as done.

Strip unneeded attributes from test.

rickyz added a comment.Thu, May 7, 5:29 PM
In D79602#2026367, @vsk wrote:

This looks correct to me. Please wait for another +1 before committing this.

Thanks! I'm not a committer, so someone else will probably need to commit this once it has the additional +1.

Thanks for this!
The change looks good to me, but I think we should trim the test a bit. Please find some advices below.

llvm/test/DebugInfo/simplify-cfg-preserve-dbg-values.ll
88

Can we get rid of using this?

91

This is unused, so please delete this.

104

Can we get rid of using this?

117

filename: "test.cc", directory: "/dir"

124

directory: /dir

137

I guess we don't need TBAA for this test.

138

Little trick to trim the test: you can use only one location for all other locations with the same scope, e.g.:

all !dbg !23, !dbg !24, !dbg !25, !dbg !26 replace with !dbg !22, and then delete these unused`DILocations`

rickyz updated this revision to Diff 262840.Fri, May 8, 1:59 AM
rickyz marked 9 inline comments as done.

Minimize simplify-cfg-preserve-dbg-values.ll some more.

rickyz marked an inline comment as done.Fri, May 8, 2:00 AM
rickyz added inline comments.
llvm/test/DebugInfo/simplify-cfg-preserve-dbg-values.ll
117

I completely missed that these snuck in - thanks for catching!

138

Done - also removed the !range metadata, which didn't seem to be needed.

Looks good to me! Thanks!

This revision was automatically updated to reflect the committed changes.