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

[Reid Kleckner via llvm-commits]

rnk added a comment.

Can we separate the instance member function type changes from the class info changes? We should already get method types from member functions.


================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:1362
@@ -1361,3 +1361,3 @@
 public:
-  VirtualBaseClassRecord(MemberAccess Access, TypeIndex BaseType,
+  VirtualBaseClassRecord(bool Indirect, MemberAccess Access, TypeIndex BaseType,
                          TypeIndex VBPtrType, uint64_t Offset, uint64_t Index)
----------------
It would be more consistent to directly take the TypeRecordKind as a parameter here.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:876-878
@@ -871,1 +875,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.
+  //
----------------
We should assert if this happens. I'd be interested in a test case that triggered this scenario.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:927
@@ +926,3 @@
+  if (auto STy = dyn_cast_or_null<DISubroutineType>(BaseType)) {
+    PointeeTI = lowerTypeMemberFunction(STy, Ty->getClassType().resolve());
+    PM = PointerMode::PointerToMemberFunction;
----------------
I would really prefer it if we formed a new key into our type index map and called the generic getTypeIndex method here. Calling 'lowerTypeMemberFunction' unconditionally is a good way to end up lowering a type twice by accident.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:964-966
@@ +963,5 @@
+
+  // While processing the type being pointed to, it is possible we already
+  // created this modifier type.  If so, we check here and return the existing
+  // modifier type.
+  //
----------------
Ditto, this is bad.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1220
@@ +1219,3 @@
+  const DICompositeType *DDTy = dyn_cast<DICompositeType>(BaseTy);
+  ClassInfo &InherInfo = collectClassInfo(DDTy);
+
----------------
Ah, yeah, OK, when generating complete type info we will need to recurse onto base types. Your comment from before about not caching the hash table lookup makes sense now.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1301
@@ +1300,3 @@
+
+static StringRef stripScopesFromName(StringRef name) {
+  StringRef::size_type pos;
----------------
Please don't commit this code if we can just fix it in clang. It's really easy to change our name printing code, we have a printing option that detects if codeview is enabled.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.h:145-150
@@ -140,3 +144,8 @@
   /// DIType* and DISubprogram*.
-  DenseMap<const DINode *, codeview::TypeIndex> TypeIndices;
+  typedef DenseMap<const DINode *, codeview::TypeIndex> TypeIndicesMap;
+  /// Map from record type metadata nodes to TypeIndicesMap. Primarily indexed
+  /// by DIType*.
+  /// This is needed for methods as DISubroutineType representing static method
+  /// type are shared with non-method function type.
+  DenseMap<const DIType *, TypeIndicesMap> TypeIndicesPerScope;
 
----------------
Having DenseMaps inside DenseMaps will be terribly inefficient. A better way to do this would be to extend the key to have more data, perhaps with a PointerUnion between DINode* and MyCustomSubroutineKey.

Actually, I think it might be better to change DISubroutineType to have a `scope` operand. We're going to need to add a `callingconv` field anyway, might as well go all the way so we can simplify this. I need to look at Duncan's recent ODR changes to understand if it's OK to have this cycle in the metadata.


http://reviews.llvm.org/D21011