This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef] Remove a faulty assertion
ClosedPublic

Authored by jmorse on Aug 16 2021, 8:09 AM.

Details

Summary

This patch removes an assertion, and adds a regression test showing why the assertion is broken.

For context, LocIdx is a key/index number for machine locations, so that we can describe locations as a single integer and ignore whether they're on the stack, in registers or otherwise. Back when InstrRefBasedLDV was added, I happened to bake in a "special" zero number for various reasons, which Vedant identified as undesirable in this review comment: https://reviews.llvm.org/D83047#inline-765495 . I subsequently removed that special zero number, but it looks like I didn't delete this assertion at the time, which assumes that a zero LocIdx is invalid.

The attached test shows that this assertion is reachable on valid code -- on x86 $rsp always gets the LocIdx number zero, and if you transfer a variable value into it, InstrRefBasedLDV crashes on that assertion. The code might be a bit wild to be storing variables to $rsp like that, however we shouldn't crash on it.

Diff Detail

Event Timeline

jmorse created this revision.Aug 16 2021, 8:09 AM
jmorse requested review of this revision.Aug 16 2021, 8:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2021, 8:09 AM
Orlando accepted this revision.Aug 20 2021, 5:26 AM

Makes sense, LGTM.

This revision is now accepted and ready to land.Aug 20 2021, 5:26 AM
This revision was landed with ongoing or failed builds.Aug 20 2021, 6:24 AM
This revision was automatically updated to reflect the committed changes.