Page MenuHomePhabricator

[DebugInfo] Don't translate dbg.addr and similar intrinsics into indirect DBG_VALUEs
ClosedPublic

Authored by jmorse on Mon, Oct 14, 8:20 AM.

Details

Summary

This patch kills-off a significant user of the "IsIndirect" field of DBG_VALUE machine insts. Brought up in in this bug [0], I suggested that IsIndirect was redundant, and @dblaikie broadly agreed. IMHO IsIndirect is a bit of a liability because its current useage can mean two things:

  • The IR input (dbg.declare/dbg.addr) wants the operand plus expression dereferenced,
  • The register allocator spilt the operand, and wants a dereference between the spill-address-calculation and whatever the original expression was.

Better IMO to just eliminate the first use case by appending a DW_OP_deref to the expression when the DBG_VALUE is created -- there are no circumstances AFAIUI where the machine-lowering transforms needs to know whether the location a DBG_VALUE refers to will be interpreted as an address or not. In the future we can use the IsIndirect field for Better Things (TM), more on that in a future patch [1]. This patch isn't quite NFC as it exposes another bug (see below).

This patch has all creators of DBG_VALUEs append a DW_OP_deref to all dbg.declare/dbg.addr's they handle, which makes the DWARF expression backend emit a memory location, the same as the IsIndirect field. Some producers were also setting IsIndirect for constant-valued DBG_VALUEs, I've switched those to set IsIndirect to false.

SelectionDAGBuilder::EmitFuncArgumentDbgValue has code that theoretically could lower a dbg.declare onto multiple registers. I don't think this ever happens, and I've added an assertion; no test hits it, a clang-3.4 build for x86_64 doesn't hit it. I don't *think* a scenario where LLVM specifies something is in memory with dbg.declare but then the backend reckons it arrives in registers would make sense, but I'm no ABI expert.

I've stripped out the "IsIndirect" field throughout LiveDebugVariables as its use case is gone, and added an assertion that LiveDebugVariables doesn't see any incoming DBG_VALUEs with IsIndirect set.

There are a lot of test changes, they fall into four categories:

  • Textual output change: a bunch of tests check what the textual assembly printer produces. If the IsIndirect field is set, the assembly printer writes "[$reg+0]" to represent indirection, wheras if there's a DW_OP_deref in the expression then it prints "[DW_OP_deref] $reg". Both produce the same DWARF. Tests that change in this way are:
    • test/CodeGen/ARM/debug-info-arg.ll
    • test/DebugInfo/ARM/PR16736.ll
    • test/DebugInfo/X86/dbg-addr-dse.ll
    • test/DebugInfo/X86/dbg-addr.ll
    • test/DebugInfo/X86/live-debug-vars-dse.mir
    • test/DebugInfo/X86/spill-indirect-nrvo.ll
    • test/DebugInfo/X86/spill-nontrivial-param.ll
  • Internal representation changes (IsIndirect -> DW_OP_deref), which is just a different way of expressing the same thing.
    • test/CodeGen/AArch64/GlobalISel/debug-cpp.ll
    • test/CodeGen/AArch64/GlobalISel/debug-insts.ll
    • test/CodeGen/PowerPC/debuginfo-stackarg.ll
    • test/DebugInfo/ARM/float-stack-arg.ll
  • There were a few tests where the input IR had a 'DW_OP_deref' in a dbg.declare, which doesn't play well with this implementation. I don't think it makes sense for dbg.declare to have a deref in its expression, so in these tests I've deleted it.
    • test/DebugInfo/X86/op_deref.ll
    • test/DebugInfo/X86/parameters.ll
    • test/DebugInfo/X86/safestack-byval.ll
    • test/DebugInfo/X86/vla.ll <--- I also added a llvm-dwarfdump check that the DWARF output is right

One COFF/CodeView test changes. This too had an excess DW_OP_deref in the input, but, fixing that causes the type name in the CodeView output to change (see diff). The new type appears correct (the function argument is a struct, not a reference), but I know nothing about CodeView (paging @rnk)

  • test/DebugInfo/COFF/pieces.ll

I've added XFails to two tests -- this is because of PR41992 [2], which @Orlando has a patch for somewhere.

I haven't added any new test, this is an internal representation change so I don't believe one is necessary.

[0] https://bugs.llvm.org/show_bug.cgi?id=41675#c9
[1] Signalling more information about spill locations between LiveDebugVariables and LiveDebugValues.
[2] https://bugs.llvm.org/show_bug.cgi?id=41992

