This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Fix updateDbgUsersToReg to support DBG_VALUE_LIST
ClosedPublic

Authored by StephenTozer on Apr 29 2021, 5:12 AM.

Details

Summary

This patch resolves an issue reported in review https://reviews.llvm.org/D91722 (post-commit). The issue is relatively simple; the function updateDbgUsersToReg was not updated to support DBG_VALUE_LIST instructions in the patch that introduced that instruction. The fix is also simple, simply replacing hardcoded operand indices (getOperand(0)) with the new debug operand getter. The patch has also modified the assertion MI->isDebugInstr() with MI->isDebugValue(), as the former is overly general for this function. A test will be added to the review shortly, but the functional changes of the patch should be easy to review regardless.

Diff Detail

Event Timeline

StephenTozer created this revision.Apr 29 2021, 5:12 AM
StephenTozer requested review of this revision.Apr 29 2021, 5:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2021, 5:12 AM

Looks mechanically correct, test wanted seeing how this was missed before.

Added test case.

Remove git hash and directory path from test case metadata.

jmorse added inline comments.Apr 29 2021, 9:18 AM
llvm/test/DebugInfo/Generic/machine-cp-updates-dbg-reg.mir
4
12

Hardcoded metadata numbers will break in the future, this needs to be captured and compared.

14

IMO: better to check the entire line.

Fixed an issue with the previous implementation of this fix: the CopyDbgUsers vectors would sometimes contain duplicate elements, causing errors when we attempt to replace the same debug operand multiple times. This has been fixed by making it into a SmallSet instead of a SmallVector, although we still convert to SmallVector before updating to avoid changing the MRI interface.

jmorse accepted this revision.May 5 2021, 6:07 AM

This LGTM modulo one inline comment, plus: the test is in the "Generic" directory, but it is not fully generic. It depends on the armv6 triple being supported, so things like the powerpc build bots will likely trip on it. You should add an appropriate REQUIRES line.

llvm/test/DebugInfo/Generic/machine-cp-updates-dbg-reg.mir
5

"then" -> "when" :)

This revision is now accepted and ready to land.May 5 2021, 6:07 AM
jmorse added inline comments.May 5 2021, 7:11 AM
llvm/test/DebugInfo/Generic/machine-cp-updates-dbg-reg.mir
5

Nope, actually today I can't read, ignore this.

Added a "requires" to the test, and simplified the test target to just arm (instead of armv6k, which is not needed for the test to pass).

Added a "requires" to the test, and simplified the test target to just arm (instead of armv6k, which is not needed for the test to pass).

Might it be more suitable to move the test to the DebugInfo/ARM directory - which I think implicitly has such a requirement?

Might it be more suitable to move the test to the DebugInfo/ARM directory - which I think implicitly has such a requirement?

This is something I considered before updating; I believed that it would be better in the generic folder, because the test isn't about anything actually specific to ARM, we only need the ARM target for the current reproducer. Making it require ARM is the easiest solution, and if that means it should be in the ARM directory then I'm fine to move it there - although the ideal solution in that case would probably be to find a reproducer that doesn't depend on using ARM as the target.

Might it be more suitable to move the test to the DebugInfo/ARM directory - which I think implicitly has such a requirement?

This is something I considered before updating; I believed that it would be better in the generic folder, because the test isn't about anything actually specific to ARM, we only need the ARM target for the current reproducer. Making it require ARM is the easiest solution, and if that means it should be in the ARM directory then I'm fine to move it there - although the ideal solution in that case would probably be to find a reproducer that doesn't depend on using ARM as the target.

Yeah - I don't think the target-specific directories are about issues being innately target specific, they only express the raw idea that the test requires ARM to run, mechanically speaking.

Yeah, generic tests are generally nicer - but I'll admit I don't try /especially/ hard to get generic reproducers - I'll checkin x86 tests pretty regularly, for instance.

Move test to DebugInfo/ARM, remove explicit ARM target requirement.

I believe this is causing a crash: https://crbug.com/1206764. There is a non-reduced repro in the bug, currently creducing to come up with a reduced repro. Reverting...

lebedev.ri reopened this revision.May 7 2021, 12:07 PM
lebedev.ri added a subscriber: lebedev.ri.

@aeubanks reverted this the moment i was going to do so.

This revision is now accepted and ready to land.May 7 2021, 12:07 PM

I've identified the issue with this patch, I'll submit an updated version for review shortly.

The previous version of the patch was handling subregisters incorrectly; the updateDbgUsersToReg function assumed that each operand MO that corresponded to OldRegwould satisfy MO.getReg() == OldReg - this is correct for virtual registers, which use the subReg field, but not for physical registers.

To fix the issue, updatedbgUsersToReg has been modified to take MCRegisters as arguments, and checks each debug operand for the target DBG_VALUE* instruction to see if it has any RegUnits in common with OldReg; if it does, then we replace it with NewReg.

Hmmmmm. What if we have:

$rax = COPY $rcx
DBG_VALUE $ax

And the copy to $rax is copy-prop'd? As I understand it, the DBG_VALUE will appear on the MaybeDeadDbgUsers list, and its operand $ax will be detected as being affected by the copy propagation. But because we use:

Op.setReg(NewReg);

In updateDbgUsersToReg, doesn't this then produce:

DBG_VALUE $rcx

i.e., widens what was a 16 bit register to a 64 bit one?

Another question is what the old implementation of this did. Was it similarly borken?

Another question is what the old implementation of this did. Was it similarly borken?

You're exactly correct, this implementation has that flaw and the old one did as well. I think in most cases where we aren't using a full register, this doesn't matter because we're simply using the variable value directly, in which case the truncation to the variable size fixes this for us. There are some cases where this won't work however - such as if we have DW_OP_minus in the DWARF expression, in which case including the higher bits may result in an incorrect output. I'll open a bugzilla ticket about the issue, but since it exists before this patch I think the fix can come later.

I'll open a bugzilla ticket about the issue, but since it exists before this patch I think the fix can come later.

Misery. But, now tracked misery at the very least. To me, this LGTM as the cause of the assertion failure is now understood and addressed.

This revision was landed with ongoing or failed builds.May 12 2021, 2:20 AM
This revision was automatically updated to reflect the committed changes.