Page MenuHomePhabricator

[DebugInfo] Correctly place DW_OP_derefs for arguments passed on stack
ClosedPublic

Authored by jmorse on Wed, Oct 16, 4:48 AM.

Details

Summary

Following on from D68945, @uabelho pointed out a scenario where there's some confusion about where a DW_OP_deref should be added for arguments passed on the stack. A Value/Argument that gets placed in memory requires the stack slot it lands in to be dereferenced, and that deref would go at the start of the expression so that it operates on the slot. If the argument is the operand to a dbg.declare, then it might require a deref at the end of the expression as well, to make it a memory location.

In SelectionDAG::EmitFuncArgumentDbgValue that's what I've implemented, best illustrated by the baz function in the attached test:

define i8 @baz(i32 *%blah) !dbg !40 {
entry:
  call void @llvm.dbg.declare(metadata i32* %blah, metadata !43, metadata !DIExpression(DW_OP_plus_uconst, 4)), !dbg !41
  ret i8 0, !dbg !41
}

%blah is an incoming pointer, but it arrives in a stack slot, which must be dereferenced first. And because it is used by a dbg.declare, we append a deref to force it to be a memory location. With this patch, we get the DBG_VALUE:

DBG_VALUE %fixed-stack.0, $noreg, !123, !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 4, DW_OP_deref)

i.e., "load the stack slot, fiddle with the pointer, then be a memory location". I've added llvm-dwarfdump checks to ensure this comes out the far end in the correct format (I'm 97% the encoding is correct there).

I've deleted the "IsIndirect" var in EmitFuncArgumentDbgValue, as it wasn't doing anything useful IMO.

Diff Detail

Event Timeline

jmorse created this revision.Wed, Oct 16, 4:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Oct 16, 4:48 AM
uabelho added inline comments.Wed, Oct 16, 5:25 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5558 ↗(On Diff #225193)

Nice, I think this is heading the right direction!
However, if we look at PEI::replaceFrameIndices which handles the indirect stuff, it uses DW_OP_deref_size rather than DW_OP_deref:

if (MI.isIndirectDebugValue() && DIExpr->isImplicit()) {
  SmallVector<uint64_t, 2> Ops = {dwarf::DW_OP_deref_size, Size};
  bool WithStackValue = true;
  DIExpr = DIExpression::prependOpcodes(DIExpr, Ops, WithStackValue);
  // Make the DBG_VALUE direct.
  MI.getOperand(1).ChangeToRegister(0, false);
}

I think we need to take the size into consideration here in SelectionDAGBuilder too.

My out-of-tree big-endian target example now goes wrong since we read a value that is placed adjacent to the wanted value on the stack. I think using DW_OP_deref_size could perhaps solve that.

jmorse updated this revision to Diff 225220.Wed, Oct 16, 7:34 AM

Cool, here's a version with deref_size; this could be refactored with the PrologEpilog site too, but it's not obvious which headers such a helper should live in.

The PrologEpilog site only uses deref_size if it's an implicit location, I've done the same here.

Just to clarify things in my own mind: dbg.declare expressions describe a location, while dbg.value expressions describe a value? And that's why you're wanting to add a trailing deref?
Then when we emit the actual DWARF, the trailing deref is omitted so the expression again describes a location.

Just to clarify things in my own mind: dbg.declare expressions describe a location, while dbg.value expressions describe a value? And that's why you're wanting to add a trailing deref?

Yup, that's correct. I repeatedly screw up the correct use of the term "location", so to be super explicit, dbg.declare always describes a _memory_ location.

Then when we emit the actual DWARF, the trailing deref is omitted so the expression again describes a location.

Indeed.

As suggested in https://reviews.llvm.org/D68945#1710656 , I'd enjoy a future where the DwarfExpression code didn't have to guess what kind of location we were dealing with (no "unknown" state).

Cool, here's a version with deref_size;

Great, now that failing testcase passes! Thanks!

I got another testcase that also started failing with the original commit that I hoped would be the same problem, but that isn't solved yet with this fix so I'll need to dig into that one and see what's going on there.

I got another testcase that also started failing with the original commit that I hoped would be the same problem, but that isn't solved yet with this fix so I'll need to dig into that one and see what's going on there.

That was probably false alarm, I think it's a problem in our testcase rather than in the patch.

vsk added a comment.Thu, Oct 17, 2:33 PM

This looks great.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5564 ↗(On Diff #225220)

Out of curiosity, why can't the sized deref be used for stack values?

jmorse marked an inline comment as done.Fri, Oct 18, 5:33 AM
jmorse added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5564 ↗(On Diff #225220)

Alas, I've no idea -- this is just closely matching the existing code in PrologEpilog. Most likely answer IMO is that it would be legal, but there'd be large amounts of test changes for very little gain.

vsk accepted this revision.Fri, Oct 18, 10:55 AM

Thanks!

This revision is now accepted and ready to land.Fri, Oct 18, 10:55 AM

I got another testcase that also started failing with the original commit that I hoped would be the same problem, but that isn't solved yet with this fix so I'll need to dig into that one and see what's going on there.

That was probably false alarm, I think it's a problem in our testcase rather than in the patch.

To be clear: It was false alarm. So I'm not aware of any problems with this patch.

Thanks!

(Reverse ping!) Any reason why this hasn't landed yet?

Ah blast, I mentally filed this under "things to push upwards when there's spare time" as opposed to "a regression that needs fixing". Sorry for the delay, I'll give it a kick now.

Ah blast, I mentally filed this under "things to push upwards when there's spare time" as opposed to "a regression that needs fixing". Sorry for the delay, I'll give it a kick now.

I think we considered adding this workaround downstream but ended up reverting the patch that broke our test case instead (or rather, we had already reverted it so this has not been highest prio for us).
Anyway, we are not really stuck due to this downstream, I just realized that it is a pity that we haven't moved forward with using (and testing) the new way of handling "IsIndirect" yet.

This revision was automatically updated to reflect the committed changes.