Page MenuHomePhabricator

[DebugInfo] Add DWARF emission for DBG_VALUE_LIST
Needs ReviewPublic

Authored by StephenTozer on Jul 9 2020, 11:09 AM.



Continuing the work discussed in the RFC[0], this patch implements the actual emission of DWARF from a DBG_VALUE_LIST instruction.

The logic for handling the new instruction is simple in most places; DbgEntityHistoryCalculator has a more complex set of changes since it's more involved with register tracking, and the code for producing DW_AT_location in both DwarfDebug and DwarfExpression also required some heftier work.

Previously, the code in emitDebugLocEntry functioned along the lines of:

  1. Emit any fragment info
  2. Emit any entry value info
  3. Emit the location specified in the DBG_VALUE, e.g. DW_OP_reg X or DW_OP_constu X
  4. Finally call DwarfExpression::addExpression(), which handles the DIExpression (except fragments)

Since there may now be multiple locations scattered throughout the expression, rather than a single location at the front, addExpression has been modified to optionally take a lambda that is used to handle DW_OP_LLVM_arg N; the lambda is passed in from emitDebugLocEntry, and performs step 3 using the Nth debug operand. Non-list debug values follow the same behaviour as before. DwarfCompileUnit::constructVariableDIEImpl is similar, but simpler.

The alternative to using the lambda would be to move some of the code in DwarfDebug::emitDebugLocEntry directly into DwarfExpr, and passing a list of locations to addExpression. The hard part with this is that DwarfDebug and DwarfCompileUnit perform step 3 differently, although it's possible their behaviour can be merged. The purpose of choosing the lambda was to minimize the amount of actual change made, but if the alternative option seems like an objectively good refactor then I'm happy to adjust.


Diff Detail

Event Timeline

StephenTozer created this revision.Jul 9 2020, 11:09 AM

Hey, just noticed a couple of comments to remove from the tests.


You can remove this XXX note.


I assume this no longer fails?


Can remove this XXX note. As you mentioned offline, having multiple references to the same arg (i.e. multiple DW_OP_LLVM_arg, 0 in the expr) is never a problem.

Though, slightly tangentially, I'm still a little unclear on what the final decision was on how to handle duplicate register arg operands. In D82363 you said 'always treat DBG_VALUE_LISTs as potentially having them'. Please could you explain a little further? (i.e. is it an error state, do we need to add extra checks when dealing with DBG_VALUE_LISTs etc).

StephenTozer marked an inline comment as done.Jul 10 2020, 4:28 AM
StephenTozer added inline comments.

It is not an error state, just a slightly more inconvenient form than one without duplicates. It requires some extra work in a few places (operating on a vector instead of a single pointer), but there is no reason for it to be invalid.

Remove old comments from tests.

Could we reduce complexity by entirely replacing DBG_VALUE with DBG_VALUE_LIST, *, DW_OP_arg 0, *?


Out of curiosity: Is there an operand kind that we could switch() over?


By using a callback here the callee cannot use the advanced functionality of addMachineRegExpression for any but a leading DW_OP_LLVM_arg. Do you see a way of either generalizing addMachineRegExpression or otherwise reorganizing this so the addMachineRegExpression functionality becomes available to DBG_VALUE_LIST?

StephenTozer marked 2 inline comments as done.Jul 17 2020, 1:43 PM

Could we reduce complexity by entirely replacing DBG_VALUE with DBG_VALUE_LIST, *, DW_OP_arg 0, *?

Yes, and in the long run I think that should be the goal; it would also be nice to remove the "indirect" operand and all the code paths that use it. So far it has been easier to create this as a separate instruction, but if the debug info cabal as a whole is positive on the replacement then there's no harm in bringing the replacement into these patches (or more likely, adding an extra patch to do so on top of the ones that are already being reviewed).


It would be useful to have one if there isn't; I didn't want to fold a change like that into this work, but if it exists I can use it (and if not it'd be nice to add in another patch).


It actually can use that functionality - in this case, all of the functionality that would normally be applied to the location in the DBG_VALUE is applied by this callback. The callback in this case can advance the ExprCursor, so there are no issues with using addMachineRegExpression normally at any point in a DBG_VALUE_LIST.

Add test to confirm that addMachineRegExpression is being correctly used for DBG_VALUE_LIST regs; replace if-else chain with switch in AsmPrinter::emitDebugValueComment.