This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] LiveDebugValues: explicitly terminate overwritten stack locations
ClosedPublic

Authored by jmorse on Aug 29 2019, 6:10 AM.

Details

Summary

Currently, if a stack spill location is overwritten to by another spill instruction, nothing notices and any variables located in that slot are not terminated. This can lead to incorrect variable values being reported by debuggers, see PR42772 [0]. The test added replicates this scenario. We should notice this memory-clobber and terminate variable locations when it happens.

Scanning for clobbered spill locations happens before scanning for new spills/restores, terminates open ranges and adds DBG_VALUE $noregs. The early scanning is because the current spill/restore logic appears to quit after moving one location, even if there could be multiple candidates, and I don't want to go near that right now. One function gets refactored into two: the old isSpillInstruction performed a few register liveness tests to see whether an instruction _looked_ like a spill, wheras all we're interested in for terminating existing locations is whether there's a stack write at all.

Note that this creates some slight differences between how registers and spill locations are treated: right now DbgEntityHistoryCalculator will spot register clobbers and terminate any variable locations that have been overwritten, thus manually inserting DBG_VALUE $noreg isn't necessary for registers in LiveDebugValues. IMO it's much harder for DbgEntityHistoryCalculator to spot memory/stack clobbers (it would have to dig into DIExpressions) wheras in LiveDebugValues we already have that information to hand.

I've left quite a lot of machine-code in the MIR test where it could be simpler; IMHO it's good to have a slightly larger test here just in case LiveDebugValues tries to do something clever in the future and gets it wrong.

There's a fractional increase in scope-bytes-covered by 0.05%, it's not completely clear to me why unfortunately D:

[0] https://bugs.llvm.org/show_bug.cgi?id=42772

Diff Detail

Repository
rL LLVM

Event Timeline

jmorse created this revision.Aug 29 2019, 6:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2019, 6:10 AM

Currently, if a stack spill location is overwritten to by another spill instruction, nothing notices and any variables located in that slot are not terminated.

Are all DBG_VALUEs pointing to spill slots created by LiveDebugValues and is this indicative of a bug elsewhere, i.e., should we instead make sure to insert a DBG_VALUE undef, var right before the value is popped from the spill slot (followed by a DBG_VALUE pointing to the restored register)?

lib/CodeGen/LiveDebugValues.cpp
357 ↗(On Diff #217845)

out of curiosity: is @MI legal doxygen?

870 ↗(On Diff #217845)

any previous DBG_VALUEs pointing to the spill slot? or does this mean something else?

jmorse marked 3 inline comments as done.Aug 30 2019, 7:54 AM

Are all DBG_VALUEs pointing to spill slots created by LiveDebugValues and is this indicative of a bug elsewhere,

LiveDebugVariables also creates DBG_VALUEs that point at spill slots. However, I think that's a feature, because a variable location may move to a spill slot because of a DBG_VALUE of a vreg that's already been spilt. i.e.,

CALL foo
DBG_VALUE %0
CALL bar

gets regalloc'd to

MOV %stack.0, $rax ; store to spill slot
CALL foo
DBG_VALUE %stack.0
CALL bar

This actually leads to all kinds of trouble (I've got a patch cooking about this), because we bake all information about the spill location into DIExpressions after the prologepilog pass eliminates stack-frame references. LiveDebugValues currently interprets any spill location created by LiveDebugVariables as being a register location with a complex expression, rather than being a spill location. Fixing this leads to some interesting choices, I'm currently writing that up.

There's also dbg.addr intrinsics to consider. These are special because they currently present as normal indirect DBG_VALUE instructions, but don't actually want to be terminated when their stack slot is written to. I haven't thought about how to represent that yet, but AFAIUI dbg.addrs are rarely produced right now, and terminating them early is better than allowing invalid locations to continue.

should we instead make sure to insert a DBG_VALUE undef, var right before the value is popped from the spill slot (followed by a DBG_VALUE pointing to the restored register)?

I believe we're handling that operation accurately with the existing restore mechanism (at least, for spills created by LiveDebugValues). Although, that code only ever restores a single variable location (even if there are multiple variables located there), which needs a bit of investigation.

The problem this patch addresses is when the spill has gone out of liveness without being restored -- there's no record from the register allocator of when that happens, thus we have to look for overwrites. It would be hard to fix at a higher level too, because stack slots get merged after register allocation.

lib/CodeGen/LiveDebugValues.cpp
357 ↗(On Diff #217845)

Dunno, I think it might be javadoc that doxygen swallows? Either way it appears from the live docs on llvm.org that doxygen produces no documentation for the LiveDebugValues pass, possibly because there's no class level comment? (I haven't dug into this, only moved the comment block).

870 ↗(On Diff #217845)

Ah yeah, that should be something like "First, if there are any DBG_VALUEs pointing to a spill slot that is written to..."

jmorse updated this revision to Diff 218366.Sep 2 2019, 7:19 AM
jmorse marked an inline comment as done.

Update a comment, we don't terminate variables, instead we close locations.

aprantl added inline comments.Sep 3 2019, 9:21 AM
lib/CodeGen/LiveDebugValues.cpp
797 ↗(On Diff #218366)

This doesn't appear to be used in the function?

881 ↗(On Diff #218366)

One last question: Could/should we fix this in DbgEntityHistoryCalculator instead?

jmorse updated this revision to Diff 218719.Sep 4 2019, 8:28 AM
jmorse marked an inline comment as done.

Delete a vector that isn't used by anything.

jmorse added inline comments.Sep 4 2019, 8:40 AM
lib/CodeGen/LiveDebugValues.cpp
881 ↗(On Diff #218366)

Overall it would make more sense to fix it in DbgEntityHistoryCalculator, that way the two logically similar things (working out when a location no longer contains the desired value) would happen in the same place. However, doing so would require parsing a DIExpression to work out where in the stack frame a DBG_VALUE points, which IMO is generally a bad plan.

My feeling is that creating some undef DBG_VALUEs here where the information is readily available is better than parsing a DIExpression at a later stage.

aprantl accepted this revision.Sep 4 2019, 12:48 PM

I see we would have to do an abstract interpretation of a DIExpression together with the implicit register operand bound by the DBG_VALUE to computes all stack slots that my be touched by the expression, assuming we even know where non-frame-pointer registers point. Could you add an explanation to the comment that lays out why this isn't feasible?

This revision is now accepted and ready to land.Sep 4 2019, 12:48 PM
jmorse updated this revision to Diff 218947.Sep 5 2019, 10:24 AM

Expand a comment, explaining why we terminate overwritten spill-locations during LiveDebugValues, instead of leaving it until later.

This revision was automatically updated to reflect the committed changes.