[CodeView] omit forward references for unnamed structs and unions

[bwyma]

Summary

When a forward reference is emitted, Visual Studio uses the name to match the forward reference to the definition. For unnamed structs and unions there is no name to perform this matching. Instead, references to unnamed types are expected to refer to the complete type definition.

The changes to CodeViewDebug omit the forward declaration for unnamed types.
The test DebugInfo/COFF/unnamed.ll was added to verify the correct emission of unnamed types.
The test DebugInfo/COFF/bitfields.ll needed an update because the source contains an unnamed struct.

[rnk]

Isn't this problematic if the unnamed struct has methods? That's the usual way you get cycles in the type graph.

If this only applies to plain C structs, I would rather start inventing "unique" names by hashing the source location in some way and then continuing to emit forward decls. That solves problems with named types in anonymous namespaces, which we currently don't give unique names.

[bwyma]

Unnamed C++ structs are given a name as of rL274401, so unnamed structs containing a method will have a name assigned to them and will not create a cycle in this case.

If you feel strongly about naming these then I can improvise a name as you suggested, but I believe the current proposal has two minor advantages:

1. Visual Studio omits the forward reference for these types and compatibility is generally a good thing.
2. Omitting the forward reference and eliminating the manufactured name will use less space, and keeping the debug information size small whenever possible is desirable.

If you are willing to accept this suggestion but are still concerned about potential cycles in the type graph, then I can add additional code to prevent them.

Let me know what you think.

[rnk]

I think I've convinced myself that it's impossible to create a cycle in the C type graph. I'll sleep better if you check for an empty identifier in addition to an empty name on the DICompositeType. That is essentially a proxy for "is this a C++ type" because all C++ types have identifiers, which are effectively mangled names.

However, it's still possible to construct a .ll file that has a cycle by taking a C++ example with a cycle (consider any unnamed struct with a method) and removing the name and identifier. We should add that as an LLVM test and make sure we gracefully detect it and error out. Calling either report_fatal_error or the LLVMContext dianostic handler would be fine, and should be testable in lit.

[bwyma]

Thank you Reid. I'll make the changes you requested and upload a new patch. Much appreciated.

[bwyma]

I made the changes you requested. Specifically:

• I check for an empty identifier in addition to an empty name on the composite type.
• I updated the code to call report_fatal_error when an unexpected cycle occurs in the type graph.
• I added a lit test containing a cycle in the type graph to make sure the error is caught.

[rnk]

lgtm with one nit, thanks!

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

1846–1847 ↗

