Page MenuHomePhabricator

[LiveDebugValues] Omit entry values for DBG_VALUEs with pre-existing expressions
ClosedPublic

Authored by dstenb on Dec 12 2019, 5:57 AM.

Details

Summary

This is a quickfix for PR44275. An assertion that checks that the
DIExpression is valid failed due to attempting to create an entry value
for an indirect parameter. This started appearing after D69028, as the
indirect parameter started being represented using an DW_OP_deref,
rather than with the DBG_VALUE's second operand, meaning that the
isIndirectDebugValue() check in LiveDebugValues did not exclude such
parameters. A DIExpression that has an entry value operation can
currently not have any other operation, leading to the failed isValid()
check.

This patch simply makes us stop considering emitting entry values
for such parameters. To support such cases I think we at least need
to do the following changes:

  • In DIExpression::isValid(): Remove the limitation that a DW_OP_LLVM_entry_value operation can be the only operation in a DIExpression.
  • In LiveDebugValues::emitEntryValues(): Create an entry value of size 1, so that it only wraps the register operand, and not the whole pre-existing expression (the DW_OP_deref).
  • In LiveDebugValues::removeEntryValue(): Check that the new debug value has the same debug expression as the original, rather than checking that the debug expression is empty.
  • In DwarfExpression::addMachineRegExpression(): Modify the logic so that a DW_OP_reg* expression is emitted for the entry value. That is how GCC emits entry values for indirect parameters. That will currently not happen to due the DW_OP_deref causing the !HasComplexExpression to fail. The LocationKind needs to be changed also, rather than always emitting a DW_OP_stack_value for entry values.

There are probably more things I have missed, but that could hopefully
be a good starting point for emitting such entry values.

Diff Detail

Event Timeline

dstenb created this revision.Dec 12 2019, 5:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2019, 5:57 AM

Thanks! LGTM.

We can use llvm.org/pr44275 to track improvements for this.

llvm/test/DebugInfo/MIR/X86/dbgcall-site-reference.mir
104

I guess we do not need the tracksRegLiveness, liveins, stack and callSites for the test case.

LGTM (I'm not very familiar with entry values, so best to see what other reviewers thing too).

aprantl accepted this revision.Dec 12 2019, 9:20 AM

Seems obviously safe.

This revision is now accepted and ready to land.Dec 12 2019, 9:20 AM
vsk accepted this revision.Dec 12 2019, 10:38 AM

Thanks, lgtm as well.

dstenb marked 2 inline comments as done.Dec 13 2019, 1:51 AM
dstenb added inline comments.
llvm/test/DebugInfo/MIR/X86/dbgcall-site-reference.mir
104

I'll remove those when landing this.

This revision was automatically updated to reflect the committed changes.
dstenb marked an inline comment as done.