Page MenuHomePhabricator

[InstrRef][X86] Drop debug instruction numbers from x87 instructions
ClosedPublic

Authored by jmorse on Jul 8 2021, 2:21 PM.

Details

Summary

Avoid a crash when using instruction referencing if x87 floating point instructions are used. These instructions are significantly mutated when they're rewritten from referring to registers, to referring to floating-point-stack positions. As a result, their operands are re-ordered, and (InstrRef) LiveDebugValues asserts when it sees a DBG_INSTR_REF referring to a non-reg non-def register operand.

To fix this, I'm just dropping the instruction numbers, and thus variable locations. This avoids the crash -- we could try and record the substitution from fp-register operand to floating-point-stack-position but then... what would we do with the floating point stack position? They shift position throughout the program, and an additional dataflow analysis would be needed to work out how to refer to them correctly.

Instead, take the pragmatic approach that VarLoc LiveDebugvalues / variable locations does, and drop the lot of them. This isn't a coverage regression at all, because all DBG_VALUEs of, for example, $fp0, don't make it to the output file because they're pseudo-registers with no DWARF register number. I'll open a PR for poor x87 variable location support, however I suspect interest is limited.

This patch adds a "dropDebugNumber" helper to MachineInstr, the idea being that it's (sort-of) self documenting what's going on. The attached test case covers all six call sites that I add in the x86-codegen pass.

Diff Detail

Event Timeline

jmorse created this revision.Jul 8 2021, 2:21 PM
jmorse requested review of this revision.Jul 8 2021, 2:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2021, 2:21 PM
StephenTozer added a subscriber: StephenTozer.EditedJul 16 2021, 11:08 AM

Quick question: Do call instructions need to be handled here? I tried modifying the C source to have a variable directly assigned from a call instruction:

d = ext();
a *= d;

Then with the following command line (I don't know if there's a way to pass experimental-debug-variable-locations directly to clang), the call+instruction reference appear as in the below block: clang -m32 -g -O2 test.c -S -emit-llvm -o - | llc -experimental-debug-variable-locations -stop-before=x86-codegen -o test.mir

CALLpcrel32 @ext, csr_32, implicit $esp, implicit $ssp, implicit-def $esp, implicit-def $ssp, implicit-def $fp0, debug-instr-number 6, debug-location !23
ADJCALLSTACKUP32 0, 0, implicit-def dead $esp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $esp, implicit $ssp, debug-location !23
DBG_INSTR_REF 6, 6, !19, !DIExpression(), debug-location !21

If I understand the syntax correctly, this means that the DBG_INSTR_REF is referring to $fp0 in the call instruction. Finally, running llc test.mir -experimental-debug-variable-locations -run-pass=x86-codegen -o - translates this to the following:

CALLpcrel32 @ext, csr_32, implicit $esp, implicit $ssp, implicit-def $esp, implicit-def $ssp, debug-instr-number 6, debug-location !23
ADJCALLSTACKUP32 0, 0, implicit-def dead $esp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $esp, implicit $ssp, debug-location !23
DBG_INSTR_REF 6, 6, !19, !DIExpression(), debug-location !21

If I've understood correctly, the DBG_INSTR_REF now refers to a non-existent operand, while the call instruction still has a debug-instr-number attached. I'd assume that this is an issue given the other areas handled by this patch, does this need handling here?

Edit: Just an extra point, the only other place that looked like it wasn't being handled was return instructions, but I'm assuming that we would never use a return instruction for a debug instruction reference.

llvm/include/llvm/CodeGen/MachineInstr.h
475

👍

llvm/test/DebugInfo/MIR/InstrRef/x86-fp-stackifier-drop-locations.mir
85

Nit, remove git+commit reference.

Hi @jmorse,

Thanks for this. It looks reasonable, but I was wondering if there is an automatic way for ensuring that when deletion of an operand occurs, there is no dangling debug ref to it. It seems like a good candidate for an assertion somehow? I guess we should be aware that this feature will be used for some downstream targets as well, so we need to provide some kind of mechanism for them in order to keep things valid, any thoughts?

Best,
Djordje

jmorse updated this revision to Diff 359723.Jul 19 2021, 3:26 AM

Stephen wrote:

