DebugInfo: SROA on references should result in undef dbg.values

[dblaikie]

Summary

Investigating a buggy/bit-rotten test case (llvm/test/DebugInfo/X86/dbg-large-unsigned-const.ll) I stumbled across a bug in SROA+debug info.

Given this source:

long glbl;
inline __attribute__((always_inline)) bool func(const long  &l) {
  return l == glbl;
}
int main() {
  return func(9223372036854775807);
}

The function is inlined, SROA removes the alloca and uses the constant directly (sorry, the constant could be smaller, it doesn't really matter - just some history from the test case I was investigating) and then replaces the dbg.value that references the memory location with a dbg.value using the constant directly.

What this does is cause the debug info to get very confused - the type of the variable is a reference, that means its value is the pointer to the value, not the value itself. So the debugger tries to dereference the constant.

No good.

It looks as if the entire chunk of code in AllocaPromoter::updateDebugInfo dedicated to looking through the loads and stores and creating a new dbg.value from that should be replaced with code to create dbg.values of undef. For situations where we still end up with a memory operand (we removed one layer of indirection, but there's still another) we can probably represent this in debug info one day. But for registers and constants I don't think there's anything we can do to represent their value in DWARF when we've lost the indirection they required.

Does this make sense to people?

Oh, and the other changes in this patch are one attempt at addressing a knock-on issue that fixing this bug triggers: since the parameter is undef, it goes missing later on. By preserving the dbg_values a bit further we don't lose the parameter anymore, even though we can't describe its location.

[dblaikie]

Hmm - never quite sure how to get this to send to the list properly. Hopefully this does it.

[dblaikie]

I've relaxed an assert in http://llvm.org/viewvc/llvm-project?rev=209240&view=rev to workaround this issue, but it'd be good to get it fixed.

I'll try to add a commitable test case here at some point.

[aprantl]

As mentioned earlier, I believe that DW_TAG_struct should be part of that list.

struct S { unsigned int c; };
unsigned int foo() {
struct S s = { 42 };
return s.c;
}

And imagine a slightly smarter SROA turns this into:

; ModuleID = 's.c'
target triple = "x86_64-apple-macosx10.9.0"

%struct.S = type { i32 }

; Function Attrs: nounwind readnone ssp uwtable
define i32 @foo() #0 {
entry:

tail call void @llvm.dbg.value(metadata !17, i64 0, metadata !10), !dbg !18
ret i32 42, !dbg !19

}

; Function Attrs: nounwind readnone
declare void @llvm.dbg.value(metadata, i64, metadata) #1

attributes #0 = { nounwind readnone ssp uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
attributes #1 = { nounwind readnone }

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!14, !15}
!llvm.ident = !{!16}

!0 = metadata !{i32 786449, metadata !1, i32 12, metadata !"clang version 3.5.0 ", i1 true, metadata !"", i32 0, metadata !2, metadata !2, metadata !3, metadata !2, metadata !2, metadata !"", i32 1} ; [ DW_TAG_compile_unit ] [/Volumes/Data/llvm/_build.ninja.debug/s.c] [DW_LANG_C99]
!1 = metadata !{metadata !"s.c", metadata !"/Volumes/Data/llvm/_build.ninja.debug"}
!2 = metadata !{}
!3 = metadata !{metadata !4}
!4 = metadata !{i32 786478, metadata !1, metadata !5, metadata !"foo", metadata !"foo", metadata !"", i32 2, metadata !6, i1 false, i1 true, i32 0, i32 0, null, i32 0, i1 true, i32 ()* @foo, null, null, metadata !9, i32 2} ; [ DW_TAG_subprogram ] [line 2] [def] [foo]
!5 = metadata !{i32 786473, metadata !1} ; [ DW_TAG_file_type ] [/Volumes/Data/llvm/_build.ninja.debug/s.c]
!6 = metadata !{i32 786453, i32 0, null, metadata !"", i32 0, i64 0, i64 0, i64 0, i32 0, null, metadata !7, i32 0, null, null, null} ; [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]
!7 = metadata !{metadata !8}
!8 = metadata !{i32 786468, null, null, metadata !"unsigned int", i32 0, i64 32, i64 32, i64 0, i32 0, i32 7} ; [ DW_TAG_base_type ] [unsigned int] [line 0, size 32, align 32, offset 0, enc DW_ATE_unsigned]
!9 = metadata !{metadata !10}
!10 = metadata !{i32 786688, metadata !4, metadata !"s", metadata !5, i32 3, metadata !11, i32 0, i32 0} ; [ DW_TAG_auto_variable ] [s] [line 3]
!11 = metadata !{i32 786451, metadata !1, null, metadata !"S", i32 1, i64 32, i64 32, i32 0, i32 0, null, metadata !12, i32 0, null, null, null} ; [ DW_TAG_structure_type ] [S] [line 1, size 32, align 32, offset 0] [def] [from ]
!12 = metadata !{metadata !13}
!13 = metadata !{i32 786445, metadata !1, metadata !11, metadata !"c", i32 1, i64 32, i64 32, i64 0, i32 0, metadata !8} ; [ DW_TAG_member ] [c] [line 1, size 32, align 32, offset 0] [from unsigned int]
!14 = metadata !{i32 2, metadata !"Dwarf Version", i32 2}
!15 = metadata !{i32 2, metadata !"Debug Info Version", i32 1}
!16 = metadata !{metadata !"clang version 3.5.0 "}
!17 = metadata !{i32 42}
!18 = metadata !{i32 3, i32 0, metadata !4, null}
!19 = metadata !{i32 4, i32 0, metadata !4, null}

