[codeview] Emit typedefs of types derived from incomplete types

[rnk]

Summary

The last time we evaluated this behavior was in r311904, which had this
message:

S_UDT symbols are the debugger's "index" for all the structs,
typedefs, classes, and enums in a program.  If any of those
structs/classes don't have a complete declaration, or if there is a
typedef to something that doesn't have a complete definition, then
emitting the S_UDT is unhelpful because it doesn't give the debugger
enough information to do anything useful.  On the other hand, it
results in a huge size blow-up in the resulting PDB, which is
exacerbated by an order of magnitude when linking with
/DEBUG:FASTLINK.

With this patch, we drop S_UDT records for types that refer either
directly or indirectly (e.g. through a typedef, pointer, etc) to a
class/struct/union/enum without a complete definition.  This brings us
about 50% of the way towards parity with /DEBUG:FASTLINK PDBs
generated from cl-compiled object files.

This modifies LLVM so that we only discard S_UDTs pointing directly to
incomplete types, but keep S_UDTs that indirectly use incomplete types,
as in the following C++ fragment:

struct Opaque;
typedef Opaque *OpaqueHandle;
OpaqueHandle gv;

If we don't emit the OpaqueHandle typedef in this TU, it's possible that
no TU will emit the typedef. On the other hand, this could considerably
increase object file size, so we should keep an eye on that.

[zturner]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

1447

Is this sufficient? If T->isForwardDecl() is true, does that mean the debug info does not contain a full decl at all?

[rnk]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

1447

Yes, the metadata has only a forward decl or a complete definition.

[aganea]

I just wanted to point out that this overall process of dropping UDT records in the compiler is not the behavior that MSVC exhibits. S_UDTs are (should be) stripped by the linker, not by the compiler. It's also the linker's job (in my sense) to remap forward references to full complete types (albeit if you can it locally in the compiler, it is probably better).

> cl udts-fwd.cpp /c /Z7
> cvdump udts-fwd.obj

*** SYMBOLS

(0000C8) S_GDATA32: [0000:00000000], Type:             0x105C, global_ptr
(0000E1) S_UDT:             0x105D, TypedefFwdPtr
(0000F7) S_UDT:             0x105B, TypedefFwd

*** TYPES

0x105b : Length = 34, Leaf = 0x1505 LF_STRUCTURE
	# members = 0,  field list type 0x0000, FORWARD REF, 
	Derivation list type 0x0000, VT shape type 0x0000
	Size = 0, class name = Foo, unique name = .?AUFoo@@

0x105c : Length = 10, Leaf = 0x1002 LF_POINTER
	Pointer (__ptr64), Size: 8
	Element type : 0x105B

0x105d : Length = 10, Leaf = 0x1002 LF_POINTER
	Pointer (__ptr64), Size: 8
	Element type : 0x105B

> link udts-fwd.obj /debug /entry:ExitProcess kernel32.lib /subsytem:console
> cvdump udts-fwd.pdb


*** TYPES

0x1000 : Length = 34, Leaf = 0x1505 LF_STRUCTURE
	# members = 0,  field list type 0x0000, FORWARD REF, 
	Derivation list type 0x0000, VT shape type 0x0000
	Size = 0, class name = Foo, unique name = .?AUFoo@@

0x1001 : Length = 10, Leaf = 0x1002 LF_POINTER
	Pointer (__ptr64), Size: 8
	Element type : 0x1000

*** GLOBALS

S_UDT:             0x1001, TypedefFwdPtr
S_GDATA32: [0003:00000000], Type:             0x1001, global_ptr

