Fix bug in getCompleteTypeIndex in codeview debug info

[akhuang]

Summary

When there are multiple instances of a forward decl record type, only the first one is emitted with a type index, because
the type is added to a map with a null type index. Avoid this by reordering so that forward decl types aren't added to the map.

[rnk]

This should have a test. I think you can use this C++ to start:

struct Foo;
extern "C" __declspec(allocator) Foo *alloc_foo();
void escape(Foo*);
void doit() {
  escape(alloc_foo());
  escape(alloc_foo());
  escape(alloc_foo());
}

Before this change, the symbols would show the wrong type for the second and third heap alloc sites.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

2519–2521

While we're here, please simplify this to just use .find since we can't use InsertResult below.

2538–2541

If you do delete InsertResult, you should simplify this comment to just say that we need to re-do the hash lookup because complete type lowering may insert more complete types into the table.

[akhuang]

• Add comment about inserting type to map

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

2519–2521

Not inserting breaks in the case of circular references

[rnk]

In D61460#1488547, @rnk wrote:

This should have a test.

Still needs a test, did you forget to add it when making the diff?

[akhuang]

• Add case with multiple heapallocsites to test

[akhuang]

I modified the existing test a bit to add the multiple heapallocsites case.

[rnk]

lgtm

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

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.
   const auto *CTy = cast<DICompositeType>(Ty);
-  auto InsertResult = CompleteTypeIndices.insert({CTy, TypeIndex()});
-  if (!InsertResult.second)
-    return InsertResult.first->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.
   // We only emit a forward declaration for named types.
   if (!CTy->getName().empty() || !CTy->getIdentifier().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.
     if (CTy->isForwardDecl())
       return FwdDeclTI;
   }
 
