[CodeView] Don't output S_UDT records for incomplete typedefs

[zturner]

Summary

Preivously we would output S_UDT records for typedefs that pointed to forward decls. This does not match the behavior of MSVC, so we update S_UDT records to always point to full decls. However, there are some cases where a full decl is not available, so in this case we now drop these S_UDT records entirely since they are of no help. Some tests indicate that this patch closes about 20% of the gap between clang-cl and MSVC in terms of /DEBUG:FASTLINK PDB size.

[majnemer]

Does this match what they do in C mode?

[zturner]

It matches the behavior in C++ mode. I haven't tested C mode. Is there something special about C mode?

[majnemer]

In D37162#852893, @zturner wrote:

It matches the behavior in C++ mode. I haven't tested C mode. Is there something special about C mode?

I cannot remember the details ATM but I remember C mode being different with UDTs...

[zturner]

Update to be a little big more aggressive. From the bug:

Making progress. I found that in ELFObjectWriter.cpp.obj, we were emitting about 1,100 S_UDT records that referred to LF_POINTER types, while MSVC was emitting only 9. Almost all of them were from STL template instantiations.

I dug into this and found that of the first 8 of the LF_POINTERs that I looked at, they all were pointers to types that did not have a complete decl in the debug info. So, it sounds like my original optimization was correct, but I just needed to be more aggressive. Instead of dropping it if it is "just" a typedef to a type with incomplete debug info, we need to also drill through const, volatile, pointer, reference, etc until we get to the real type.

This is a very marked improvement.

                        before           after          msvc    Gap Closed
FileCheck.pdb        4,452,352       4,165,632     5,165,056     -40.23%
opt.pdb            466,702,336     421,957,632   365,957,120      44.41%
libclang.pdb     1,304,383,488   1,177,718,784   935,817,216      34.37%
clang.pdb        1,098,010,624     977,195,008   822,833,152      43.90%
lld.pdb            508,645,376     454,995,968   404,697,088      51.61%
llvm-tblgen.pdb     59,822,080      51,105,792    38,465,536      40.81%
clang-tblgen.pdb    15,085,568      13,332,480    13,545,472     113.83%

So we're now about 50% of the way to parity. Now that I'm on the right track hopefully I can figure out the rest of them pretty easily.

[rnk]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

2204 ↗

