[PATCH] D20924: [codeview] Add basic record type translation

[Reid Kleckner via llvm-commits]

rnk added a comment.

It sounds like you think this is pretty close to OK and you want to write followups, so I'm going to land this now and give you a chance to do that. I need to spend the next few days dealing with an llvm-symbolizer issue, so go for it.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1025
@@ +1024,3 @@
+  // Construct the field list and complete type record.
+  TypeRecordKind Kind = getRecordKind(Ty);
+  // FIXME: Other ClassOptions, like ContainsNestedClass and NestedClass.
----------------
aaboud wrote:
> At this point you better check if the Ty->isForwardDecl(), if it is the case we should not create a complete class.
I can do this in getCompleteTypeIndex to share the code.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1127
@@ -977,4 +1126,3 @@
 
-  auto InsertResult = TypeIndices.insert({Ty, TI});
-  (void)InsertResult;
-  assert(InsertResult.second && "DIType lowered twice");
+  // Don't reuse the hashtable lookup result from above as type lowering can
+  // insert new map entries.
----------------
aaboud wrote:
> This comment is confusing me, I do not understand why it is needed!
> We cannot use the hashtable lookup, because if we get here we then: I == TypeIndices.emd()
> You cannot use I anyway, right?
David Blaikie asked me to add it. Let me move it above to the find lookup.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1159
@@ +1158,3 @@
+      CompleteTypeIndices.insert({cast<DICompositeType>(Ty), TypeIndex()});
+  if (!InsertResult.second)
+    return InsertResult.first->second;
----------------
aaboud wrote:
> I think this check is too later, we need to check that we did not create this complete record before the switch, i.e. at line 1140.
Oops, this was the result of a late refactoring to avoid having two tag switches. It actually doesn't pass tests. :( Fixed.


http://reviews.llvm.org/D20924