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

[Amjad Aboud via llvm-commits]

aaboud added a comment.

Please see the below comments.

By the way, once you commit this patch, I can help you complete the support for classes, I have a working solution...but need couple of days to apply it to the top of trunk.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1014
@@ +1013,3 @@
+  if (!Ty->isForwardDecl()) {
+    FieldListRecordBuilder Fields;
+    for (const DINode *Element : Ty->getElements()) {
----------------
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.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1033
@@ +1032,3 @@
+          } else {
+            // Data member.
+            Fields.writeDataMember(DataMemberRecord(
----------------
Add a comment:
// FIXME: Should support bitfield members. 

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1043
@@ +1042,3 @@
+        }
+        // FIXME: Get clang to emit nested types here and do something with
+        // them.
----------------
When you said nested types, did you mean DW_TAG_inheritance? or other cases?

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1054
@@ +1053,3 @@
+    if (Kind == TypeRecordKind::Union) {
+      TypeTable.writeUnion(UnionRecord(Fields.getSubrecordCount(), CO,
+                                       HfaKind::None, FTI, SizeInBytes,
----------------
Why do we need to count the SubRecords, while we could use: Ty->getElements().size()?

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1067
@@ +1066,3 @@
+
+  // Always return the index of the forward decl.
+  return FwdDeclTI;
----------------
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?


http://reviews.llvm.org/D20924