Quick question: Do call instructions need to be handled here? I tried modifying the C source to have a variable directly assigned from a call instruction:

Erk, that's right, those should indeed have instruction numbers dropped, it's being mutated and the operands don't mean the same things any more. This revision adds another drop call, and labels a call in the MIR with a debug-instr-number, to be dropped.

I suppose it's possible that, if a function returns more than one value, that we could un-necessarily drop non-float variable information where it could have been preserved. As far as I understand it, LLVM doesn't support multiple return values though.

(Possibly, this should end up on the mailing list),

Thanks for this. It looks reasonable, but I was wondering if there is an automatic way for ensuring that when deletion of an operand occurs, there is no dangling debug ref to it. It seems like a good candidate for an assertion somehow?

That definitely makes sense for things like RemoveOperand -- it's going to cause all other operands to be re-numbered, thus breaking many connections between DBG_INSTR_REFs and the instruction. I'll try putting a dropDebugNumber into RemoveOperand and see if it affects variable coverage. (I think this patch will still be sufficient).

NB: This particular patch was generated by an assertion firing, while building a large codebase InstrRefBasedLDV followed a chain of substitutions back to an operand that wasn't a register def, and asserted. This won't catch everything, but at least caught this.

I guess we should be aware that this feature will be used for some downstream targets as well, so we need to provide some kind of mechanism for them in order to keep things valid, any thoughts?

Downstream targets are inevitably going to need some modifications to be correct, as with this patch; I should add something to SourceLevelDebugging.rst to document what the update rules are for debug-instr-numbers. I think the extra instrumentation is necessary to improve past what DBG_VALUEs can currently provide.

Unfortunately I don't think there's any way of proving or ensuring the debug-info is always correct, because proving that two functions are identical is generally undecidable. There's always going to be a small chance of an un-instrumented optimisation out there subtly modifying an instruction in a way that makes it calculate a different value, and where debug-info should be dropped, but isn't. I think we have to accept this as a good trade-off in risk: right now, register coalescing causes large numbers of DBG_VALUEs to point at the wrong value, and there's no good way to fix it, wheras with debug-instr-number there's always a safe option of dropping.

StephenTozer accepted this revision.Jul 19 2021, 6:29 AM

LGTM. Documenting the update rules for instruction referencing debug values seems important, but doesn't need to be tied to this patch in particular I think.

This revision is now accepted and ready to land.Jul 19 2021, 6:29 AM

(Possibly, this should end up on the mailing list),

+1

Thanks for this. It looks reasonable, but I was wondering if there is an automatic way for ensuring that when deletion of an operand occurs, there is no dangling debug ref to it. It seems like a good candidate for an assertion somehow?

That definitely makes sense for things like RemoveOperand -- it's going to cause all other operands to be re-numbered, thus breaking many connections between DBG_INSTR_REFs and the instruction. I'll try putting a dropDebugNumber into RemoveOperand and see if it affects variable coverage. (I think this patch will still be sufficient).

Thanks!

NB: This particular patch was generated by an assertion firing, while building a large codebase InstrRefBasedLDV followed a chain of substitutions back to an operand that wasn't a register def, and asserted. This won't catch everything, but at least caught this.

I guess we should be aware that this feature will be used for some downstream targets as well, so we need to provide some kind of mechanism for them in order to keep things valid, any thoughts?

Downstream targets are inevitably going to need some modifications to be correct, as with this patch; I should add something to SourceLevelDebugging.rst to document what the update rules are for debug-instr-numbers. I think the extra instrumentation is necessary to improve past what DBG_VALUEs can currently provide.

Unfortunately I don't think there's any way of proving or ensuring the debug-info is always correct, because proving that two functions are identical is generally undecidable. There's always going to be a small chance of an un-instrumented optimisation out there subtly modifying an instruction in a way that makes it calculate a different value, and where debug-info should be dropped, but isn't. I think we have to accept this as a good trade-off in risk: right now, register coalescing causes large numbers of DBG_VALUEs to point at the wrong value, and there's no good way to fix it, wheras with debug-instr-number there's always a safe option of dropping.

I agree and good documentation is the right way to proceed with this.

This revision was landed with ongoing or failed builds.Jul 19 2021, 7:08 AM
This revision was automatically updated to reflect the committed changes.