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

[jmorse]

Summary

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.

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

[probinson]

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).

lib/CodeGen/SelectionDAG/InstrEmitter.cpp

740

Comment needs a full-stop at the end.

[vsk]

lib/CodeGen/SelectionDAG/InstrEmitter.cpp

739

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

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5305

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]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5305

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]

lib/CodeGen/SelectionDAG/InstrEmitter.cpp

739

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]

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

[jmorse]

Correct a comment, whoops.

[jmorse]

lib/CodeGen/SelectionDAG/InstrEmitter.cpp

739

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]

LGTM if @vsk's question is answered satisfactorily.

lib/CodeGen/SelectionDAG/InstrEmitter.cpp

6

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

[vsk]

Looks great, thanks.

[jmorse]

lib/CodeGen/SelectionDAG/InstrEmitter.cpp

6

Replacing with this in the commit.

lib/CodeGen/SelectionDAG/InstrEmitter.cpp

 //==--- InstrEmitter.cpp - Emit MachineInstrs for the SelectionDAG class ---==//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This implements the Emit routines for the SelectionDAG class, which creates
 // MachineInstrs based on the decisions of the SelectionDAG instruction
 // selection.
 //
 //===----------------------------------------------------------------------===//
if (SD->getKind() == SDDbgValue::SDNODE) {
     const Value *V = SD->getConst();
     if (const ConstantInt *CI = dyn_cast<ConstantInt>(V)) {
       if (CI->getBitWidth() > 64)
         MIB.addCImm(CI);
       else
         MIB.addImm(CI->getSExtValue());
     } else if (const ConstantFP *CF = dyn_cast<ConstantFP>(V)) {
       MIB.addFPImm(CF);
+    } else if (isa<ConstantPointerNull>(V) &&
+               V->getType()->getPointerAddressSpace() == 0) {
+      // For now, assume null pointers are zero-valued
+      MIB.addImm(0);
     } else {
       // Could be an Undef.  In any case insert an Undef so we can see what we
       // dropped.
       MIB.addReg(0U);
     }
   } else {
     // Insert an Undef so we can see what we dropped.
     MIB.addReg(0U);

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

case Intrinsic::dbg_value: {
     DILocalVariable *Variable = DI.getVariable();
     DIExpression *Expression = DI.getExpression();
     dropDanglingDebugInfo(Variable, Expression);
     const Value *V = DI.getValue();
     if (!V)
       return nullptr;
 
     SDDbgValue *SDV;
-    if (isa<ConstantInt>(V) || isa<ConstantFP>(V) || isa<UndefValue>(V)) {
+    if (isa<ConstantInt>(V) || isa<ConstantFP>(V) || isa<UndefValue>(V) ||
+        isa<ConstantPointerNull>(V)) {
       SDV = DAG.getConstantDbgValue(Variable, Expression, V, dl, SDNodeOrder);
       DAG.AddDbgValue(SDV, nullptr, false);
       return nullptr;
     }
 
     // Do not use getValue() in here; we don't want to generate code at
     // this point if it hasn't been done yet.
     SDValue N = NodeMap[V];

test/DebugInfo/X86/sdag-dangling-dbgvalue.ll

 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.4.0"
 
 %struct.SS = type { i32, i32 }
 
 @S = global %struct.SS { i32 23, i32 -17 }, align 4, !dbg !0
 
 ; Verify that the def comes before the for foo1.
-; TODO: Currently dbg.value for bar1 is dropped(?), is that expected?
 define i32 @test1() local_unnamed_addr #0 !dbg !17 {
 ; CHECK-LABEL: bb.0.entry1
+; CHECK-NEXT:    DBG_VALUE 0, $noreg, ![[BAR1]], !DIExpression()
 ; CHECK-NEXT:    [[REG1:%[0-9]+]]:gr64 =
 ; CHECK-NEXT:    DBG_VALUE [[REG1]], $noreg, ![[FOO1]], !DIExpression()
 entry1:
   call void @llvm.dbg.value(metadata %struct.SS* @S, metadata !20, metadata !DIExpression()), !dbg !23
   call void @llvm.dbg.value(metadata %struct.SS* null, metadata !22, metadata !DIExpression()), !dbg !24
   ret i32 ptrtoint (%struct.SS* @S to i32), !dbg !25
 }
 

 ; CHECK-NEXT:    DBG_VALUE [[REG3]], $noreg, ![[FOO3]], !DIExpression()
 entry3:
   call void @llvm.dbg.value(metadata %struct.SS* @S, metadata !36, metadata !DIExpression()), !dbg !38
   call void @llvm.dbg.value(metadata %struct.SS* @S, metadata !35, metadata !DIExpression()), !dbg !37
   ret i32 add (i32 ptrtoint (%struct.SS* @S to i32), i32 ptrtoint (%struct.SS* @S to i32)), !dbg !39
 }
 
 ; Verify that the def comes before the for bar4.
-; TODO: Currently dbg.value for foo4 is dropped. It is set to null and not
-;       used. Just like in test1 it can be discussed if there should be a
-;       DBG_VALUE for foo4 here.
 define i32 @test4() local_unnamed_addr #0 !dbg !40 {
 ; CHECK-LABEL: bb.0.entry4
+; CHECK-NEXT:    DBG_VALUE 0, $noreg, ![[FOO4]], !DIExpression()
 ; CHECK-NEXT:    [[REG4:%[0-9]+]]:gr64 =
 ; CHECK-NEXT:    DBG_VALUE [[REG4]], $noreg, ![[BAR4]], !DIExpression()
 entry4:
   call void @llvm.dbg.value(metadata %struct.SS* @S, metadata !42, metadata !DIExpression()), !dbg !44
   call void @llvm.dbg.value(metadata %struct.SS* @S, metadata !43, metadata !DIExpression()), !dbg !45
   call void @llvm.dbg.value(metadata %struct.SS* null, metadata !42, metadata !DIExpression()), !dbg !44
   ret i32 ptrtoint (%struct.SS* @S to i32), !dbg !46
 }
 
 ; Verify that we do not get a DBG_VALUE that maps foo5 to @S here.
-; TODO: foo5 is set to null, and it is not really used. Just like in test1 it
-;       can be discussed if there should be a DBG_VALUE for foo5 here.
 define i32 @test5() local_unnamed_addr #0 !dbg !47 {
 ; CHECK-LABEL: bb.0.entry5:
+; CHECK-NEXT:    DBG_VALUE 0, $noreg, ![[FOO5]], !DIExpression()
 ; CHECK-NEXT:    [[REG5:%[0-9]+]]:gr64 =
 ; CHECK-NEXT:    DBG_VALUE [[REG5]], $noreg, ![[BAR5]], !DIExpression()
 ; CHECK-NOT:     DBG_VALUE [[REG5]], $noreg, ![[FOO5]], !DIExpression()
 ; CHECK:         RET
 entry5:
   call void @llvm.dbg.value(metadata %struct.SS* @S, metadata !49, metadata !DIExpression()), !dbg !51
   call void @llvm.dbg.value(metadata %struct.SS* @S, metadata !50, metadata !DIExpression()), !dbg !52
   call void @llvm.dbg.value(metadata %struct.SS* null, metadata !50, metadata !DIExpression()), !dbg !52