Page MenuHomePhabricator

[DebugInfo] Don't drop dbg.value's of nullptr

Authored by jmorse on Dec 3 2018, 10:38 AM.



Currently, dbg.value's of "nullptr" are dropped when entering a SelectionDAG -- apparently just because of an oversight when recognising Values that are constant (see PR39787 [0]). This patch adds ConstantPointerNull to the list of constants that can be attached to DBG_VALUEs.

An open question is what to do about address-spaces and platforms where null is nonzero. Currently the code in SelectionDAGBuilder::getValueImpl just assumes that null is zero-valued, and I've read other parts of the code base that believe the default address space always has a zero valued null. To be conservative, I've assigned null to be zero-valued for the default address space only, all the others get "undef" (which is an improvement in itself).

(More could be done on address spaces, but this patch accounts for the common case, and avoids having to ask any hard questions about how address space plumbing really works).

I haven't added an individual test for this behaviour: instead DebugInfo/X86/sdag-dangling-dbgvalue.ll needed updating to represent the new DBG_VALUEs being present. That test should trip if dbg.value of null ever goes away. Alternately I can add a dedicated test if that's desirable.


Diff Detail


Event Timeline

jmorse created this revision.Dec 3 2018, 10:38 AM

I have no answer for the address-space questions. I'd say ask on llvm-dev, as it's worth asking in a more visible forum.
Assuming we are able to say that nullptr is zero for address space zero, the patch seems fine (and modifying an existing test is perfectly fine).

740 ↗(On Diff #176425)

Comment needs a full-stop at the end.

vsk added inline comments.Dec 3 2018, 11:57 AM
739 ↗(On Diff #176425)

getValueImpl() emits a DAG.getConstant(0) regardless of which address space is used.

5305 ↗(On Diff #176425)

I'm curious what happens if V is-a Constant, but not one of these constants (e.g ConstantAggregate). It seems unlikely that V would always be in NodeMap in that case.

dstenb added inline comments.Dec 4 2018, 12:17 AM
5305 ↗(On Diff #176425)

If it is not something that is in NodeMap here, nor resolved later via DanglingDebugInfoMap, then nothing will be emitted, so the preceding dbg value can live longer than expected. There is the following TODO for this:

// TODO: When we get here we will either drop the dbg.value completely, or
// we try to move it forward by letting it dangle for awhile. So we should
// probably add an extra DbgValue to the DAG here, with a reference to
// "noreg", to indicate that we have lost the debug location for the
// variable.
jmorse added inline comments.Dec 4 2018, 1:58 AM
739 ↗(On Diff #176425)

Yeah, I've been (perhaps overly) cautious here -- getValueImpl _has_ to pick a value, but DebugInfo can emit unknown/undef. I'll stick this in the llvm-dev email too.

jmorse updated this revision to Diff 177175.Dec 7 2018, 4:20 AM

Remove address-space-zero filter on emitting zero-bit-value nulls

jmorse updated this revision to Diff 177176.Dec 7 2018, 4:21 AM

Correct a comment, whoops.

jmorse marked 4 inline comments as done.Dec 7 2018, 4:27 AM
jmorse added inline comments.
739 ↗(On Diff #176425)

The opinion on the mailing list appears to be that all address spaces have zero-bit-valued null constants, so I've now removed the address space filter.

aprantl accepted this revision.Dec 7 2018, 8:41 AM

LGTM if @vsk's question is answered satisfactorily.

739 ↗(On Diff #177176)

How about: //Note: This assumes that all nullptr constants are zero-valued.

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

Looks great, thanks.

jmorse marked 2 inline comments as done.Dec 10 2018, 3:42 AM
jmorse added inline comments.
739 ↗(On Diff #177176)

Replacing with this in the commit.

This revision was automatically updated to reflect the committed changes.
jmorse marked an inline comment as done.