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.