Page MenuHomePhabricator

[DwarfExpression] Support entry values for indirect parameters
ClosedPublic

Authored by vsk on Wed, May 20, 5:24 PM.

Details

Summary

A struct argument can be passed-by-value to a callee via a pointer to a
temporary stack copy. Add support for emitting an entry value DBG_VALUE
when an indirect parameter DBG_VALUE becomes unavailable. This is done
by omitting DW_OP_stack_value from the entry value expression, to make
the expression describe the location of an object.

rdar://63373691

Diff Detail

Event Timeline

vsk created this revision.Wed, May 20, 5:24 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptWed, May 20, 5:24 PM

Great! Thanks!

I think we should update the LangRef.rst (entry_values section) as well.

In addition, can we add a test case checking MIR output after LiveDebugValues?

llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-indirect-param.mir
19

I know that the final effect is the same, but should this be DW_OP_entry_value 2 DW_OP_bregN 0 instead of DW_OP_entry_value 1 DW_OP_regN?

72

Nit: !25-36! could be attached to !24

80

I believe we do need all the attributes.

aprantl added inline comments.Thu, May 21, 8:41 AM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
1287–1288

Can you add a comment? it's not necessarily obvious why an indirect entry value is not a memory location. Is it because an entry value is its own kind?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
790

What does "unusable" mean? Incorrect? Invalid?

2401–2402

same here

2407

factoring out this snippet into a template and reusing it in DwarfCompileUnit.cpp is probably not going to make this more readable, is it?

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
145–146

This comment is weird :-)

Additional flags that may be combined with any location description kind.?

148

Would it make more sense call this Indirect (w/o EntryValue) and treat it as a separate bit, so you can still test for EntryValue independently?

306

Is the dependency on MachineLocation really necessary, or could this just take a bool / enum IsIndirect?

307

see comment in header...

vsk marked 11 inline comments as done.Thu, May 21, 1:50 PM
vsk added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
1287–1288

This has more to do with pre-existing constraints in the dwarf emitter, more than anything else. DwarfExpression::addReg cannot be used with Memory location kinds, but we need that method to write out DW_OP_entry_value(DW_OP_regN).

I guess there's no such restriction for addBReg, but I'm not sure why we'd pick that representation (see Djordje's earlier comment).

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
790

Should be "incorrect", fixed. It's "unusable" in the sense that the debugger will error out trying to evaluate the expression: it will think, "struct Foo /is/ the pointer in this register", instead of, "struct Foo is /pointed to/ by this register".

2407

I tried doing this a few different ways :). I couldn't find a clean solution. In the end I opted for adding 'DwarfExpression::adjustLocationKind' to share just some of the logic. Lmk what you think.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
145–146

Fixed, that's much better.

148

Yes, that does seem better. It's more extensible this way, and anyway, most of the logic for treating direct/indirect entry values should be the same.

306

It aids encapsulation to not require clients to unpack the relevant bit of MachineLocation, I feel. I've changed this to accept a reference, to at least get rid of the header dependency. This seems acceptable to me, as DwarfExpression clients work with MachineLocation anyway.

llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-indirect-param.mir
19

I'm not sure, the DW_OP_regN description is simply the one that has already been wired up. Is there a reason to prefer 'DW_OP_bregN 0'?

72

That's a fair point, but I feel this is too onerous of a reduction to do by hand. As the MIR is already fairly reduced, I would prefer not to do this.

80

Fixed.

vsk updated this revision to Diff 265594.Thu, May 21, 1:53 PM
vsk marked an inline comment as done.

Address review feedback.

vsk marked an inline comment as done.Thu, May 21, 3:23 PM
vsk added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
790

Actually, this FIXME isn't correct. This is a holdover from an alternative implementation I had which introduced a DW_OP_LLVM_indirect_entry_value opcode. I'll remove this.

vsk updated this revision to Diff 265629.Thu, May 21, 3:24 PM

Remove incorrect fixme.

djtodoro added inline comments.Fri, May 22, 5:26 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
148

mot -> not

llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-indirect-param.mir
19

I was thinking about a more complex entry values where the offset is other than 0, where by using this way of representation we'll have more complex expression for the same thing.
If we have expressions as:

  1. DW_OP_entry_value 3 DW_OP_bregN M DW_OP_deref DW_OP_stack_value
  1. DW_OP_entry_value 6 DW_OP_entry_value 1 DW_OP_regN DW_OP_plus_uconst M DW_OP_deref DW_OP_stack_value

The two expressions have the same effect. They push the value memory location pointed to by value of register N upon entering current subprogram plus M had upon entering of the current subprogram.

This may not be worth of considering now, but we can put a TODO marker in DwarfExpression.

(Sorry If I am asking to much) One quick question:

Should the DW_OP_entry_value(DW_OP_reg0 W0)) be an entry value expression with dw_op_deref + dw_op_stack_val?

vsk marked 2 inline comments as done.Fri, May 22, 2:34 PM
vsk added inline comments.
llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-indirect-param.mir
19

These are good questions.

If the indirect parameter offset is not 0, then it seems perfectly fine to apply it outside of the nested entry value expression. However, we don't support emitting entry values in this case. I've added a test to illustrate that.

As for DW_OP_stack_value, I'm not sure it's the right thing to use for locations. It breaks LLDB, fwiw: LLDB doesn't have the type of the object available as it's evaluating a DWARF expression, so when it sees the DW_OP_deref, it can't figure out the size and fails to reconstruct the object.

vsk updated this revision to Diff 265805.Fri, May 22, 2:35 PM

Add test coverage for indirect parameter location with offset; fix typo; fix LangRef entry.

aprantl added inline comments.Fri, May 22, 2:47 PM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
151

I'm going to be pedantic now: Should this be Indirect instead of IndirectValue? I.e., can there be non-values (= modifyable locations) that are indirect?

158

"adjust" sounds like you could call this more than once. Should it be "set" or "initialize"? Or even be the constructor?

vsk marked 2 inline comments as done.Fri, May 22, 4:05 PM
vsk added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
151

Oh, I wasn't aware "Value" connoted "non-modifiable". In that case, "Indirect" is certainly a better fit: the user can choose to modify the temporary slot in the caller for the indirect param if they'd like.

158

"set" sounds good. The location isn't always available when the DwarfExpression is constructed.

vsk updated this revision to Diff 265815.Fri, May 22, 4:06 PM

Address review feedback.

vsk added a comment.Fri, May 22, 4:10 PM

Apparently, we always describe the value of an indirect parameter as being "whatever is in the temporary slot", even if the callee modifies it: https://godbolt.org/z/ZgWr_n. So treating the indirect parameter DBG_VALUE as pointing to a (modifiable) location sounds consistent to me.

Harbormaster failed remote builds in B57690: Diff 265815!
djtodoro accepted this revision.Mon, May 25, 6:44 AM

LGTM, thanks!

llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-indirect-param.mir
19

If the indirect parameter offset is not 0, then it seems perfectly fine to apply it outside of the nested entry value expression. However, we don't support emitting entry values in this case. I've added a test to illustrate that.

Thanks!

As for DW_OP_stack_value, I'm not sure it's the right thing to use for locations. It breaks LLDB, fwiw: LLDB doesn't have the type of the object available as it's evaluating a DWARF expression, so when it sees the DW_OP_deref, it can't figure out the size and fails to reconstruct the object.

Ah, sure.. The DW_OP_stack_value should be used for describing actual values rather than locations, so thanks!

This revision is now accepted and ready to land.Mon, May 25, 6:44 AM
This revision was automatically updated to reflect the committed changes.