(On Diff #128941)

Can you wrap this condition (which is duplicated below) into a helper, something like shouldAlwaysEmitCompleteClassType?

[bwyma]

This patch was reverted in rL327414 due a build bot failure.
It depends upon a fix for C++ lambdas which hasn't landed yet. Once that patch lands, this one will be reapplied exactly as seen here.

[bwyma]

Update the tests to use retainedNodes instead of variables following the changes made in rL331841.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

if (auto *SP = dyn_cast<DISubprogram>(Element)) {
       Info.NestedClasses.push_back(Composite);
     }
     // Skip other unrecognized kinds of elements.
   }
   return Info;
 }
 
 TypeIndex CodeViewDebug::lowerTypeClass(const DICompositeType *Ty) {
+  // Emit the complete type for unnamed classes.
+  if (Ty->getName().empty() && !Ty->isForwardDecl())
+    return getCompleteTypeIndex(Ty);
+
   // First, construct the forward decl.  Don't look into Ty to compute the
   // forward decl options, since it might not be available in all TUs.
   TypeRecordKind Kind = getRecordKind(Ty);
   ClassOptions CO =
       ClassOptions::ForwardReference | getCommonClassOptions(Ty);
   std::string FullName = getFullyQualifiedName(Ty);
   ClassRecord CR(Kind, 0, CO, TypeIndex(), TypeIndex(), TypeIndex(), 0,
                  FullName, Ty->getIdentifier());
TypeIndex CodeViewDebug::lowerCompleteTypeClass(const DICompositeType *Ty) {
   TypeTable.writeKnownType(USLR);
 
   addToUDTs(Ty, ClassTI);
 
   return ClassTI;
 }
 
 TypeIndex CodeViewDebug::lowerTypeUnion(const DICompositeType *Ty) {
+  // Emit the complete type for unnamed unions.
+  if (Ty->getName().empty() && !Ty->isForwardDecl())
+    return getCompleteTypeIndex(Ty);
+
   ClassOptions CO =
       ClassOptions::ForwardReference | getCommonClassOptions(Ty);
   std::string FullName = getFullyQualifiedName(Ty);
   UnionRecord UR(0, CO, TypeIndex(), 0, FullName, Ty->getIdentifier());
   TypeIndex FwdDeclTI = TypeTable.writeKnownType(UR);
   if (!Ty->isForwardDecl())
     DeferredCompleteTypes.push_back(Ty);
   return FwdDeclTI;
TypeIndex CodeViewDebug::getCompleteTypeIndex(DITypeRef TypeRef) {
   case dwarf::DW_TAG_class_type:
   case dwarf::DW_TAG_structure_type:
   case dwarf::DW_TAG_union_type:
     break;
   default:
     return getTypeIndex(Ty);
   }
 
-  // Check if we've already translated the complete record type.  Lowering a
-  // complete type should never trigger lowering another complete type, so we
-  // can reuse the hash table lookup result.
+  // Check if we've already translated the complete record type.
   const auto *CTy = cast<DICompositeType>(Ty);
-  auto InsertResult = CompleteTypeIndices.insert({CTy, TypeIndex()});
-  if (!InsertResult.second)
-    return InsertResult.first->second;
+  auto I = CompleteTypeIndices.find(CTy);
+  if (I != CompleteTypeIndices.end())
+    return I->second;
 
   TypeLoweringScope S(*this);
 
   // Make sure the forward declaration is emitted first. It's unclear if this
   // is necessary, but MSVC does it, and we should follow suit until we can show
-  // otherwise.
+  // otherwise.  We only emit a forward declaration for named types.
+  if (!CTy->getName().empty()) {
     TypeIndex FwdDeclTI = getTypeIndex(CTy);
 
-  // Just use the forward decl if we don't have complete type info. This might
-  // happen if the frontend is using modules and expects the complete definition
-  // to be emitted elsewhere.
+    // Just use the forward decl if we don't have complete type info. This
+    // might happen if the frontend is using modules and expects the complete
+    // definition to be emitted elsewhere.
     if (CTy->isForwardDecl())
       return FwdDeclTI;
+  }
 
   TypeIndex TI;
   switch (CTy->getTag()) {
   case dwarf::DW_TAG_class_type:
   case dwarf::DW_TAG_structure_type:
     TI = lowerCompleteTypeClass(CTy);
     break;
   case dwarf::DW_TAG_union_type:
     TI = lowerCompleteTypeUnion(CTy);
     break;
   default:
     llvm_unreachable("not a record");
   }
 
-  InsertResult.first->second = TI;
+  CompleteTypeIndices.insert({CTy, TI});
   return TI;
 }
 
 /// Emit all the deferred complete record types. Try to do this in FIFO order,
 /// and do this until fixpoint, as each complete record type typically
 /// references
 /// many other record types.
 void CodeViewDebug::emitDeferredCompleteTypes() {

llvm/test/DebugInfo/COFF/bitfields.ll

 ; CHECK:    BitOffset: 0
 ; CHECK:  }
 ; CHECK:  BitField ([[S1_w:.*]]) {
 ; CHECK:    TypeLeafKind: LF_BITFIELD (0x1205)
 ; CHECK:    Type: int (0x74)
 ; CHECK:    BitSize: 2
 ; CHECK:    BitOffset: 23
 ; CHECK:  }
+; CHECK:  FieldList ([[anon_fl:.*]]) {
+; CHECK:    TypeLeafKind: LF_FIELDLIST (0x1203)
+; CHECK:    DataMember {
+; CHECK:      TypeLeafKind: LF_MEMBER (0x150D)
+; CHECK:      Type: char (0x70)
+; CHECK:      FieldOffset: 0x0
+; CHECK:      Name: c
+; CHECK:    }
+; CHECK:    DataMember {
+; CHECK:      TypeLeafKind: LF_MEMBER (0x150D)
+; CHECK:      Type: short (0x11)
+; CHECK:      FieldOffset: 0x1
+; CHECK:      Name: s
+; CHECK:    }
+; CHECK:  }
 ; CHECK:  Struct ([[anon_ty:.*]]) {
 ; CHECK:    TypeLeafKind: LF_STRUCTURE (0x1505)
-; CHECK:    MemberCount: 0
-; CHECK:    Properties [ (0x88)
-; CHECK:      ForwardReference (0x80)
+; CHECK:    MemberCount: 2
+; CHECK:    Properties [ (0x8)
 ; CHECK:      Nested (0x8)
 ; CHECK:    ]
-; CHECK:    FieldList: 0x0
-; CHECK:    SizeOf: 0
+; CHECK:    FieldList: <field list> ([[anon_fl]])
+; CHECK:    SizeOf: 3
 ; CHECK:    Name: S1::<unnamed-tag>
 ; CHECK:  }
 ; CHECK:  BitField ([[S1_u:.*]]) {
 ; CHECK:    TypeLeafKind: LF_BITFIELD (0x1205)
 ; CHECK:    Type: short (0x11)
 ; CHECK:    BitSize: 3
 ; CHECK:    BitOffset: 0
 ; CHECK:  }

 ; CHECK:    TypeLeafKind: LF_STRUCTURE (0x1505)
 ; CHECK:    MemberCount: 7
 ; CHECK:    Properties [ (0x0)
 ; CHECK:    ]
 ; CHECK:    FieldList: <field list> ([[S1_fl]])
 ; CHECK:    SizeOf: 16
 ; CHECK:    Name: S1
 ; CHECK:  }
-; CHECK:  FieldList ([[anon_fl:.*]]) {
-; CHECK:    TypeLeafKind: LF_FIELDLIST (0x1203)
-; CHECK:    DataMember {
-; CHECK:      Type: char (0x70)
-; CHECK:      FieldOffset: 0x0
-; CHECK:      Name: c
-; CHECK:    }
-; CHECK:    DataMember {
-; CHECK:      Type: short (0x11)
-; CHECK:      FieldOffset: 0x1
-; CHECK:      Name: s
-; CHECK:    }
-; CHECK:  }
-; CHECK:  Struct ({{.*}}) {
-; CHECK:    TypeLeafKind: LF_STRUCTURE (0x1505)
-; CHECK:    MemberCount: 2
-; CHECK:    Properties [ (0x8)
-; CHECK:      Nested (0x8)
-; CHECK:    ]
-; CHECK:    FieldList: <field list> ([[anon_fl]])
-; CHECK:    SizeOf: 3
-; CHECK:    Name: S1::<unnamed-tag>
-; CHECK:  }
 ; CHECK:  BitField ([[S2_y:.*]]) {
 ; CHECK:    TypeLeafKind: LF_BITFIELD (0x1205)
 ; CHECK:    Type: int (0x74)
 ; CHECK:    BitSize: 1
 ; CHECK:    BitOffset: 0
 ; CHECK:  }
 ; CHECK:  FieldList ([[S2_fl:.*]]) {
 ; CHECK:    TypeLeafKind: LF_FIELDLIST (0x1203)

llvm/test/DebugInfo/COFF/unnamed.ll

+; RUN: llc < %s -filetype=obj | llvm-readobj - -codeview | FileCheck %s
+;
+; Verify CodeView emission does not emit forward references for unnamed
+; structs/unions.  If a forward reference is emitted for an unnamed composite
+; type then Visual Studio will not be able to display the value.
+;
+;   Previous values displayed by Visual Studio:
+;      local.unnamed_struct    {...}
+;      local.unnamed_union     {...}
+;
+;   New values displayed by Visual Studio:
+;      local.unnamed_struct    {m3=66 'B' }
+;      local.unnamed_union     {m1=65 m2=65 'A' }
+;
+; The reproducer:
+;   $ cat unnamed.c
+;   struct named_struct {
+;     int    id;
+;     union {
+;       int  m1;
+;       char m2;
+;     } unnamed_union;
+;     struct {
+;       char m3;
+;     } unnamed_struct;
+;   };
+;   
+;   int main()
+;   {
+;     struct named_struct local;
+;   
+;     local.id = 1;
+;     local.unnamed_union.m1 = 65;
+;     local.unnamed_struct.m3 = 'B';
+;   
+;     return 0;
+;   }
+;
+; To compile the reproducer:
+;   $ clang-cl /Z7 unnamed.c
+;
+; To regenerate the IR below:
+;   $ clang unnamed.c -S -emit-llvm -g -gcodeview
+;
+; CHECK:      FieldList ([[UnnamedUnionFieldList:.*]]) {
+; CHECK-NEXT:   TypeLeafKind: LF_FIELDLIST (0x1203)
+; CHECK-NEXT:   DataMember {
+; CHECK-NEXT:     TypeLeafKind: LF_MEMBER (0x150D)
+; CHECK-NEXT:     AccessSpecifier: Public (0x3)
+; CHECK-NEXT:     Type: int (0x74)
+; CHECK-NEXT:     FieldOffset: 0x0
+; CHECK-NEXT:     Name: m1
+; CHECK-NEXT:   }
+; CHECK-NEXT:   DataMember {
+; CHECK-NEXT:     TypeLeafKind: LF_MEMBER (0x150D)
+; CHECK-NEXT:     AccessSpecifier: Public (0x3)
+; CHECK-NEXT:     Type: char (0x70)
+; CHECK-NEXT:     FieldOffset: 0x0
+; CHECK-NEXT:     Name: m2
+; CHECK-NEXT:   }
+; CHECK-NEXT: }
+; CHECK:      Union ([[UnnamedUnion:.*]]) {
+; CHECK-NEXT:   TypeLeafKind: LF_UNION (0x1506)
+; CHECK-NEXT:   MemberCount: 2
+; CHECK-NEXT:   Properties [ (0x408)
+; CHECK-NEXT:     Nested (0x8)
+; CHECK-NEXT:     Sealed (0x400)
+; CHECK-NEXT:   ]
+; CHECK-NEXT:   FieldList: <field list> ([[UnnamedUnionFieldList]])
+; CHECK-NEXT:   SizeOf: 4
+; CHECK-NEXT:   Name: named_struct::<unnamed-tag>
+; CHECK-NEXT: }
+; CHECK:      FieldList ([[UnnamedStructFieldList:.*]]) {
+; CHECK-NEXT:   TypeLeafKind: LF_FIELDLIST (0x1203)
+; CHECK-NEXT:   DataMember {
+; CHECK-NEXT:     TypeLeafKind: LF_MEMBER (0x150D)
+; CHECK-NEXT:     AccessSpecifier: Public (0x3)
+; CHECK-NEXT:     Type: char (0x70)
+; CHECK-NEXT:     FieldOffset: 0x0
+; CHECK-NEXT:     Name: m3
+; CHECK-NEXT:   }
+; CHECK-NEXT: }
+; CHECK:      Struct ([[UnnamedStruct:.*]]) {
+; CHECK-NEXT:   TypeLeafKind: LF_STRUCTURE (0x1505)
+; CHECK-NEXT:   MemberCount: 1
+; CHECK-NEXT:   Properties [ (0x8)
+; CHECK-NEXT:     Nested (0x8)
+; CHECK-NEXT:   ]
+; CHECK-NEXT:   FieldList: <field list> ([[UnnamedStructFieldList]])
+; CHECK-NEXT:   DerivedFrom: 0x0
+; CHECK-NEXT:   VShape: 0x0
+; CHECK-NEXT:   SizeOf: 1
+; CHECK-NEXT:   Name: named_struct::<unnamed-tag>
+; CHECK-NEXT: }
+; CHECK:      FieldList ([[NamedStructFieldList:.*]]) {
+; CHECK-NEXT:   TypeLeafKind: LF_FIELDLIST (0x1203)
+; CHECK-NEXT:   DataMember {
+; CHECK-NEXT:     TypeLeafKind: LF_MEMBER (0x150D)
+; CHECK-NEXT:     AccessSpecifier: Public (0x3)
+; CHECK-NEXT:     Type: int (0x74)
+; CHECK-NEXT:     FieldOffset: 0x0
+; CHECK-NEXT:     Name: id
+; CHECK-NEXT:   }
+; CHECK-NEXT:   DataMember {
+; CHECK-NEXT:     TypeLeafKind: LF_MEMBER (0x150D)
+; CHECK-NEXT:     AccessSpecifier: Public (0x3)
+; CHECK-NEXT:     Type: named_struct::<unnamed-tag> ([[UnnamedUnion]])
+; CHECK-NEXT:     FieldOffset: 0x4
+; CHECK-NEXT:     Name: unnamed_union
+; CHECK-NEXT:   }
+; CHECK-NEXT:   DataMember {
+; CHECK-NEXT:     TypeLeafKind: LF_MEMBER (0x150D)
+; CHECK-NEXT:     AccessSpecifier: Public (0x3)
+; CHECK-NEXT:     Type: named_struct::<unnamed-tag> ([[UnnamedStruct]])
+; CHECK-NEXT:     FieldOffset: 0x8
+; CHECK-NEXT:     Name: unnamed_struct
+; CHECK-NEXT:   }
+; CHECK-NEXT: }
+; CHECK:      Struct ({{.*}}) {
+; CHECK-NEXT:   TypeLeafKind: LF_STRUCTURE (0x1505)
+; CHECK-NEXT:   MemberCount: 3
+; CHECK-NEXT:   Properties [ (0x0)
+; CHECK-NEXT:   ]
+; CHECK-NEXT:   FieldList: <field list> ([[NamedStructFieldList]])
+; CHECK-NEXT:   DerivedFrom: 0x0
+; CHECK-NEXT:   VShape: 0x0
+; CHECK-NEXT:   SizeOf: 12
+; CHECK-NEXT:   Name: named_struct
+; CHECK-NEXT: }
+
+; ModuleID = 'unnamed.c'
+source_filename = "unnamed.c"
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc19.0.24210"
+
+%struct.named_struct = type { i32, %union.anon, %struct.anon }
+%union.anon = type { i32 }
+%struct.anon = type { i8 }
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @main() #0 !dbg !7 {
+entry:
+  %retval = alloca i32, align 4
+  %local = alloca %struct.named_struct, align 4
+  store i32 0, i32* %retval, align 4
+  call void @llvm.dbg.declare(metadata %struct.named_struct* %local, metadata !11, metadata !25), !dbg !26
+  %id = getelementptr inbounds %struct.named_struct, %struct.named_struct* %local, i32 0, i32 0, !dbg !27
+  store i32 1, i32* %id, align 4, !dbg !28
+  %unnamed_union = getelementptr inbounds %struct.named_struct, %struct.named_struct* %local, i32 0, i32 1, !dbg !29
+  %m1 = bitcast %union.anon* %unnamed_union to i32*, !dbg !30
+  store i32 65, i32* %m1, align 4, !dbg !31
+  %unnamed_struct = getelementptr inbounds %struct.named_struct, %struct.named_struct* %local, i32 0, i32 2, !dbg !32
+  %m3 = getelementptr inbounds %struct.anon, %struct.anon* %unnamed_struct, i32 0, i32 0, !dbg !33
+  store i8 66, i8* %m3, align 4, !dbg !34
+  ret i32 0, !dbg !35
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #0 = { noinline nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "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"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 5.0.0 (trunk)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "unnamed.c", directory: "C:\5Cpath\5Cto\5Cdirectory", checksumkind: CSK_MD5, checksum: "a1874da39665a126d6949d929fbd4818")
+!2 = !{}
+!3 = !{i32 2, !"CodeView", i32 1}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"PIC Level", i32 2}
+!6 = !{!"clang version 5.0.0 (trunk)"}
+!7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 12, type: !8, isLocal: false, isDefinition: true, scopeLine: 13, isOptimized: false, unit: !0, variables: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DILocalVariable(name: "local", scope: !7, file: !1, line: 14, type: !12)
+!12 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "named_struct", file: !1, line: 1, size: 96, elements: !13)
+!13 = !{!14, !15, !21}
+!14 = !DIDerivedType(tag: DW_TAG_member, name: "id", scope: !12, file: !1, line: 2, baseType: !10, size: 32)
+!15 = !DIDerivedType(tag: DW_TAG_member, name: "unnamed_union", scope: !12, file: !1, line: 6, baseType: !16, size: 32, offset: 32)
+!16 = distinct !DICompositeType(tag: DW_TAG_union_type, scope: !12, file: !1, line: 3, size: 32, elements: !17)
+!17 = !{!18, !19}
+!18 = !DIDerivedType(tag: DW_TAG_member, name: "m1", scope: !16, file: !1, line: 4, baseType: !10, size: 32)
+!19 = !DIDerivedType(tag: DW_TAG_member, name: "m2", scope: !16, file: !1, line: 5, baseType: !20, size: 8)
+!20 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!21 = !DIDerivedType(tag: DW_TAG_member, name: "unnamed_struct", scope: !12, file: !1, line: 9, baseType: !22, size: 8, offset: 64)
+!22 = distinct !DICompositeType(tag: DW_TAG_structure_type, scope: !12, file: !1, line: 7, size: 8, elements: !23)
+!23 = !{!24}
+!24 = !DIDerivedType(tag: DW_TAG_member, name: "m3", scope: !22, file: !1, line: 8, baseType: !20, size: 8)
+!25 = !DIExpression()
+!26 = !DILocation(line: 14, column: 23, scope: !7)
+!27 = !DILocation(line: 16, column: 9, scope: !7)
+!28 = !DILocation(line: 16, column: 12, scope: !7)
+!29 = !DILocation(line: 17, column: 9, scope: !7)
+!30 = !DILocation(line: 17, column: 23, scope: !7)
+!31 = !DILocation(line: 17, column: 26, scope: !7)
+!32 = !DILocation(line: 18, column: 9, scope: !7)
+!33 = !DILocation(line: 18, column: 24, scope: !7)
+!34 = !DILocation(line: 18, column: 27, scope: !7)
+!35 = !DILocation(line: 20, column: 3, scope: !7)