Page MenuHomePhabricator

[DebugInfo] Emit "undef" DBG_VALUEs when SDNodes are optimised out
ClosedPublic

Authored by jmorse on Dec 6 2018, 7:46 AM.

Details

Summary

This is a fix for PR39896 [0], where SDNodes that have been optimised out do not lead to "DBG_VALUE undef" instructions being created. Such undef instructions are necessary to terminate earlier variable ranges, otherwise variable values leak past the point where they're valid.

The "invalidated" flag of SDDbgValue is currently being abused to mean two things:

  1. The corresponding SDNode is now invalid
  2. This SDDbgValue should not be emitted

Of which there are several legitimate combinations of meaning:

  • The SDNode has been invalidated and we should emit "DBG_VALUE undef"
  • The SDNode has been invalidated but the debug data was salvaged, don't emit anything for this SDDbgValue
  • This SDDbgValue has been emitted

I've added an "Emitted" field to distinguish when the SDNode is invalid and when we just don't want to emit it. A SDDbgValue can be re-emitted too, because it seems to happen twice for some dbg.addrs, specifically DebugInfo/X86/dbg-addr-dse.ll gets DBG_VALUEs at the start of the function and in the middle.

As well as the regression test added, I've edited three tests, explained here:

LLVM :: DebugInfo/X86/pieces-3.ll
    The tail of this function is some completely superfluous computations that
     have dbg.values attached, and gets optimised out. Previously this was ignored
     by SelectionDAG, but now we emit a DBG_VALUE undef it reduces the range of
     the variables. Seeing how the tail is needless, and isn't generated by clang for
     the given source code, I've just deleted it. Otherwise emitting undef is correct,
     because the designated value was optimised out.
LLVM :: DebugInfo/NVPTX/debug-info.ll
     The full debug info section is part of the test here, and has some kind of fractional
     change. I've just updated the lines to the latest version.
LLVM :: DebugInfo/X86/dbg-value-inlined-parameter.ll
      According to a FIXME, some parameter was accidentally dropped on Linux.
      It now doesn't, so I've normalised the test here.

[0] https://bugs.llvm.org/show_bug.cgi?id=39896

Diff Detail

Repository
rL LLVM

Event Timeline

jmorse created this revision.Dec 6 2018, 7:46 AM
jmorse updated this revision to Diff 176996.Dec 6 2018, 10:06 AM
jmorse retitled this revision from [WIP] Emit "undef" DBG_VALUEs when SDNodes are optimised out to [DebugInfo] Emit "undef" DBG_VALUEs when SDNodes are optimised out.
jmorse edited the summary of this revision. (Show Details)
jmorse set the repository for this revision to rL LLVM.
jmorse added subscribers: debug-info, llvm-commits.

Un-WIPed this revision which should be ready for review now: added llvm-commits, reviewers, and the like.

The basic idea looks good to me.

test/DebugInfo/X86/pieces-3.ll
40 ↗(On Diff #176996)

Why is this necessary?

jmorse added inline comments.Dec 7 2018, 3:56 AM
test/DebugInfo/X86/pieces-3.ll
40 ↗(On Diff #176996)

IMHO lines 41-45 are wrong. On line 40 we dbg.value assign the (truncated) return value to the 0-32 fragment of "i1", rotate and truncate %outer.coerce1 to get the other part of "i1" on line 42, then on lines 43-45 we dbg.value assign it to the same 0-32 fragment of "i1". The lower field of "i1" thus gets two dbg.value assignments where the original C program had only one. The second refers to values that SelectionDAG drops -- previously this was ignored, but now it un-necessarily shortens the range of the first assignment.

Originally I just deleted this block of code because it seemed invalid (and clang doesn't generate it any more). Prodding further, a better solution might be pointing the lines 43-44 dbg.values at the fragment of i1 that they truly correspond to, I'll upload a revised patch that does this.

jmorse updated this revision to Diff 177172.Dec 7 2018, 3:58 AM

In a test, alter the fragment that two dbg.values refer to. I believe they were incorrect in the first place but silently ignored.

aprantl accepted this revision.Dec 7 2018, 8:38 AM
aprantl added inline comments.
test/DebugInfo/X86/pieces-3.ll
40 ↗(On Diff #176996)

Totally possible that this is bitrotted or was never correct to begin with. Thanks for explaining!

This revision is now accepted and ready to land.Dec 7 2018, 8:38 AM
vsk accepted this revision.Dec 7 2018, 8:47 AM

LGTM, thanks! I'd suggest running through debuginfo-tests if you haven't already (ninja check-debuginfo).

This revision was automatically updated to reflect the committed changes.

Looks like this caused a noticeable shrinking of location lists, but that means we're getting more accurate: http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607042.4&highlight_run=117379 !