[PATCH] D21011: [codeview] Add complex record type translation

[Amjad Aboud via llvm-commits]

aaboud added a comment.

Thanks Reid for the comments.
Please just let me know what you think about the double check for Modifier and Pointer record generation (see my comment below).
I will upload a new patch with all the fixes accordingly.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:879-881
@@ -871,1 +878,5 @@
 
+  // While processing the type being pointed to it is possible we already
+  // created this pointer type.  If so, we check here and return the existing
+  // pointer type.
+  //
----------------
rnk wrote:
> This example works fine today without this extra lookup, though. The only way I could imagine this happening is if translating 'A*' triggers translation of the complete A type, and we can avoid that. The graph of types should be acyclic if you don't follow edges through record types, which is what I was trying to do.
> 
> Instead, I think we should queue all complete record types that we've seen during type translation in a separate list, and lower them later. We'll need this anyway to implement S_UDT.
The only reason why this is working today without this check, is that we have another check in MemoryTypeTableBuilder for the hashed record.
But this is still inefficient as we are creating the Record and building the String using the TypeRecordBuilder, just to find out that we already have the record hashed!

If you think this is good enough, I do not mind removing all these checks (we can add them later anyway).

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1065
@@ +1064,3 @@
+                                               const DISubprogram *SP) {
+  switch (SP->getFlags() & DINode::FlagAccessibility) {
+  case DINode::FlagPrivate:   return MemberAccess::Private;
----------------
rnk wrote:
> Hm, maybe this thing should just take `(unsigned RecordTag, unsigned Flags)` so we can avoid the duplication.
Will fix in next uploaded patch.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1293
@@ +1292,3 @@
+// stripScopesFromName(name)
+//
+// Subprogram names used to contain only the name, now they are fully
----------------
rnk wrote:
> Sure, we can do that. Just modify the metadata in your test case to have the names you expect and we'll fix clang later.
Will do that in next uploaded patch.


http://reviews.llvm.org/D21011