Like I was suggesting in D55236, I think this is part of a post-merge step that link.exe does, see here and here (although I haven't looked exactly into detail).

[zturner]

In D55239#1318690, @aganea wrote:

I just wanted to point out that this overall process of dropping UDT records in the compiler is not the behavior that MSVC exhibits. S_UDTs are (should be) stripped by the linker, not by the compiler.

I think it's fine to drop them in the compiler, because we're relying on another TU to emit them. So this saves the linker from doing some unnecessary merging in cases where we know there's no benefit to emitting them in the compiler.

In D55239#1318690, @aganea wrote:

It's also the linker's job (in my sense) to remap forward references to full complete types (albeit if you can it locally in the compiler, it is probably better).

In an ideal world, probably. But this is also going to be rather slow. In Reid's example above, which I'll repeat here:

struct Opaque;
typedef Opaque *OpaqueHandle;
OpaqueHandle gv;

we can't emit a full declaration for the S_UDT for Opaque, because we don't even have one. So the linker still has some work to do here if we're trying to be perfect, because if a full declaration exists anywhere in the program, we would want it to point to it. On the other hand, if it was struct Opaque {}; then the compiler might as well do the remapping to save the linker some work.

But I think this patch isn't trying to address the first case. We can evaluate it in a followup, but since it has the potential to slow down link times, we should be careful.

As an aside, we should also be careful about any remapping done in the linker, since it has the potential to interfere with ghash. In this case we're fine since we only ghash types and not symbols, and S_UDT is a symbol. But it's something to keep in mind.

[aganea]

My whole point was that our slowdowns were with cl.exe-generated OBJs, not clang OBJs. If LLD doesn't support this remapping (and dropping) in the same way as link.exe, this won't completly fix S_UDTs.

[aganea]

But overall I agree with @zturner, the more optimizations you can do early in the compiler, the better it is. This patch is fine and achieves that purpose. I'm just saying that it isn't enough if the goal is to support linking with cl OBJs.

[zturner]

In D55239#1318747, @aganea wrote:

My whole point was that our slowdowns were with cl.exe-generated OBJs, not clang OBJs. If LLD doesn't support this remapping (and dropping) in the same way as link.exe, this won't completly fix S_UDTs.

Yes, that's true. However, it's possible my other patch (https://reviews.llvm.org/D55228) could, since it's specifically adding functionality to the linker that wasn't there before. It's possible that our omission of S_UDT records entirely was causing the slowdown.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

if (DIScope *Scope = T->getScope().resolve()) {
       case dwarf::DW_TAG_structure_type:
       case dwarf::DW_TAG_class_type:
       case dwarf::DW_TAG_union_type:
         return false;
       }
     }
   }
 
-  while (true) {
-    if (!T || T->isForwardDecl())
-      return false;
+  // Look through nested typedefs.
+  while (T->getTag() == dwarf::DW_TAG_typedef)
+    T = cast<DIDerivedType>(T)->getBaseType().resolve();
+
+  // Don't emit a UDT if we only have a forward declaration.
+  if (T && T->isForwardDecl())
 
-    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))

llvm/test/DebugInfo/COFF/udts-fwd.ll

+; RUN: llc -filetype=obj %s -o %t.obj
+; RUN: llvm-pdbutil dump -symbols %t.obj | FileCheck %s
+
+; Foo is forward declared here. Check that we emit the typedef for the pointer,
+; but not for the incomplete type. We adopted this behavior to make
+; /debug:fastlink more efficient, but it's not clear if it's still the right
+; behavior. This test just encodes it.
+
+; C++ source:
+; struct Foo;
+; typedef Foo TypedefFwd;
+; typedef TypedefFwd *TypedefFwdPtr;
+; TypedefFwdPtr global_ptr;
+
+; CHECK:                           Symbols
+; CHECK-NEXT: ============================================================
+; CHECK-NOT:   S_UDT {{.*}} `TypedefFwd`
+; CHECK:       S_UDT [size = {{.*}}] `TypedefFwdPtr`
+; CHECK-NOT:   S_UDT {{.*}} `TypedefFwd`
+
+
+; ModuleID = 't.cpp'
+source_filename = "t.cpp"
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc19.14.26433"
+
+%struct.Foo = type opaque
+
+@"?global_ptr@@3PEAUFoo@@EA" = dso_local global %struct.Foo* null, align 8, !dbg !0
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!10, !11, !12, !13}
+!llvm.ident = !{!14}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "global_ptr", linkageName: "?global_ptr@@3PEAUFoo@@EA", scope: !2, file: !3, line: 4, type: !6, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "clang version 8.0.0 ", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5, nameTableKind: None)
+!3 = !DIFile(filename: "t.cpp", directory: "C:\5Csrc\5Cllvm-project\5Cbuild", checksumkind: CSK_MD5, checksum: "eda0a32edcbd7e1d7b0220b9ef0192c7")
+!4 = !{}
+!5 = !{!0}
+!6 = !DIDerivedType(tag: DW_TAG_typedef, name: "TypedefFwdPtr", file: !3, line: 3, baseType: !7)
+!7 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !8, size: 64)
+!8 = !DIDerivedType(tag: DW_TAG_typedef, name: "TypedefFwd", file: !3, line: 2, baseType: !9)
+!9 = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo", file: !3, line: 1, flags: DIFlagFwdDecl, identifier: ".?AUFoo@@")
+!10 = !{i32 2, !"CodeView", i32 1}
+!11 = !{i32 2, !"Debug Info Version", i32 3}
+!12 = !{i32 1, !"wchar_size", i32 2}
+!13 = !{i32 7, !"PIC Level", i32 2}
+!14 = !{!"clang version 8.0.0 "}