[codeview] support more DW_OPs for more complete debug info

[inglorion]

Summary

Some variables show up in Visual Studio as "optimized out" even in -O0
-Od builds. This change fixes two issues that would cause this to
happen. The first issue is that not all DIExpressions we generate were
recognized by the CodeView writer. This has been addressed by adding
support for DW_OP_constu, DW_OP_minus, and DW_OP_plus. The second
issue is that we had no way to encode DW_OP_deref in CodeView. We get
around that by changinge the type we encode in the debug info to be
a reference to the type in the source code.

This fixes PR34261.

[inglorion]

(Still a work in progress.)

[inglorion]

Added support for DW_OP_deref, and a test case. This is now ready for review.

[inglorion]

arc diff --verbatim

[aprantl]

test

llvm/test/CodeGen/X86/codeview-nrvo.ll

41

We usually strip out all quoted attributes from testcases to make them more future-proof / readable.

73

Shouldn't the testcase include a complex expression?

[rnk]

llvm/test/CodeGen/X86/codeview-nrvo.ll

73

The dbg.declare gets lowered to a DEBUG_VALUE MI instruction with a DW_OP_deref. The complex DIExpression would be more obvious if this were a .mir test case. We probably want all of an IR NRVO test case, an MIR NRVO test case, and hand-crafted IR test cases with complex DIExpressions.

[rnk]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

1009

Please factor out the DIExpression parsing into a helper that extracts the fragment info, Deref, Supported, and Offset.

In the long run, we might want to remodel DIExpression so that it encodes this information directly. Right now DIExpressions feel very "stringly-typed": they're just a sequence of integer opcodes and arguments that don't have any clear relationship to each other. Every bit of code that touches them rolls its own parsing.

[aprantl]

In the long run, we might want to remodel DIExpression so that it encodes this information directly. Right now DIExpressions feel very "stringly-typed": they're just a sequence of integer opcodes and arguments that don't have any clear relationship to each other. Every bit of code that touches them rolls its own parsing.

I think it might make sense to separate out the DW_OP_LLVM_fragment, but otherwise the DIExpression operand iterators are there to abstract from the array of opcodes implementation.

[inglorion]

Thanks for taking a look!

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

1009

In the long run, we might want to remodel DIExpression so that it encodes this information directly.

I was thinking that, too. It's great to be able to express arbitrarily complex expressions, but if we generate only simple ones and backends handle only a few specific forms anyway, we might as well simplify our lives and avoid the extra parsing.

For now, I'll factor this out. I was actually planning to do that today anyway.

[inglorion]

llvm/test/CodeGen/X86/codeview-nrvo.ll

73

How can I make the DIExpressions more obvious in the MIR? I'm new to MIR, so I'm not sure what the best way to generate it is. I did llc -O0 -stop-after=prologepilog, which gives me machine IR that has the debug records that I want, but the MIR still has dbg.declare and empty DIExpressions, so I'm not sure that's what we want. I tried several other passes, with similar results. At some point, the translation from dbg.declare to dbg.value must happen, and there must be a point where we have DIExpressions with arithmetic and deref, but I can't seem to find an invocation that dumps this.

[aprantl]

Which pass generates the DIExpressions you are interested in? If it is a MIR pass, you'll need to stop-after that pass, and the DIExpressions will be referenced by the DBG_VALUE instructions in the MIR, and should appear in the IR dump metadata (but not in the IR code portion).

[rnk]

llvm/test/CodeGen/X86/codeview-nrvo.ll

73

So, for some reason -stop-after=prologepilog doesn't print any DBG_VALUE instructions. I'm not sure why, but we weren't able to use that to make a test case. I think we're find with just the original NRVO test case plus some handcrafted IR DIExpression test cases.

[inglorion]

How is this for the testcase? It checks positive offsets, negative offsets, and deref.

[aprantl]

Testcase seems fine as far as I am concerned. Is it possible to include a comment with instructions for how to regenerate the test from source if that becomes necessary in the future?

[rnk]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

948

Shouldn't need this with a member initializer.

1094

Are you sure this always works? I would expect it to assert sometimes. We haven't run the assembler yet. We can't in general know the distance between two labels, even if they are in the same section. If we can, we can make a large number of simplifications to this code.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h

97

Let's initialize this to false so we don't need to remember it everywhere.

[inglorion]

@aprantl, I would like to do that, but this doesn't really correspond to any source anymore. I suppose you could take the LLVM IR part of it, take the offsets out of the DIExpressions, and then run it through llc -O0 -stop-after=prologepilog. Then you can edit the DIExpressions to be what you need them to be. Would it be useful to include that in a comment?