Diff Detail

Event Timeline

jmorse created this revision.Mon, Oct 14, 8:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Oct 14, 8:20 AM
aprantl accepted this revision.Mon, Oct 14, 9:39 AM

Thanks a lot! When changing something like this, you might want to diff the debug info of a test program before and after the change and look for any unexpected changes not covered by our tests.

Yes, a DW_OP_deref is preferable to the indirect bit. How hard would it be to convert the remaining use-cases to retire the flag?

lib/CodeGen/GlobalISel/IRTranslator.cpp
1381 ↗(On Diff #224846)

Out of curiosity: is passing 0 here really the same as creating an undef?

This revision is now accepted and ready to land.Mon, Oct 14, 9:39 AM
rnk added a comment.Mon, Oct 14, 11:41 AM

This makes sense to me, we should be able to replace isIndirect with DW_OP_deref.

I checked the test case updates, they look good.

One concern that I have about folding everything into the DIExpression is that they are effectively leaked. For every unique variable location description, which now includes offsets and indirection, we make a DIExpression and never clean it up. This is probably not significant in overall compilation, but it feels sloppy to me. As a future alternative, maybe we could use the same representation (sequence of DWARF opcodes), but store them as MachineInstr operands, so that they get cleaned up after emitting the MachineFunction.

test/DebugInfo/COFF/pieces.ll
208 ↗(On Diff #224846)

This change looks good to me. The reference usage came about in D36907, which shouldn't apply in this case. The DefRangeRegisterRel record below already implies that the variable o lives in memory pointed to by RCX, which is correct, o is passed indirectly and it's address lives in RCX.

We started using references sometimes in D36907 as a way to squeeze out one more level of indirection through memory from this format. We'd use the reference if RCX was spilled, for example. In that case, we'd say that o was a reference, it lives *in* RCX, and later is spilled to stack slot offset N, and the debugger will display o as expected, even though it's the wrong type at the source level.

test/DebugInfo/X86/dbg-addr-dse.ll
6–8 ↗(On Diff #224846)

I just want to make sure that this gets addressed soon-ish, I suppose before the next release. I see that you said @Orlando has a patch for it.

I don't think it will cause any issues for Chromium, because we still set -instcombine-lower-dbg-declare=0, so things that are escaped are mainly described as stack locations in the side table. But, if we ever want to get off that flag, I want to make sure that LLVM can reliably track escaped variables.

In D68945#1708337, @rnk wrote:

One concern that I have about folding everything into the DIExpression is that they are effectively leaked. For every unique variable location description, which now includes offsets and indirection, we make a DIExpression and never clean it up. This is probably not significant in overall compilation, but it feels sloppy to me. As a future alternative, maybe we could use the same representation (sequence of DWARF opcodes), but store them as MachineInstr operands, so that they get cleaned up after emitting the MachineFunction.

I don't think that this poses a real problem since they are on average <16 bytes each and very repetitive and DIExpressions are globally uniqued by a FoldingSet, but I do see your point. A pathological case would be long-lived JIT that generates consecutive DIExpressions DW_OP_constu 0 , DW_OP_constu 1, ... .

jmorse marked 2 inline comments as done.Tue, Oct 15, 3:24 AM

Thanks a lot! When changing something like this, you might want to diff the debug info of a test program before and after the change and look for any unexpected changes not covered by our tests.

It looks like clang-3.4's X86ISelLowering.cpp produces identical debuginfo; I was expecting to see more PR41992 differences,

Yes, a DW_OP_deref is preferable to the indirect bit. How hard would it be to convert the remaining use-cases to retire the flag?

Moderately complicated, there's currently a good use-case for IsIndirect, it lets the PrologEpilog pass distinguish:

  • DBG_VALUEs that refer to the contents of a spill stack slot (IsIndirect=True)
  • DBG_VALUEs that should evaluate to a stack pointer, not its value (IsIndirect=False)

There are other ways of implementing this though, such as examining the stack-slot type or LiveDebugVariables prepending a DW_OP_deref for slot-contents locations.

I'd like to keep the field around, but use it to pass spill-slot frame indexes to LiveDebugValues, more on that in another patch though.

Reid wrote:

One concern that I have about folding everything into the DIExpression is that they are effectively leaked. [...] As a future alternative, maybe we could use the same representation (sequence of DWARF opcodes), but store them as MachineInstr operands.

That too might be achievable by putting the spill-slot frame index in DBG_VALUE insts.

lib/CodeGen/GlobalISel/IRTranslator.cpp
1381 ↗(On Diff #224846)

I think so -- both Indirect and Direct builders use the register-form of BuildMI and create DBG_VALUEs with $noreg as the first operand.

This change isn't technically necessary, but we may as well skip adding a spurious deref to a $noreg DBG_VALUE if we can.

lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
129 ↗(On Diff #224846)

Note to self, I missed this, and it risks asserting later

This revision was automatically updated to reflect the committed changes.

Thanks a lot! When changing something like this, you might want to diff the debug info of a test program before and after the change and look for any unexpected changes not covered by our tests.

It looks like clang-3.4's X86ISelLowering.cpp produces identical debuginfo; I was expecting to see more PR41992 differences,

Yes, a DW_OP_deref is preferable to the indirect bit. How hard would it be to convert the remaining use-cases to retire the flag?

Moderately complicated, there's currently a good use-case for IsIndirect, it lets the PrologEpilog pass distinguish:

  • DBG_VALUEs that refer to the contents of a spill stack slot (IsIndirect=True)
  • DBG_VALUEs that should evaluate to a stack pointer, not its value (IsIndirect=False) There are other ways of implementing this though, such as examining the stack-slot type or LiveDebugVariables prepending a DW_OP_deref for slot-contents locations.

    I'd like to keep the field around, but use it to pass spill-slot frame indexes to LiveDebugValues, more on that in another patch though.

Sounds like the problem here is that we don't encode the difference between Memory locations and Register locations in DIExpression and/or DBG_VALUE. I think we currently decide the very late in DwarfExpression.cpp.

(But we do have DW_OP_stack_value). Maybe we need a DW_OP_LLVM_memory_value, or rename this flag?

Hi,

I'm struggling with a case for my out-of-tree target where I think debug info is wrong with this change.

Before Isel we have:

call void @llvm.dbg.value(metadata i16 %x, metadata !20, metadata !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)), !dbg !13

And before this change we got the following after Isel:

DBG_VALUE %fixed-stack.1, 0, !"bar_y", !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)

With this change, we instead get this after Isel:

DBG_VALUE %fixed-stack.1, $noreg, !"bar_y", !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_deref, DW_OP_stack_value)

Isn't the DW_OP_deref inserted in the wrong place? I think it should be done before the DW_OP_LLVM_convert:s rather than after?

Adrian wrote:

Sounds like the problem here is that we don't encode the difference between Memory locations and Register locations in DIExpression and/or DBG_VALUE. I think we currently decide the very late in DwarfExpression.cpp.

Yeah, I've been bitten by this in the past, sometimes adding an extra DW_OP_deref to the end of an expression causes no change to the output DWARF because DwarfExpression.cpp was already guessing the location type ("unknown" -> memory). Particularly awkward is that adding any opcode might implicitly add a deref, if DwarfExpression.cpp starts interpreting the expression as a memory location rather than register location. Unless the expression is already an implicit location, in which case you don't get that extra deref!

(But we do have DW_OP_stack_value). Maybe we need a DW_OP_LLVM_memory_value, or rename this flag?

IMHO, YMMV, it would be better to head in the other direction and abandon what "kind" of location we have in the compilation stages, because it's a DWARF encoding detail. That would mean adding an opcode to a DIExpression always meant the same thing, with no flags to change their interpretation. Location kinds would have to be decided by DwarfExpression.cpp, something like:

  • All expressions ending in a deref are memory locations
  • Anything else not ending in a deref that performs some kind of computation is an implicit location
  • An empty expression is interpreted as a register location

Hi Mikael,

And before this change we got the following after Isel:

DBG_VALUE %fixed-stack.1, 0, !"bar_y", !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)

With this change, we instead get this after Isel:

DBG_VALUE %fixed-stack.1, $noreg, !"bar_y", !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_deref, DW_OP_stack_value)