+  // Check if we've already translated the complete record type.
+  // Insert the type with a null TypeIndex to signify that the type is currently
+  // being lowered.
+  auto InsertResult = CompleteTypeIndices.insert({CTy, TypeIndex()});
+  if (!InsertResult.second)
+    return InsertResult.first->second;
+
   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");
   }
 
   // Update the type index associated with this CompositeType.  This cannot
   // use the 'InsertResult' iterator above because it is potentially
   // invalidated by map insertions which can occur while lowering the class
   // type above.
   CompleteTypeIndices[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.

llvm/test/CodeGen/X86/label-heapallocsite.ll

 ; RUN: llc -O0 < %s | FileCheck %s
 ; FIXME: Add test for llc with optimizations once it is implemented.
 
 ; Source to regenerate:
-; $ clang --target=x86_64-windows-msvc -S heapallocsite.c  -g -gcodeview -o t.ll \
+; $ clang --target=x86_64-windows-msvc -S heapallocsite.cpp -g -gcodeview -o t.ll \
 ;      -emit-llvm -O0 -Xclang -disable-llvm-passes -fms-extensions
-; __declspec(allocator) char *myalloc(void);
-; void f() {
-;   myalloc()
-; }
 ;
 ; struct Foo {
 ;   __declspec(allocator) virtual void *alloc();
 ; };
-; void use_alloc(void*);
-; void do_alloc(Foo *p) {
+;
+; extern "C" __declspec(allocator) Foo *alloc_foo();
+;
+; extern "C" void use_alloc(void*);
+; extern "C" void call_virtual(Foo *p) {
 ;   use_alloc(p->alloc());
 ; }
+;
+; extern "C" void call_multiple() {
+;   use_alloc(alloc_foo());
+;   use_alloc(alloc_foo());
+; }
 
 target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-windows-msvc"
 
 %struct.Foo = type { i32 (...)** }
 
 ; Function Attrs: noinline optnone uwtable
-define dso_local void @f() #0 !dbg !8 {
+define dso_local void @call_virtual(%struct.Foo* %p) #0 !dbg !8 {
 entry:
-  %call = call i8* @myalloc(), !dbg !11, !heapallocsite !2
-  ret void, !dbg !12
+  %p.addr = alloca %struct.Foo*, align 8
+  store %struct.Foo* %p, %struct.Foo** %p.addr, align 8
+  call void @llvm.dbg.declare(metadata %struct.Foo** %p.addr, metadata !13, metadata !DIExpression()), !dbg !14
+  %0 = load %struct.Foo*, %struct.Foo** %p.addr, align 8, !dbg !15
+  %1 = bitcast %struct.Foo* %0 to i8* (%struct.Foo*)***, !dbg !15
+  %vtable = load i8* (%struct.Foo*)**, i8* (%struct.Foo*)*** %1, align 8, !dbg !15
+  %vfn = getelementptr inbounds i8* (%struct.Foo*)*, i8* (%struct.Foo*)** %vtable, i64 0, !dbg !15
+  %2 = load i8* (%struct.Foo*)*, i8* (%struct.Foo*)** %vfn, align 8, !dbg !15
+  %call = call i8* %2(%struct.Foo* %0), !dbg !15, !heapallocsite !2
+  call void @use_alloc(i8* %call), !dbg !15
+  ret void, !dbg !16
 }
 
-; CHECK-LABEL: f: # @f
-; CHECK: .Lheapallocsite0:
-; CHECK: callq myalloc
-; CHECK: .Lheapallocsite1:
-; CHECK: retq
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
 
-declare dso_local i8* @myalloc() #1
+declare dso_local void @use_alloc(i8*) #2
 
 ; Function Attrs: noinline optnone uwtable
-define dso_local void @do_alloc(%struct.Foo* %p) #0 !dbg !13 {
+define dso_local void @call_multiple() #0 !dbg !17 {
 entry:
-  %p.addr = alloca %struct.Foo*, align 8
-  store %struct.Foo* %p, %struct.Foo** %p.addr, align 8
-  call void @llvm.dbg.declare(metadata %struct.Foo** %p.addr, metadata !18, metadata !DIExpression()), !dbg !19
-  %0 = load %struct.Foo*, %struct.Foo** %p.addr, align 8, !dbg !20
-  %1 = bitcast %struct.Foo* %0 to i8* (%struct.Foo*)***, !dbg !20
-  %vtable = load i8* (%struct.Foo*)**, i8* (%struct.Foo*)*** %1, align 8, !dbg !20
-  %vfn = getelementptr inbounds i8* (%struct.Foo*)*, i8* (%struct.Foo*)** %vtable, i64 0, !dbg !20
-  %2 = load i8* (%struct.Foo*)*, i8* (%struct.Foo*)** %vfn, align 8, !dbg !20
-  %call = call i8* %2(%struct.Foo* %0), !dbg !20, !heapallocsite !2
-  call void @use_alloc(i8* %call), !dbg !20
-  ret void, !dbg !21
+  %call = call %struct.Foo* @alloc_foo(), !dbg !20, !heapallocsite !12
+  %0 = bitcast %struct.Foo* %call to i8*, !dbg !20
+  call void @use_alloc(i8* %0), !dbg !20
+  %call1 = call %struct.Foo* @alloc_foo(), !dbg !21, !heapallocsite !12
+  %1 = bitcast %struct.Foo* %call1 to i8*, !dbg !21
+  call void @use_alloc(i8* %1), !dbg !21
+  ret void, !dbg !22
 }
 
-; CHECK-LABEL: do_alloc: # @do_alloc
-; CHECK: .Lheapallocsite2:
+declare dso_local %struct.Foo* @alloc_foo() #2
+
+; CHECK-LABEL: call_virtual: # @call_virtual
+; CHECK: .Lheapallocsite0:
 ; CHECK: callq *(%rax)
+; CHECK: .Lheapallocsite1:
+; CHECK: retq
+
+; CHECK-LABEL: call_multiple: # @call_multiple
+; CHECK: .Lheapallocsite4:
+; CHECK: callq alloc_foo
+; CHECK: .Lheapallocsite5:
+; CHECK: .Lheapallocsite2:
+; CHECK: callq alloc_foo
 ; CHECK: .Lheapallocsite3:
 ; CHECK: retq
 
 ; CHECK-LABEL: .short  4423                    # Record kind: S_GPROC32_ID
 ; CHECK:       .short  4446                    # Record kind: S_HEAPALLOCSITE
 ; CHECK-NEXT:  .secrel32 .Lheapallocsite0
 ; CHECK-NEXT:  .secidx .Lheapallocsite0
 ; CHECK-NEXT:  .short .Lheapallocsite1-.Lheapallocsite0
 ; CHECK-NEXT:  .long 3
 ; CHECK-NEXT:  .p2align 2
 ; CHECK-LABEL: .short  4431                    # Record kind: S_PROC_ID_END
 
 ; CHECK-LABEL: .short  4423                    # Record kind: S_GPROC32_ID
 ; CHECK:       .short  4446                    # Record kind: S_HEAPALLOCSITE
 ; CHECK-NEXT:  .secrel32 .Lheapallocsite2
 ; CHECK-NEXT:  .secidx .Lheapallocsite2
 ; CHECK-NEXT:  .short .Lheapallocsite3-.Lheapallocsite2
