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

[Reid Kleckner via llvm-commits]

rnk added a comment.

Thanks, I actually rearchitected things significantly.

We're going to need some way to iterate all the record types, BTW. I'm not sure how to do that in the current metadata scheme.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1014
@@ +1013,3 @@
+  if (!Ty->isForwardDecl()) {
+    FieldListRecordBuilder Fields;
+    for (const DINode *Element : Ty->getElements()) {
----------------
aaboud wrote:
> I would move the FieldListRecord building to separate function call it:
> TypeIndex CodeViewDebug::lowerTypeClassFieldList(const DICompositeType *Ty)
> 
> Reason: this code will get larger once we support methods, so better leave creating the ClassRecord clear.
Yeah, that's a good idea. It also makes it easier to split union and class/struct handling while still sharing most of the code.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1043
@@ +1042,3 @@
+        }
+        // FIXME: Get clang to emit nested types here and do something with
+        // them.
----------------
aaboud wrote:
> When you said nested types, did you mean DW_TAG_inheritance? or other cases?
No, this sort of thing:
  struct A { struct Nested {}; };

We have to change the output of Clang to mention these so that the ClassRecord for A always mentions A::Nested, otherwise the complete class definitions will not merge during type stream merging.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1054
@@ +1053,3 @@
+    if (Kind == TypeRecordKind::Union) {
+      TypeTable.writeUnion(UnionRecord(Fields.getSubrecordCount(), CO,
+                                       HfaKind::None, FTI, SizeInBytes,
----------------
aaboud wrote:
> Why do we need to count the SubRecords, while we could use: Ty->getElements().size()?
Hm, this is actually wrong currently. MSVC counts the number of members, counting each overload separately, not the number of records in the field list. I'm not sure we should use getElements().size(), though, given that we skip lowering things like member functions currently. We should probably count members manually in lowerRecordFieldList. I'll do that.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1067
@@ +1066,3 @@
+
+  // Always return the index of the forward decl.
+  return FwdDeclTI;
----------------
aaboud wrote:
> I believe that it is not correct to always generate a FwdDecl record.
> But we can fix this later, as it will need some complex checks to achieve that.
> 
> David, can you confirm my assumption?
Huh, you're right. It looks like all intra-type stream type references use the forward decl, but the symbol stream may refer to the complete type index. I'm going to try to address this now, it seems important.


http://reviews.llvm.org/D20924