Isn't the DW_OP_deref inserted in the wrong place? I think it should be done before the DW_OP_LLVM_convert:s rather than after?

You're right, that definitely looks wrong. This change was only supposed to affect DBG_VALUEs generated from indirect intrinsics (dbg.addr/dbg.declare); at a guess, it looks like dbg.values of Values that becomes FrameIndexes have the IsIndirect flag set too. I'll try to replicate this locally and patch it, please do back the commit out if it's causing significant misery.

(This is a great example of the kind of bug IsIndirect might be / have been causing, it doesn't express where the deref happens if there's an existing expression).

Hi Mikael,

And before this change we got the following after Isel:

DBG_VALUE %fixed-stack.1, 0, !"bar_y", !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)

With this change, we instead get this after Isel:

DBG_VALUE %fixed-stack.1, $noreg, !"bar_y", !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_deref, DW_OP_stack_value)

Isn't the DW_OP_deref inserted in the wrong place? I think it should be done before the DW_OP_LLVM_convert:s rather than after?

You're right, that definitely looks wrong. This change was only supposed to affect DBG_VALUEs generated from indirect intrinsics (dbg.addr/dbg.declare); at a guess, it looks like dbg.values of Values that becomes FrameIndexes have the IsIndirect flag set too. I'll try to replicate this locally and patch it, please do back the commit out if it's causing significant misery.