$ bin/llc s.ll
Assertion failed: (T == dwarf::DW_TAG_typedef || T == dwarf::DW_TAG_const_type || T == dwarf::DW_TAG_volatile_type || T == dwarf::DW_TAG_restrict_type || T == dwarf::DW_TAG_enumeration_type), function isUnsignedDIType, file ../lib/CodeGen/AsmPrinter/DwarfUnit.cpp, line 766.

[dblaikie]

Ping (Chandler, mostly)

[echristo]

The debug info side of things sounds ok for now, it might be nice to figure out some way to represent the value in the future with the type. Haven't looked at the actual code yet.

[dblaikie]

In D3714#11, @echristo wrote:

The debug info side of things sounds ok for now, it might be nice to figure out some way to represent the value in the future with the type.

Yeah - I believe that's in the territory of DWARF extensions/improvements. For now, so far as I understand DWARF, you can only describe the value of the variable (a pointer in this case, and something we don't have anymore - we'd need the opposite of DW_OP_deref (DW_OP_addr_of?)). Not sure if this overlaps with the broader problem of things like Argument Promotion, but I think it does.

Prior to that, though, we may need a better structured representation of addresses - I know, for example, that a screwed up when fixing the pass-by-non-trivial-copy (fixing the bug where we described those parameters as references instead of values) - by tagging the DIVariable itself with the "indirect" flag. That's no good in cases like this - we'll need to be able to add/remove such description on a per-variable-instance basis, so it should go in the dbg.value/declare/whatnot in a way that things like ArgPromo can see it and remove it if they get fancy (hey, we inlined your copy ctor/dtor and found nothing interesting, so we promoted your pass-by-non-trivial-copy to a trivial copy, but we can still describe the location of that variable, etc... - we still have the issue with "how does the debugger call this function now that it doesn't match the ABI" which we've also bantered about a bit)

[aprantl]

I've been wishing for a DW_OP_addr_of before. But, I was just browsing through the DWARF spec and came across:

DW_OP_stack_value
The DW_OP_stack_value operation specifies that the object does not exist in memory but its value is nonetheless known and is at the top of the DWARF expression stack. In this form of location description, the DWARF expression represents the actual value of the object, rather than its location. The DW_OP_stack_value operation terminates the expression.

This *could* be interpreted as the DW_OP_addr_of we're looking for.

I set aside some time this week to experiment with moving the complex expression part of DIVariable into an additional operand of dbg.value. My main motivation is reducing the memory footprint of OpPiece exprs. This will be a massive refactoring, but I'll keep you updated on how that goes.

[chandlerc]

Not sure there is anything you need from *me* here. This part of SROA is part I just ripped from the old one, and those of you working on debug info probably understand it much better.

Generally, I'd really like us to figure out how to make source-level local variables which happen to be LLVM registers when lowered accessible from the debugger. stack_value style stuff seems the right kind of approach, but what do I know.

Let me know if I can help here, but removing myself as a rewiever for now to clear out my dashboard.

[dblaikie]

Original bug doesn't seem to reproduce, at least. (the variable is omitted entirely, instead of a reference's location being described by a constant) - presumably fixed/removed/etc in the intervening years.

lib/CodeGen/LiveDebugVariables.cpp