(On Diff #112762)

This isn't attached to the null check. It would look better as a function level comment (///).

2215 ↗

(On Diff #112762)

You should be able to dyn_cast to DICompositeType and call ->isForwardDecl(). Generally speaking, when translating one representation to another, it's better to look at the input and not the output whenever possible.

Actually, is there any reason you can't do that during addToUdts? What does the TI == CTI comparision achieve?

2217–2219 ↗

(On Diff #112762)

You can simplify this to return the condition.

[zturner]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

2215 ↗

(On Diff #112762)

We want to emit a UDT if:

1. This is an enum/struct/union that has a complete decl
2. This is a simple type.

If we just say return !T->isForwardDecl(); this doesn't handle the case where T is a forward decl but a complete decl exists that we could remap to.

Also, for typedefs, addToUdts(Ty) can happen before we know if there will ever be a complete decl for Ty so this can't happen until the end.

[zturner]

Previous patch was slightly wrong. With this new update we're about 70% to parity with MSVC's fastlink PDBs.

[rnk]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

2215 ↗

(On Diff #112762)

Re: typedefs, that's what I'm getting at. We don't have to wait to the end, because we should have the DICompositeType, and it already knows if it's complete or forward.

[rnk]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

2205 ↗

(On Diff #112770)

All this discussion of what we have and haven't seen doesn't make sense to me. As long as we keep looking at the DI metadata, we know up front what types will be complete and what types used forward decls. Clang replaces forward decls with complete DICompositeTypes when it thinks it needs one.

We should be able to look at a DIType before we add it to the UDT list and say, "after looking through pointers and qualifiers, does this refer to a complete class or struct type?" If yes, add it, otherwise, don't.

[zturner]

So maybe you're right. I was under the impression that you could end up with IR that contained two DICompositeTypes. one with DW_TAG_structure_type and DIFlagFwdDecl, and a second without the forward decl flag. Then I assumed you could have a DW_TAG_typedef that referred to the forward decl, even though the complete decl appeared separately later in the IR. It seems this is not the case though and for a given linkage name, the generated IR contains either a forward decl or a complete decl, but not both. Is that right?

[zturner]

Alright, let's try this instead. Is this more what you had in mind?

[rnk]

Yep! Lgtm

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

1106 ↗

(On Diff #112792)

Make this a while loop to avoid recursion when possible.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h

190 ↗

(On Diff #112792)

I bet we can simplify this and get rid of the std::string by computing it later, but that seems like a separate patch.

llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h

public:
   bool hasUniqueName() const {
     return (Options & ClassOptions::HasUniqueName) != ClassOptions::None;
   }
 
   bool isNested() const {
     return (Options & ClassOptions::Nested) != ClassOptions::None;
   }
 
+  bool isForwardRef() const {
+    return (Options & ClassOptions::ForwardReference) != ClassOptions::None;
+  }
+
   uint16_t getMemberCount() const { return MemberCount; }
   ClassOptions getOptions() const { return Options; }
   TypeIndex getFieldList() const { return FieldList; }
   StringRef getName() const { return Name; }
   StringRef getUniqueName() const { return UniqueName; }
 
   uint16_t MemberCount;
   ClassOptions Options;

llvm/trunk/lib/CodeGen/AsmPrinter/CodeViewDebug.h

class LLVM_LIBRARY_VISIBILITY CodeViewDebug : public DebugHandlerBase {
   unsigned TypeEmissionLevel = 0;
 
   codeview::TypeIndex VBPType;
 
   const DISubprogram *CurrentSubprogram = nullptr;
 
   // The UDTs we have seen while processing types; each entry is a pair of type
   // index and type name.
-  std::vector<std::pair<std::string, codeview::TypeIndex>> LocalUDTs,
-      GlobalUDTs;
+  std::vector<std::pair<std::string, const DIType *>> LocalUDTs;
+  std::vector<std::pair<std::string, const DIType *>> GlobalUDTs;
 
   using FileToFilepathMapTy = std::map<const DIFile *, std::string>;
   FileToFilepathMapTy FileToFilepathMap;
 
   StringRef getFullFilepath(const DIFile *S);
 
   unsigned maybeRecordFile(const DIFile *F);
 
class LLVM_LIBRARY_VISIBILITY CodeViewDebug : public DebugHandlerBase {
   void emitInlineeLinesSubsection();
 
   void emitDebugInfoForFunction(const Function *GV, FunctionInfo &FI);
 
   void emitDebugInfoForGlobals();
 
   void emitDebugInfoForRetainedTypes();
 
-  void emitDebugInfoForUDTs(
-      ArrayRef<std::pair<std::string, codeview::TypeIndex>> UDTs);
+  void
+  emitDebugInfoForUDTs(ArrayRef<std::pair<std::string, const DIType *>> UDTs);
 
   void emitDebugInfoForGlobal(const DIGlobalVariable *DIGV,
                               const GlobalVariable *GV, MCSymbol *GVSym);
 
   /// Opens a subsection of the given kind in a .debug$S codeview section.
   /// Returns an end label for use with endCVSubsection when the subsection is
   /// finished.
   MCSymbol *beginCVSubsection(codeview::DebugSubsectionKind Kind);
class LLVM_LIBRARY_VISIBILITY CodeViewDebug : public DebugHandlerBase {
 
   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);
+  void addToUDTs(const DIType *Ty);
 
   codeview::TypeIndex lowerType(const DIType *Ty, const DIType *ClassTy);
   codeview::TypeIndex lowerTypeAlias(const DIDerivedType *Ty);
   codeview::TypeIndex lowerTypeArray(const DICompositeType *Ty);
   codeview::TypeIndex lowerTypeBasic(const DIBasicType *Ty);
   codeview::TypeIndex lowerTypePointer(const DIDerivedType *Ty);
   codeview::TypeIndex lowerTypeMemberPointer(const DIDerivedType *Ty);
   codeview::TypeIndex lowerTypeModifier(const DIDerivedType *Ty);

llvm/trunk/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

void CodeViewDebug::beginFunctionImpl(const MachineFunction *MF) {
 
   // Record beginning of function if we have a non-empty prologue.
   if (PrologEndLoc && !EmptyPrologue) {
     DebugLoc FnStartDL = PrologEndLoc.getFnDebugLoc();
     maybeRecordLocation(FnStartDL, MF);
   }
 }
 
-void CodeViewDebug::addToUDTs(const DIType *Ty, TypeIndex TI) {
+static bool shouldEmitUdt(const DIType *T) {
+  while (true) {
+    if (!T || T->isForwardDecl())
+      return false;
+
+    const DIDerivedType *DT = dyn_cast<DIDerivedType>(T);
+    if (!DT)
+      return true;
+    T = DT->getBaseType().resolve();
+  }
+  return true;
+}
+
+void CodeViewDebug::addToUDTs(const DIType *Ty) {
   // Don't record empty UDTs.
   if (Ty->getName().empty())
     return;
+  if (!shouldEmitUdt(Ty))
+    return;
 
   SmallVector<StringRef, 5> QualifiedNameComponents;
   const DISubprogram *ClosestSubprogram = getQualifiedNameComponents(
       Ty->getScope().resolve(), QualifiedNameComponents);
 
   std::string FullyQualifiedName =
       getQualifiedName(QualifiedNameComponents, getPrettyScopeName(Ty));
 
-  if (ClosestSubprogram == nullptr)
-    GlobalUDTs.emplace_back(std::move(FullyQualifiedName), TI);
-  else if (ClosestSubprogram == CurrentSubprogram)
-    LocalUDTs.emplace_back(std::move(FullyQualifiedName), TI);
+  if (ClosestSubprogram == nullptr) {
+    GlobalUDTs.emplace_back(std::move(FullyQualifiedName), Ty);
+  } else if (ClosestSubprogram == CurrentSubprogram) {
+    LocalUDTs.emplace_back(std::move(FullyQualifiedName), Ty);
+  }
 
   // TODO: What if the ClosestSubprogram is neither null or the current
   // subprogram?  Currently, the UDT just gets dropped on the floor.
   //
   // The current behavior is not desirable.  To get maximal fidelity, we would
   // need to perform all type translation before beginning emission of .debug$S
   // and then make LocalUDTs a member of FunctionInfo
 }
TypeIndex CodeViewDebug::lowerType(const DIType *Ty, const DIType *ClassTy) {
   }
 }
 
 TypeIndex CodeViewDebug::lowerTypeAlias(const DIDerivedType *Ty) {
   DITypeRef UnderlyingTypeRef = Ty->getBaseType();
   TypeIndex UnderlyingTypeIndex = getTypeIndex(UnderlyingTypeRef);
   StringRef TypeName = Ty->getName();
 
-  addToUDTs(Ty, UnderlyingTypeIndex);
+  addToUDTs(Ty);
 
   if (UnderlyingTypeIndex == TypeIndex(SimpleTypeKind::Int32Long) &&
       TypeName == "HRESULT")
     return TypeIndex(SimpleTypeKind::HResult);
   if (UnderlyingTypeIndex == TypeIndex(SimpleTypeKind::UInt16Short) &&
       TypeName == "wchar_t")
     return TypeIndex(SimpleTypeKind::WideCharacter);
 
TypeIndex CodeViewDebug::lowerCompleteTypeClass(const DICompositeType *Ty) {
 
   if (const auto *File = Ty->getFile()) {
     StringIdRecord SIDR(TypeIndex(0x0), getFullFilepath(File));
     TypeIndex SIDI = TypeTable.writeKnownType(SIDR);
     UdtSourceLineRecord USLR(ClassTI, SIDI, Ty->getLine());
     TypeTable.writeKnownType(USLR);
   }
 
-  addToUDTs(Ty, ClassTI);
+  addToUDTs(Ty);
 
   return ClassTI;
 }
 
 TypeIndex CodeViewDebug::lowerTypeUnion(const DICompositeType *Ty) {
   ClassOptions CO =
       ClassOptions::ForwardReference | getCommonClassOptions(Ty);
   std::string FullName = getFullyQualifiedName(Ty);
UnionRecord UR(FieldCount, CO, FieldTI, SizeInBytes, FullName,
                  Ty->getIdentifier());
   TypeIndex UnionTI = TypeTable.writeKnownType(UR);
 
   StringIdRecord SIR(TypeIndex(0x0), getFullFilepath(Ty->getFile()));
   TypeIndex SIRI = TypeTable.writeKnownType(SIR);
   UdtSourceLineRecord USLR(UnionTI, SIRI, Ty->getLine());
   TypeTable.writeKnownType(USLR);
 
-  addToUDTs(Ty, UnionTI);
+  addToUDTs(Ty);
 
   return UnionTI;
 }
 
 std::tuple<TypeIndex, TypeIndex, unsigned, bool>
 CodeViewDebug::lowerRecordFieldList(const DICompositeType *Ty) {
   // Manually count members. MSVC appears to count everything that generates a
   // field list record. Each individual overload in a method overload group

 
 void CodeViewDebug::endCVSubsection(MCSymbol *EndLabel) {
   OS.EmitLabel(EndLabel);
   // Every subsection must be aligned to a 4-byte boundary.
   OS.EmitValueToAlignment(4);
 }
 
 void CodeViewDebug::emitDebugInfoForUDTs(
-    ArrayRef<std::pair<std::string, TypeIndex>> UDTs) {
-  for (const std::pair<std::string, codeview::TypeIndex> &UDT : UDTs) {
+    ArrayRef<std::pair<std::string, const DIType *>> UDTs) {
+  for (const auto &UDT : UDTs) {
+    const DIType *T = UDT.second;
+    assert(shouldEmitUdt(T));
+
     MCSymbol *UDTRecordBegin = MMI->getContext().createTempSymbol(),
              *UDTRecordEnd = MMI->getContext().createTempSymbol();
     OS.AddComment("Record length");
     OS.emitAbsoluteSymbolDiff(UDTRecordEnd, UDTRecordBegin, 2);
     OS.EmitLabel(UDTRecordBegin);
 
     OS.AddComment("Record kind: S_UDT");
     OS.EmitIntValue(unsigned(SymbolKind::S_UDT), 2);
 
     OS.AddComment("Type");
-    OS.EmitIntValue(UDT.second.getIndex(), 4);
+    OS.EmitIntValue(getCompleteTypeIndex(T).getIndex(), 4);
 
     emitNullTerminatedSymbolName(OS, UDT.first);
     OS.EmitLabel(UDTRecordEnd);
   }
 }
 
 void CodeViewDebug::emitDebugInfoForGlobals() {
   DenseMap<const DIGlobalVariableExpression *, const GlobalVariable *>

llvm/trunk/test/DebugInfo/COFF/purge-typedef-udts.ll

+; RUN: llc < %s -filetype=obj | llvm-readobj - -codeview | FileCheck %s
+source_filename = "test/DebugInfo/COFF/purge-typedef-udts.ll"
+target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
+target triple = "i686-pc-windows-msvc19.11.25506"
+
+; C++ source to regenerate:
+; $ cat t.cpp
+; struct Foo;
+; struct Bar {
+;   Bar() {}
+;   int X;
+; };
+;
+; typedef Foo FooTypedef;
+; typedef Bar BarTypedef;
+;
+; int func(void *F) { return 7; }
+; int func(const FooTypedef *F) { return func((void*)F); }
+; int func(const BarTypedef *B) { return func((void*)B->X); }
+
+; CHECK-NOT: UDTName: FooTypedef
+; CHECK: UDTName: BarTypedef
+
+%struct.Foo = type opaque
+%struct.Bar = type { i32 }
+
+; Function Attrs: noinline nounwind optnone
+define i32 @"\01?func@@YAHPAX@Z"(i8* %F) #0 !dbg !10 {
+entry:
+  %F.addr = alloca i8*, align 4
+  store i8* %F, i8** %F.addr, align 4
+  call void @llvm.dbg.declare(metadata i8** %F.addr, metadata !14, metadata !DIExpression()), !dbg !15
+  ret i32 7, !dbg !16
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: noinline nounwind optnone
+define i32 @"\01?func@@YAHPBUFoo@@@Z"(%struct.Foo* %F) #0 !dbg !17 {
+entry:
+  %F.addr = alloca %struct.Foo*, align 4
+  store %struct.Foo* %F, %struct.Foo** %F.addr, align 4
+  call void @llvm.dbg.declare(metadata %struct.Foo** %F.addr, metadata !24, metadata !DIExpression()), !dbg !25
+  %0 = load %struct.Foo*, %struct.Foo** %F.addr, align 4, !dbg !26
+  %1 = bitcast %struct.Foo* %0 to i8*, !dbg !26
+  %call = call i32 @"\01?func@@YAHPAX@Z"(i8* %1), !dbg !27
+  ret i32 %call, !dbg !28
+}
+
+; Function Attrs: noinline nounwind optnone
+define i32 @"\01?func@@YAHPBUBar@@@Z"(%struct.Bar* %B) #0 !dbg !29 {
+entry:
+  %B.addr = alloca %struct.Bar*, align 4
+  store %struct.Bar* %B, %struct.Bar** %B.addr, align 4
+  call void @llvm.dbg.declare(metadata %struct.Bar** %B.addr, metadata !42, metadata !DIExpression()), !dbg !43
+  %0 = load %struct.Bar*, %struct.Bar** %B.addr, align 4, !dbg !44
+  %X = getelementptr inbounds %struct.Bar, %struct.Bar* %0, i32 0, i32 0, !dbg !45
+  %1 = load i32, i32* %X, align 4, !dbg !45
+  %2 = inttoptr i32 %1 to i8*, !dbg !46
+  %call = call i32 @"\01?func@@YAHPAX@Z"(i8* %2), !dbg !47
+  ret i32 %call, !dbg !48
+}
+
+attributes #0 = { noinline nounwind optnone "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 }
+
+!llvm.dbg.cu = !{!0}
+!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, retainedTypes: !3)
+!1 = !DIFile(filename: "t.cpp", directory: "D:\5Csrc\5Cllvmbuild\5Cninja", checksumkind: CSK_MD5, checksum: "27c44c8a5531845f61f582a24ef5c151")
+!2 = !{}
+!3 = !{!4}
+!4 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 32)
+!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: "func", linkageName: "\01?func@@YAHPAX@Z", scope: !1, file: !1, line: 10, type: !11, isLocal: false, isDefinition: true, scopeLine: 10, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13, !4}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !DILocalVariable(name: "F", arg: 1, scope: !10, file: !1, line: 10, type: !4)
+!15 = !DILocation(line: 10, column: 16, scope: !10)
+!16 = !DILocation(line: 10, column: 21, scope: !10)
+!17 = distinct !DISubprogram(name: "func", linkageName: "\01?func@@YAHPBUFoo@@@Z", scope: !1, file: !1, line: 11, type: !18, isLocal: false, isDefinition: true, scopeLine: 11, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!18 = !DISubroutineType(types: !19)
+!19 = !{!13, !20}
+!20 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !21, size: 32)
+!21 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !22)
+!22 = !DIDerivedType(tag: DW_TAG_typedef, name: "FooTypedef", file: !1, line: 7, baseType: !23)
+!23 = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo", file: !1, line: 1, flags: DIFlagFwdDecl, identifier: ".?AUFoo@@")
+!24 = !DILocalVariable(name: "F", arg: 1, scope: !17, file: !1, line: 11, type: !20)
+!25 = !DILocation(line: 11, column: 28, scope: !17)
+!26 = !DILocation(line: 11, column: 52, scope: !17)
+!27 = !DILocation(line: 11, column: 40, scope: !17)
+!28 = !DILocation(line: 11, column: 33, scope: !17)
+!29 = distinct !DISubprogram(name: "func", linkageName: "\01?func@@YAHPBUBar@@@Z", scope: !1, file: !1, line: 12, type: !30, isLocal: false, isDefinition: true, scopeLine: 12, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!30 = !DISubroutineType(types: !31)
+!31 = !{!13, !32}
+!32 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !33, size: 32)
+!33 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !34)
+!34 = !DIDerivedType(tag: DW_TAG_typedef, name: "BarTypedef", file: !1, line: 8, baseType: !35)
+!35 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Bar", file: !1, line: 2, size: 32, elements: !36, identifier: ".?AUBar@@")
+!36 = !{!37, !38}
+!37 = !DIDerivedType(tag: DW_TAG_member, name: "X", scope: !35, file: !1, line: 4, baseType: !13, size: 32)
+!38 = !DISubprogram(name: "Bar", scope: !35, file: !1, line: 3, type: !39, isLocal: false, isDefinition: false, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: false)
+!39 = !DISubroutineType(cc: DW_CC_BORLAND_thiscall, types: !40)
+!40 = !{null, !41}
+!41 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !35, size: 32, flags: DIFlagArtificial | DIFlagObjectPointer)
+!42 = !DILocalVariable(name: "B", arg: 1, scope: !29, file: !1, line: 12, type: !32)
+!43 = !DILocation(line: 12, column: 28, scope: !29)
+!44 = !DILocation(line: 12, column: 52, scope: !29)
+!45 = !DILocation(line: 12, column: 55, scope: !29)
+!46 = !DILocation(line: 12, column: 45, scope: !29)
+!47 = !DILocation(line: 12, column: 40, scope: !29)
+!48 = !DILocation(line: 12, column: 33, scope: !29)