Page MenuHomePhabricator

[DebugInfo] Use DBG_VALUEs IsIndirect field for describing stack spills

Authored by jmorse on Oct 18 2019, 9:15 AM.



This is a patch that significantly changes the meaning of the IsIndirect field of DBG_VALUEs. The objective here is to pass more stack-spill information down from LiveDebugVariables to LiveDebugValues: at the moment, if a DBG_VALUE of a stack slot is created by LiveDebugVariables recording a stack spill, then LiveDebugValues interprets it as a register location, not a spill location (see the added test, dbg-value-when-spilt.ll).

Rather than just using IsIndirect to indicate "memory location please" to DwarfExpression.cpp, with this patch it means:

  • Between LiveDebugVariables and PrologEpilog and where operand 0 is a stack slot, $noreg means this is a stack _pointer_ variable, immediate means it's a spill location.
  • Between PrologEpilog and LiveDebugValues, for spill locations this patch places the frame-index of the spill into the IsIndirect field, sets it to $noreg for everything else.
  • IsIndirect is invalid / must be $noreg everywhere else.

This means that when LiveDebugValues handles spill-location DBG_VALUEs, it can extract the frame offset and slot size from the frame index and create a SpillLocKind VarLoc instead of a register location based on the stack pointer. As a result, we can recognise loads from spills that we couldn't before, and, we can access a DIExpression that doesn't have the stack-offset already baked into it. Finally, LiveDebugValues can now propagate stack-spill locations over instructions that modify the stack pointer (like push) as shown in the new dbg-value-of-spill-with-fp.ll.

(This last improvement might be a bit debatable as the debugger would need to be aware that the stack pointer has changed. Ignoring stack-pointer writes for spills is what we do already for LiveDebugValues detected spills. I couldn't quickly convince gcc to cough up a location list with no frame pointer in a function where the stack pointer was modified, so can't show that anyone else does this).

There's a downside however: in some circumstances we newly recognise a spill restore in the middle of a loop, restore it, and then drop both the spilt and restored location for the whole loop because there isn't a single location on entering the head of the loop. For an example, see test/DebugInfo/X86/op_deref.ll where this now happens, I've marked the test XFail until there's been some discussion about this. Happily I think we can say that this is moving LiveDebugValues into the domain of having to make better choices about which location to pick, instead of just trying to stop it producing bad locations! My feeling is that there's a hierarchy of duration, of {volatile-reg, nonvolatile-reg, stack, entry-value}, and it might be worthwhile building this into LiveDebugValues. (Consider also PR43373 [0] which exhibits problems with mixing "long term" and "short term" locations).

Unfortunately there are negligable differences in location coverage on a clang-3.4 build, probably due to a mixture of the above improvements and downsides. This patch might also be too much churn; IMHO getting this information down to LiveDebugValues is something we should aim to do though. If we're going to be aware of spills/restores in LiveDebugValues then we want to know about what was spilt during LiveDebugVariables. Right now, that would have to involve interpreting a DIExpression, which IMO is undesirable.

I've added three new flavours of tests:

  • The whole backend of LLVM-IR to dwarf "spill and restore of an arg" performed by regalloc should be tested for. test/DebugInfo/X86/spill-indirect-nrvo.ll does most of this at the moment, I've added a check that a restore happens.
  • A DBG_VALUE that happens in the range where a Value is spilt should be dealt with in the same way. dbg-value-when-spilt.ll covers this.
  • For not closing spill locations when we def the stack pointer, dbg-value-of-spill-with-fp.ll

There are a bunch of uninteresting test changes; the "standard" updates to inputs and changes in outputs are:

  • I've updated MIR inputs to use frame indexes in the IsIndirect field rather than having offsets in DIExpressions and IsIndirect=0
  • Checks for after LiveDebugValues now expect IsIndirect to never be set, and every memory location to explicitly have a DW_OP_deref
  • Textual assembly output changes same as D68945

The uninteresting standard changes are:
test/DebugInfo/MIR/X86/live-debug-values-spill.mir: <--- I also set a bunch of _'s to explicitly be $noreg
test/DebugInfo/MIR/X86/prolog-epilog-indirection.mir: <-- this test is mostly checking stack slot pointers, not spilt values
test/DebugInfo/MIR/X86/live-debug-values-restore.mir: <--- and now gets the deref in the right location
test/DebugInfo/MIR/X86/live-debug-values-stack-clobber.mir: <--- I added some more expression testing
test/DebugInfo/X86/prolog-params.mir: <--- contains some negative FrameIndexes that get printed positive

More interesting but sound updates:
prologepilog no longer produces completely lowered DBG_VALUEs. Instead, I've just let the test run until after LiveDebugValues, and left the checks the same.

We now recognise a restore to eax prior to the end of the function. Which could be considered better.

This dbg.declare gets lowered to a single DBG_VALUE in a stack slot, which is then restored.