for (unsigned i = 0; i != Defs.size(); ++i) {
 
     // For physregs, use the live range of the first regunit as a guide.
     unsigned Unit = *MCRegUnitIterator(Loc.getReg(), &TRI);
     LiveRange *LR = &LIS.getRegUnit(Unit);
     const VNInfo *VNI = LR->getVNInfoAt(Idx);
     // Don't track copies from physregs, it is too expensive.
     extendDef(Idx, LocNo, LR, VNI, nullptr, LIS, MDT, UVS);
   }
-
-  // Finally, erase all the undefs.
-  for (LocMap::iterator I = locInts.begin(); I.valid();)
-    if (I.value() == ~0u)
-      I.erase();
-    else
-      ++I;
 }
 
 void LDVImpl::computeIntervals() {
   for (unsigned i = 0, e = userValues.size(); i != e; ++i) {
     UserValueScopes UVS(userValues[i]->getDebugLoc(), LS);
     userValues[i]->computeIntervals(MF->getRegInfo(), *TRI, *LIS, *MDT, UVS);
     userValues[i]->mapVirtRegs(this);
   }
DebugLoc UserValue::findDebugLoc() {
   dl = DebugLoc();
   return D;
 }
 void UserValue::insertDebugValue(MachineBasicBlock *MBB, SlotIndex Idx,
                                  unsigned LocNo,
                                  LiveIntervals &LIS,
                                  const TargetInstrInfo &TII) {
   MachineBasicBlock::iterator I = findInsertLocation(MBB, Idx, LIS);
-  MachineOperand &Loc = locations[LocNo];
+  MachineOperand *Loc = LocNo == ~0u ? nullptr : &locations[LocNo];
   ++NumInsertedDebugValues;
 
-  if (Loc.isReg())
+  if (!Loc)
+    BuildMI(*MBB, I, findDebugLoc(), TII.get(TargetOpcode::DBG_VALUE),
+            IsIndirect, 0, offset, variable);
+  else if (Loc->isReg())
     BuildMI(*MBB, I, findDebugLoc(), TII.get(TargetOpcode::DBG_VALUE),
-            IsIndirect, Loc.getReg(), offset, variable);
+            IsIndirect, Loc->getReg(), offset, variable);
   else
     BuildMI(*MBB, I, findDebugLoc(), TII.get(TargetOpcode::DBG_VALUE))
-      .addOperand(Loc).addImm(offset).addMetadata(variable);
+      .addOperand(*Loc).addImm(offset).addMetadata(variable);
 }
 
 void UserValue::emitDebugValues(VirtRegMap *VRM, LiveIntervals &LIS,
                                 const TargetInstrInfo &TII) {
   MachineFunction::iterator MFEnd = VRM->getMachineFunction().end();
 
   for (LocMap::const_iterator I = locInts.begin(); I.valid();) {
     SlotIndex Start = I.start();

lib/Transforms/Scalar/SROA.cpp

for (SmallVectorImpl<DbgDeclareInst *>::const_iterator I = DDIs.begin(),
       if (StoreInst *SI = dyn_cast<StoreInst>(Inst))
         ConvertDebugDeclareToDebugValue(DDI, SI, DIB);
       else if (LoadInst *LI = dyn_cast<LoadInst>(Inst))
         ConvertDebugDeclareToDebugValue(DDI, LI, DIB);
     }
     for (SmallVectorImpl<DbgValueInst *>::const_iterator I = DVIs.begin(),
            E = DVIs.end(); I != E; ++I) {
       DbgValueInst *DVI = *I;
-      Value *Arg = nullptr;
-      if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
-        // If an argument is zero extended then use argument directly. The ZExt
-        // may be zapped by an optimization pass in future.
-        if (ZExtInst *ZExt = dyn_cast<ZExtInst>(SI->getOperand(0)))
-          Arg = dyn_cast<Argument>(ZExt->getOperand(0));
-        else if (SExtInst *SExt = dyn_cast<SExtInst>(SI->getOperand(0)))
-          Arg = dyn_cast<Argument>(SExt->getOperand(0));
-        if (!Arg)
-          Arg = SI->getValueOperand();
-      } else if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
-        Arg = LI->getPointerOperand();
-      } else {
-        continue;
-      }
-      Instruction *DbgVal =
-        DIB.insertDbgValueIntrinsic(Arg, 0, DIVariable(DVI->getVariable()),
-                                     Inst);
+      Value *Arg = UndefValue::get(DVI->getValue()->getType());
+      Instruction *DbgVal = DIB.insertDbgValueIntrinsic(
+          Arg, 0, DIVariable(DVI->getVariable()), Inst);
       DbgVal->setDebugLoc(DVI->getDebugLoc());
     }
   }
 };
 } // end anon namespace
 
 
 namespace {