Thanks!

The C code originally looked like

typedef signed short T;
T bar(T d0, T d1, T d2, T d3, T d4, T x, T d5) {
  signed long bar_y = x;
  return bar_y;
}

(where short is 16bits and long is 32bits on my target) and "x" was passed on the stack.

Follow-up in D69028; hopefully the test there replicates the issue you've encountered, and the patch fixes it.

Adrian wrote:

Sounds like the problem here is that we don't encode the difference between Memory locations and Register locations in DIExpression and/or DBG_VALUE. I think we currently decide the very late in DwarfExpression.cpp.

Yeah, I've been bitten by this in the past, sometimes adding an extra DW_OP_deref to the end of an expression causes no change to the output DWARF because DwarfExpression.cpp was already guessing the location type ("unknown" -> memory). Particularly awkward is that adding any opcode might implicitly add a deref, if DwarfExpression.cpp starts interpreting the expression as a memory location rather than register location. Unless the expression is already an implicit location, in which case you don't get that extra deref!

(But we do have DW_OP_stack_value). Maybe we need a DW_OP_LLVM_memory_value, or rename this flag?

IMHO, YMMV, it would be better to head in the other direction and abandon what "kind" of location we have in the compilation stages, because it's a DWARF encoding detail. That would mean adding an opcode to a DIExpression always meant the same thing, with no flags to change their interpretation. Location kinds would have to be decided by DwarfExpression.cpp, something like:

  • All expressions ending in a deref are memory locations
  • Anything else not ending in a deref that performs some kind of computation is an implicit location
  • An empty expression is interpreted as a register location

Those two last examples don't really work: Due to salvageDebugInfo it is perfectly possible to arrive at either a register or a memory location that ends in DW_OP_constu 1 DW_OP_plus

I think we need slightly different tuning knobs. We need to distinguish:

  • axis 1: read-only vs. read-write locations Is it safe for the debugger to write to that location to change the variable's value? I think we can only guarantee that for dbg.declare(alloca) at the moment. In DWARF terms this is the difference between DW_OP_reg0 and DW_OP_breg0 DW_OP_stack_value.
  • axis 2: SSA-Value/(v)reg vs. memory locations

I need to think harder about this the primary open questions for me are:

  • why do we currently add DW_OP_stack_value in LLVM IR (which if the two axis does it address)?
  • is axis 2 currently implicitly encoded by the kind of value being bound by the DBG_VALUE and would it remove ambiguity to make it explicit? Because of my example above I'm convinced that DW_OP_deref is not a sufficient indicator.

Hmm.. I think my example was bad, too. A non-empty expression isn't a register location in the DWARF sense, and something that modifies the *value* isn't a memory location in the DWARF sense either. That said, I think the rest of the comment is still valid :-)

I'm working in this area for pr41992 so I noticed a couple of things (inline comments). I am happy to just add the changes I'm suggesting into my own upcoming patch if you'd like?

llvm/lib/CodeGen/LiveDebugVariables.cpp
105

I don't think either of these asserts are necessary now. Or, at least the first one's message needs updating to something like "expected DbgValueLocation to be pointer size".

112

I think this should have been updated. Previously the LocNo was 31 bits and UndefLocNo is defined above as ~0u, so after assigning LocNo = UndefLocNo, LocNo == INT_MAX. Now it's just equal to UndefLocNo.

I'm working in this area for pr41992 so I noticed a couple of things (inline comments). I am happy to just add the changes I'm suggesting into my own upcoming patch if you'd like?

Those both sound legit, please do!