-; CHECK-NEXT:  .long 3
+; CHECK-NEXT:  .long 4096
 ; CHECK-NEXT:  .p2align 2
-; CHECK-LABEL: .short  4431                    # Record kind: S_PROC_ID_END
-
-; Function Attrs: nounwind readnone speculatable
-declare void @llvm.dbg.declare(metadata, metadata, metadata) #2
 
-declare dso_local void @use_alloc(i8*) #1
+; CHECK:       .short  4446                    # Record kind: S_HEAPALLOCSITE
+; CHECK-NEXT:  .secrel32 .Lheapallocsite4
+; CHECK-NEXT:  .secidx .Lheapallocsite4
+; CHECK-NEXT:  .short .Lheapallocsite5-.Lheapallocsite4
+; CHECK-NEXT:  .long 4096
+; CHECK-NEXT:  .p2align 2
+; CHECK-LABEL: .short  4431                    # Record kind: S_PROC_ID_END
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4, !5, !6}
 !llvm.ident = !{!7}
 
-!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 9.0.0 (https://github.com/llvm/llvm-project.git 4eff3de99423a62fd6e833e29c71c1e62ba6140b)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
-!1 = !DIFile(filename: "heapallocsite.cpp", directory: "C:\5Csrc\5Ctest", checksumkind: CSK_MD5, checksum: "6d758cfa3834154a04ce8a55102772a9")
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 9.0.0 (https://github.com/llvm/llvm-project.git 9c8073f44f786fbf47335e53f20abe64429e8e47)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)!1 = !DIFile(filename: "filename", directory: "directory", checksumkind: CSK_MD5, checksum: "096443b661a0af36da9006330c08f97e")
 !2 = !{}
 !3 = !{i32 2, !"CodeView", i32 1}
 !4 = !{i32 2, !"Debug Info Version", i32 3}
 !5 = !{i32 1, !"wchar_size", i32 2}
 !6 = !{i32 7, !"PIC Level", i32 2}
-!7 = !{!"clang version 9.0.0 (https://github.com/llvm/llvm-project.git 4eff3de99423a62fd6e833e29c71c1e62ba6140b)"}
-!8 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 3, type: !9, scopeLine: 3, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!7 = !{!"clang version 9.0.0 (https://github.com/llvm/llvm-project.git 9c8073f44f786fbf47335e53f20abe64429e8e47)"}
+!8 = distinct !DISubprogram(name: "call_virtual", scope: !1, file: !1, line: 8, type: !9, scopeLine: 8, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
 !9 = !DISubroutineType(types: !10)
-!10 = !{null}
-!11 = !DILocation(line: 4, scope: !8)
-!12 = !DILocation(line: 5, scope: !8)
-!13 = distinct !DISubprogram(name: "do_alloc", scope: !1, file: !1, line: 11, type: !14, scopeLine: 11, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
-!14 = !DISubroutineType(types: !15)
-!15 = !{null, !16}
-!16 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !17, size: 64)
-!17 = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo", file: !1, line: 7, flags: DIFlagFwdDecl)
-!18 = !DILocalVariable(name: "p", arg: 1, scope: !13, file: !1, line: 11, type: !16)
-!19 = !DILocation(line: 11, scope: !13)
-!20 = !DILocation(line: 12, scope: !13)
-!21 = !DILocation(line: 13, scope: !13)
-
+!10 = !{null, !11}
+!11 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !12, size: 64)
+!12 = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo", file: !1, line: 1, flags: DIFlagFwdDecl, identifier: ".?AUFoo@@")
+!13 = !DILocalVariable(name: "p", arg: 1, scope: !8, file: !1, line: 8, type: !11)
+!14 = !DILocation(line: 8, scope: !8)
+!15 = !DILocation(line: 9, scope: !8)
+!16 = !DILocation(line: 10, scope: !8)
+!17 = distinct !DISubprogram(name: "call_multiple", scope: !1, file: !1, line: 12, type: !18, scopeLine: 12, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!18 = !DISubroutineType(types: !19)
+!19 = !{null}
+!20 = !DILocation(line: 13, scope: !17)
+!21 = !DILocation(line: 14, scope: !17)
+!22 = !DILocation(line: 15, scope: !17)