[aprantl]

Sometimes I say something like "compiled from the following source with -O0 and manually edited", I'll leave it up to your judgement.

[inglorion]

• rewrite non-deref ranges for variables we changed to references
• factored out interpretation of debug info expressions

[inglorion]

removed stray assert

[inglorion]

• updated COFF/pieces.ll test to check for records we now emit
• refactored to avoid iterating twice

[inglorion]

Thanks for the feedback! I think I incorporated all of it. Does this look good now?

[rnk]

llvm/lib/CodeGen/AsmPrinter/VariableLocation.cpp

33–50 ↗

(On Diff #112652)

I was thinking about how to improve DIExpression over the weekend, and one of the ideas I came up with is that we should "canonicalize" them on construction, and we should add a high-level opcode like DW_OP_LLVM_fragment that expresses DW_OP_deref plus a signed offset. That would eliminate the need for a ton of DW_OP_const/minus/plus parsing and centralize the logic in the DIExpression opcode canonicalization phase. CodeGen and mid-level passes would be able to reason about chains of loads with offsets, or they would give up if the expression is too complicated.

This is an idle thought for a future patch, though...

llvm/lib/CodeGen/AsmPrinter/VariableLocation.h

1 ↗

(On Diff #112652)

IMO we should call this DbgVariableLocation. I'd recommend moving it to DebugHandlerBase.h for now. That's the current home for common functionality between dwarf + CV emission.

13–14 ↗

(On Diff #112652)

These are not system headers, these should be quoted.

llvm/test/DebugInfo/COFF/pieces.ll

182 ↗

(On Diff #112652)

Cool. :)

[inglorion]

llvm/lib/CodeGen/AsmPrinter/VariableLocation.cpp

33–50 ↗

(On Diff #112652)

Yeah, I was thinking along similar lines: Have a DW_OP that represents what we commonly want, and keep the existing format for DIExpression so we don't have to change the representation and we can still express complex things. Like you said, that's for a future patch, though.

[inglorion]

changes requested by @rnk

[rnk]

lgtm

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.h

41 ↗

(On Diff #112975)

Make this unsigned, otherwise it will only store the values 0 and -1, which can be confusing from time to time.

[inglorion]

changed 1-bit ints to unsigneds, ran clang-format

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h

static LocalVarDefRange createDefRangeGeneral(uint16_t CVRegister,
                                                 bool InMemory, int Offset,
                                                 bool IsSubfield,
                                                 uint16_t StructOffset);
 
   /// Similar to DbgVariable in DwarfDebug, but not dwarf-specific.
   struct LocalVariable {
     const DILocalVariable *DIVar = nullptr;
     SmallVector<LocalVarDefRange, 1> DefRanges;
+    bool Deref;
   };
 
   struct InlineSite {
     SmallVector<LocalVariable, 1> InlinedLocals;
     SmallVector<const DILocation *, 1> ChildSites;
     const DISubprogram *Inlinee = nullptr;
 
     /// The ID of the inline site or function used with .cv_loc. Not a type
class LLVM_LIBRARY_VISIBILITY CodeViewDebug : public DebugHandlerBase {
   /// Emits an S_LOCAL record and its associated defined ranges.
   void emitLocalVariable(const LocalVariable &Var);
 
   /// Translates the DIType to codeview if necessary and returns a type index
   /// for it.
   codeview::TypeIndex getTypeIndex(DITypeRef TypeRef,
                                    DITypeRef ClassTyRef = DITypeRef());
 
+  codeview::TypeIndex getTypeIndexForReferenceTo(DITypeRef TypeRef);
+
   codeview::TypeIndex getMemberFunctionType(const DISubprogram *SP,
                                             const DICompositeType *Class);
 
   codeview::TypeIndex getScopeIndex(const DIScope *Scope);
 
   codeview::TypeIndex getVBPTypeIndex();
 
   void addToUDTs(const DIType *Ty, codeview::TypeIndex TI);

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

for (const InsnRange &Range : Scope->getRanges()) {
       const MCSymbol *End = getLabelAfterInsn(Range.second);
       End = End ? End : Asm->getFunctionEnd();
       DefRange.Ranges.emplace_back(Begin, End);
     }
 
     LocalVariable Var;
     Var.DIVar = VI.Var;
     Var.DefRanges.emplace_back(std::move(DefRange));
+    Var.Deref = false;
     recordLocalVariable(std::move(Var), VI.Loc->getInlinedAt());
   }
 }
 
 void CodeViewDebug::collectVariableInfo(const DISubprogram *SP) {
   DenseSet<InlinedVariable> Processed;
   // Grab the variable info that was squirreled away in the MMI side-table.
   collectVariableInfoFromMFTable(Processed);
for (const auto &I : DbgValues) {
     // If variable scope is not found then skip this variable.
     if (!Scope)
       continue;
 
     LocalVariable Var;
     Var.DIVar = DIVar;
 
     // Calculate the definition ranges.
+    // Because we cannot express DW_OP_deref in CodeView directly,
+    // we use a trick: we encode the type as a reference to the
+    // real type. Unfortunately, this means we cannot mix deref
+    // defranges with non-deref defranges. We track how much of
+    // the code is covered by each, and use the variant which
+    // gives us the most coverage.
+    uint64_t DerefLength = 0;
+    uint64_t NonDerefLength = 0;
+    SmallVector<LocalVarDefRange, 1> DerefDefRanges;
+    SmallVector<LocalVarDefRange, 1> NonDerefDefRanges;
     for (auto I = Ranges.begin(), E = Ranges.end(); I != E; ++I) {
       const InsnRange &Range = *I;
       const MachineInstr *DVInst = Range.first;
       assert(DVInst->isDebugValue() && "Invalid History entry");
       const DIExpression *DIExpr = DVInst->getDebugExpression();
       bool InMemory = DVInst->getOperand(1).isImm();
       bool IsSubfield = false;
       unsigned StructOffset = 0;
-      // Recognize a +Offset expression.
+      Optional<DIExpression::FragmentInfo> Fragment;
+      // Read DIExpr to get the offset (if any).
+      // We only handle expressions generated by DIExpression::appendOffset,
+      // which doesn't require a full stack machine.
       int Offset = 0;
-      DIExpressionCursor Ops(DIExpr);
-      auto Op = Ops.peek();
-      if (Op && Op->getOp() == dwarf::DW_OP_plus_uconst) {
-        Offset = Op->getArg(0);
-        Ops.take();
+      bool Supported = true;
+      bool Deref = false;
+      auto Op = DIExpr->expr_op_begin();
+      while (Supported && Op != DIExpr->expr_op_end()) {
+        switch (Op->getOp()) {
+        case dwarf::DW_OP_constu: {
+          int Value = Op->getArg(0);
+          ++Op;
+          if (Op != DIExpr->expr_op_end()) {
+            switch (Op->getOp()) {
+            case dwarf::DW_OP_minus:
+              Offset -= Value;
+              break;
+            case dwarf::DW_OP_plus:
+              Offset += Value;
+            default:
+              continue;
             }
+          }
+        } break;
+        case dwarf::DW_OP_plus_uconst:
+          Offset += Op->getArg(0);
+          break;
+        case dwarf::DW_OP_LLVM_fragment:
+          Fragment = {Op->getArg(1), Op->getArg(0)};
+          break;
+        case dwarf::DW_OP_deref:
+          Deref = true;
+          break;
+        default:
+          Supported = false;
+        }
+        ++Op;
+      }
+
+      // If we don't know how to handle this range, skip past it.
+      if (!Supported || (Offset && !InMemory))
+        continue;
 
       // Handle fragments.
-      auto Fragment = Ops.getFragmentInfo();
       if (Fragment) {
         IsSubfield = true;
         StructOffset = Fragment->OffsetInBits / 8;
       }
-      // Ignore unrecognized exprs.
-      if (Ops.peek() && Ops.peek()->getOp() != dwarf::DW_OP_LLVM_fragment)
-        continue;
-      if (!InMemory && Offset)
-        continue;
-
       // Bail if operand 0 is not a valid register. This means the variable is a
       // simple constant, or is described by a complex expression.
       // FIXME: Find a way to represent constant variables, since they are
       // relatively common.
       unsigned Reg =
           DVInst->getOperand(0).isReg() ? DVInst->getOperand(0).getReg() : 0;
       if (Reg == 0)
         continue;
 
+      SmallVector<LocalVarDefRange, 1> &DefRanges =
+          Deref ? DerefDefRanges : NonDerefDefRanges;
+      uint64_t &Length = Deref ? DerefLength : NonDerefLength;
+
       // Handle the two cases we can handle: indirect in memory and in register.
       unsigned CVReg = TRI->getCodeViewRegNum(Reg);
       {
         LocalVarDefRange DR;
         DR.CVRegister = CVReg;
         DR.InMemory = InMemory;
         DR.DataOffset = Offset;
         DR.IsSubfield = IsSubfield;
         DR.StructOffset = StructOffset;
 
-        if (Var.DefRanges.empty() ||
-            Var.DefRanges.back().isDifferentLocation(DR)) {
-          Var.DefRanges.emplace_back(std::move(DR));
+        if (DefRanges.empty() || DefRanges.back().isDifferentLocation(DR)) {
+          DefRanges.emplace_back(std::move(DR));
         }
       }
 
       // Compute the label range.
       const MCSymbol *Begin = getLabelBeforeInsn(Range.first);
       const MCSymbol *End = getLabelAfterInsn(Range.second);
       if (!End) {
         // This range is valid until the next overlapping bitpiece. In the
         // common case, ranges will not be bitpieces, so they will overlap.
         auto J = std::next(I);
         while (J != E &&
                !fragmentsOverlap(DIExpr, J->first->getDebugExpression()))
           ++J;
         if (J != E)
           End = getLabelBeforeInsn(J->first);
         else
           End = Asm->getFunctionEnd();
       }
+      assert(End);
+      Length += End->getOffset() - Begin->getOffset();
 
       // If the last range end is our begin, just extend the last range.
       // Otherwise make a new range.
       SmallVectorImpl<std::pair<const MCSymbol *, const MCSymbol *>> &Ranges =
-          Var.DefRanges.back().Ranges;
+          DefRanges.back().Ranges;
       if (!Ranges.empty() && Ranges.back().second == Begin)
         Ranges.back().second = End;
       else
         Ranges.emplace_back(Begin, End);
 
       // FIXME: Do more range combining.
     }
 
+    if (DerefLength > NonDerefLength) {
+      Var.Deref = true;
+      Var.DefRanges = std::move(DerefDefRanges);
+    } else {
+      Var.Deref = false;
+      Var.DefRanges = std::move(NonDerefDefRanges);
+    }
+
     recordLocalVariable(std::move(Var), InlinedAt);
   }
 }
 
 void CodeViewDebug::beginFunctionImpl(const MachineFunction *MF) {
   const Function *GV = MF->getFunction();
   assert(FnDebugInfo.count(GV) == false);
   CurFn = &FnDebugInfo[GV];
TypeIndex CodeViewDebug::getTypeIndex(DITypeRef TypeRef, DITypeRef ClassTyRef) {
   if (I != TypeIndices.end())
     return I->second;
 
   TypeLoweringScope S(*this);
   TypeIndex TI = lowerType(Ty, ClassTy);
   return recordTypeIndexForDINode(Ty, TI, ClassTy);
 }
 
+TypeIndex CodeViewDebug::getTypeIndexForReferenceTo(DITypeRef TypeRef) {
+  DIType *Ty = TypeRef.resolve();
+  PointerRecord PR(getTypeIndex(Ty),
+                   getPointerSizeInBytes() == 8 ? PointerKind::Near64
+                                                : PointerKind::Near32,
+                   PointerMode::LValueReference, PointerOptions::None,
+                   Ty->getSizeInBits() / 8);
+  return TypeTable.writeKnownType(PR);
+}
+
 TypeIndex CodeViewDebug::getCompleteTypeIndex(DITypeRef TypeRef) {
   const DIType *Ty = TypeRef.resolve();
 
   // The null DIType is the void type. Don't try to hash it.
   if (!Ty)
     return TypeIndex::Void();
 
   // If this is a non-record type, the complete type index is the same as the
void CodeViewDebug::emitLocalVariable(const LocalVariable &Var) {
 
   LocalSymFlags Flags = LocalSymFlags::None;
   if (Var.DIVar->isParameter())
     Flags |= LocalSymFlags::IsParameter;
   if (Var.DefRanges.empty())
     Flags |= LocalSymFlags::IsOptimizedOut;
 
   OS.AddComment("TypeIndex");
-  TypeIndex TI = getCompleteTypeIndex(Var.DIVar->getType());
+  TypeIndex TI = Var.Deref ? getTypeIndexForReferenceTo(Var.DIVar->getType())
+                           : getCompleteTypeIndex(Var.DIVar->getType());
   OS.EmitIntValue(TI.getIndex(), 4);
   OS.AddComment("Flags");
   OS.EmitIntValue(static_cast<uint16_t>(Flags), 2);
   // Truncate the name so we won't overflow the record length field.
   emitNullTerminatedSymbolName(OS, Var.DIVar->getName());
   OS.EmitLabel(LocalEnd);
 
   // Calculate the on disk prefix of the appropriate def range record. The

llvm/test/CodeGen/X86/codeview-nrvo.ll

+; RUN: llc -filetype=obj -O0 < %s | llvm-readobj -codeview | FileCheck %s
+
+; CHECK: LocalSym {
+; CHECK-NEXT:   Kind: S_LOCAL (0x113E)
+; CHECK-NEXT:   Type: string&
+; CHECK-NEXT:   Flags [ (0x0)
+; CHECK-NEXT:   ]
+; CHECK-NEXT:   VarName: Result
+; CHECK-NEXT: }
+; CHECK-NEXT: DefRangeRegisterRelSym {
+; CHECK-NEXT:   Kind: S_DEFRANGE_REGISTER_REL (0x1145)
+; CHECK-NEXT:   BaseRegister: 22
+; CHECK-NEXT:   HasSpilledUDTMember: No
+; CHECK-NEXT:   OffsetInParent: 0
+; CHECK-NEXT:   BasePointerOffset: {{[^0]}}
+
+target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
+target triple = "i386-pc-windows-msvc19.0.24215"
+
+%struct.string = type { i32, i32, i8* }
+
+; Function Attrs: noinline nounwind optnone sspstrong
+define void @"\01?fun@@YA?AUstring@@XZ"(%struct.string* noalias sret %agg.result) #0 !dbg !10 {
+entry:
+  call void @llvm.dbg.declare(metadata %struct.string* %agg.result, metadata !21, metadata !22), !dbg !23
+  %call = call dereferenceable(12) %struct.string* @"\01?getString@@YAAAUstring@@XZ"(), !dbg !23
+  %0 = bitcast %struct.string* %agg.result to i8*, !dbg !23
+  %1 = bitcast %struct.string* %call to i8*, !dbg !23
+  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %0, i8* %1, i32 12, i32 4, i1 false), !dbg !23
+  ret void, !dbg !24
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+declare dereferenceable(12) %struct.string* @"\01?getString@@YAAAUstring@@XZ"() #2
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture writeonly, i8* nocapture readonly, i32, i32, i1) #3
+
+attributes #0 = { noinline nounwind optnone sspstrong "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="pentium4" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone speculatable }
+attributes #2 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "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" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="pentium4" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #3 = { argmemonly nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.linker.options = !{!3, !4}
+!llvm.module.flags = !{!5, !6, !7, !8}
+!llvm.ident = !{!9}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 6.0.0 ", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "codeview-nrvo.ll", directory: "C:\5Csrc\5Ctmp", checksumkind: CSK_MD5, checksum: "c547c362c610fa79e7abaddc76e1efe7")
+!2 = !{}
+!3 = !{!"/DEFAULTLIB:libcmt.lib"}
+!4 = !{!"/DEFAULTLIB:oldnames.lib"}
+!5 = !{i32 1, !"NumRegisterParameters", i32 0}
+!6 = !{i32 2, !"CodeView", i32 1}
+!7 = !{i32 2, !"Debug Info Version", i32 3}
+!8 = !{i32 1, !"wchar_size", i32 2}
+!9 = !{!"clang version 6.0.0 "}
+!10 = distinct !DISubprogram(name: "fun", linkageName: "\01?fun@@YA?AUstring@@XZ", scope: !1, file: !1, line: 9, type: !11, isLocal: false, isDefinition: true, scopeLine: 9, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13}
+!13 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "string", file: !1, line: 1, size: 96, elements: !14, identifier: ".?AUstring@@")
+!14 = !{!15, !17, !18}
+!15 = !DIDerivedType(tag: DW_TAG_member, name: "length", scope: !13, file: !1, line: 2, baseType: !16, size: 32)
+!16 = !DIBasicType(name: "long int", size: 32, encoding: DW_ATE_signed)
+!17 = !DIDerivedType(tag: DW_TAG_member, name: "size", scope: !13, file: !1, line: 3, baseType: !16, size: 32, offset: 32)
+!18 = !DIDerivedType(tag: DW_TAG_member, name: "data", scope: !13, file: !1, line: 4, baseType: !19, size: 32, offset: 64)
+!19 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !20, size: 32)
+!20 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!21 = !DILocalVariable(name: "Result", scope: !10, file: !1, line: 10, type: !13)
+!22 = !DIExpression()
+!23 = !DILocation(line: 10, scope: !10)
+!24 = !DILocation(line: 11, scope: !10)