This is an archive of the discontinued LLVM Phabricator instance.

[Utils] Fix incorrect dbg.declare store conversion
ClosedPublic

Authored by loladiro on Jan 13 2016, 5:57 PM.

Details

Summary

The dbg.declare -> dbg.value conversion did not check which operand of the store instruction the alloca was passed to. As a result code that stored the address of an alloca, rather than storing to the alloca, would still trigger the conversion routine, leading to the insertion of an incorrect dbg.value intrinsic.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 44820.Jan 13 2016, 5:57 PM
loladiro retitled this revision from to [Utils] Fix incorrect dbg.declare store conversion.
loladiro updated this object.
loladiro added a reviewer: aprantl.
loladiro set the repository for this revision to rL LLVM.
loladiro added a subscriber: llvm-commits.
aprantl accepted this revision.Jan 14 2016, 8:54 AM
aprantl edited edge metadata.

Good catch! Looks pretty obvious to me. Some minor comments inline.

lib/Transforms/Utils/Local.cpp
1133

Please add more/full context to diffs uploaded to phabricator; it makes reviewing easier.

1138

Does turning this if cascade into a switch() make it any more readable?

test/Transforms/Util/store-first-op.ll
11

pass pass

13

I would still add a positive check — this way we'll be notified when the testcase needs to be updated.

This revision is now accepted and ready to land.Jan 14 2016, 8:54 AM
loladiro added inline comments.Jan 14 2016, 9:29 AM
lib/Transforms/Utils/Local.cpp
1138

I think that would introduce a lot of extra casting, which would probably make it less readable (assuming you were thinking of a cast on ->getOperand()), if there's a better syntax, do let me know.

loladiro added inline comments.Jan 14 2016, 9:31 AM
lib/Transforms/Utils/Local.cpp
1138

Sorry ->getOpcode of course.

loladiro added inline comments.Jan 14 2016, 9:34 AM
test/Transforms/Util/store-first-op.ll
13

What kind of positive test are you imagining. In the corrected version, no dbg.* intrinsic is inserted.

aprantl added inline comments.Jan 14 2016, 10:51 AM
lib/Transforms/Utils/Local.cpp
1138

Yes, that's what I meant. If it doesn't improve the readability (speed doesn't matter with three cases) let's leave it as is.

test/Transforms/Util/store-first-op.ll
13

My thought was that if no dbg.value is inserted the dbg.declare should still be there but that isn't actually the case. In this testcase the alloca is dead because there is no store to it or load, so not having any intrinsics makes sense.

Hm. This brings up an interesting point: Shouldn't LowerDbgDeclare() replace the alloca in the dbg.declare instruction with an %undef or alternatively leave the dbg.declare in there if the dbg.declare was not lowered? Otherwise we're going to loose function arguments and local variables.

loladiro added inline comments.Jan 14 2016, 10:57 AM
test/Transforms/Util/store-first-op.ll
13

Yes, I think that would be reasonable (but not for this patch - will go ahead and commit).

This revision was automatically updated to reflect the committed changes.