This wants to check for spill expression, I've let it run til after LiveDebugValues not LiveDebugVars

We pick up a restore here that we didn't before, on account of not recognising when a DBG_VALUE was a spill.

The sketchier test updates:
We recognise a restore now, but the hash of the split-dwarf file changes. I think this is expected, but know nothing of split-dwarf.

I've left this XFail, because I anticipate people will have opinions on how we should go about deciding when to restore from spill and when to not.

I've updated the DBG_VALUEs here to use stack slots, but one of the output offsets (-4) doesn't correspond to any actual stack slot. I've updated checks for the current output, but it's not really a sound update. Problems with this test:

  • It dbg.declares an arbitrary thing in memory
  • it doesn't dbg.declare a pointer
  • We would now never generate this IR or MIR, I think.

My preference would be to delete the sketchy function in this test; or at the least replace the DBG_VALUE with one that isn't impersonating a stack spill.

Diff Detail

Event Timeline

jmorse created this revision.Oct 18 2019, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2019, 9:15 AM
aprantl added inline comments.Oct 18 2019, 10:37 AM
223 ↗(On Diff #225644)

This warrants some comment as to what those two hashes are used for.

vsk added a subscriber: inglorion.Oct 18 2019, 3:30 PM

The high level approach makes sense to me. Restricting the meaning of DBG_VALUE's "IsIndirect" operand should reduce the complexity surrounding creating/updating debug instructions. And the ability to recognize loads-from-spills and propagate more DBG_VALUEs is a big plus.

+ @inglorion for any comment on test/CodeGen/MIR/X86/diexpr-win32.mir.

223 ↗(On Diff #225644)

I can't really tell right away what it's for, but suspect it's used in place of memcmp to do equality checking. If so, it may be simpler to just use memcmp.

330 ↗(On Diff #225644)

It looks like the diff needs to be clang-formatted.

17 ↗(On Diff #225644)

I'm assuming these two stack slots for %somearg are from before/after the first asm sideeffect. I suppose this is the repeated location spec you referred to? If so, wdyt of tightening this to:

DW_OP_breg4 ESP+[[SLOT:[0-9]]]
DW_OP_breg4 ESP+[[SLOT]]
61 ↗(On Diff #225644)

Mind moving the XFAIL to the first line of the test, with a pointer to the more detailed comment within for.body, and one to the check line that fails?

Alternatively, if at least one location for vla survives, why not disable the failing check line(s) instead of xfailing the whole test? You might consider filing a PR about possible improvements related to this test.

jmorse updated this revision to Diff 231884.Dec 3 2019, 5:39 AM

Address review comments -- the "hash" fields were for comparing the contents of the Loc union, as it's now too big to be a value type. I've used memset and memcmp in its place now, which should be a bit clearer.

There's an additional change to the test CodeGen/ARM/dbg-range-extension.mir, but it's not interesting (constant-valued DBG_VALUE had isIndirect set). Not sure why this didn't fail before, I probably just forgot to add it to this patch.

wmi added a subscriber: wmi.Dec 3 2019, 9:19 AM
aprantl added inline comments.Dec 3 2019, 9:30 AM

It looks like this is exploding the size of the union. Could SpillLocation be made to fit into 64 bits, or does the extra 8 bytes not matter, or should it be an index into a side-table in the union?

aprantl added inline comments.Dec 3 2019, 9:32 AM

At some point we'll cross over the point were this should be inheritance instead of a union.

djtodoro added inline comments.


I think we should think about refactoring the VarLoc, by creating several structures representing various locations. That should improve readability.


we do not need the PrependFlags, so:

DIExpression::prepend(DIExpr, DIExpression::ApplyOffset, Offset);

jmorse updated this revision to Diff 232124.Dec 4 2019, 7:09 AM
jmorse marked 6 inline comments as done.
jmorse added inline comments.

I think 32 bits is needed for both the offset and slot size, as it's feasible (if unlikely) that objects more than 2^16 could appear on the stack. However, there's probably no point storing the frame register in the spill location as it doesn't vary. The latest revision removes storing the base register, and assumes we use the frame register everywhere. This lets us move back to just having a single 8-byte union.


Done, cheers!

aprantl added inline comments.Dec 4 2019, 4:14 PM

Can we put a static_assert that sizeof(Loc) == 8?

jmorse marked 4 inline comments as done.Dec 5 2019, 9:09 AM
jmorse added inline comments.

Happily already present in the original code, lines 248/249

aprantl accepted this revision.Dec 5 2019, 9:43 AM
aprantl added inline comments.

Nit: assuming that prependOpcodes takes an ArrayRef, a uint64 Ops[] = {dwarf::DW_OP_deref_size, Size}; seems sufficient here?

This revision is now accepted and ready to land.Dec 5 2019, 9:43 AM