[PATCH] D22884: [codeview] Emit vftable records

[Reid Kleckner via llvm-commits]

rnk added inline comments.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2096
@@ +2095,3 @@
+  for (const DIDerivedType *VFT : DeferredVFTables)
+    getMSVFTableTypeIndex(VFT);
+}
----------------
amccarth wrote:
> Just a rant about this pattern ... I don't see an easy way to fix this and be consistent with existing code.
> 
> The comment says this loop _emits_ stuff, but the loop looks like it just looks up type indices and drops them on the floor.  In reality, we're calling this "get..." function for its side effects.  The function name doesn't even hint at the side effects.  Many people expect methods like "getFoo" to be const accessor methods.  And if they're not const, it's probably because there's a little caching involved.  But the side effects here involve more than caching.
> 
> This seems to be a pervasive anti-pattern in the bits of LLVM I've seen, a pattern that makes it hard (for me, at least) to understand the code.  Without that comment, I'd be lost.  With it, I at least know I have to go study the guts of what looks like an accessor method to figure out the side effect.  It's especially confusing, since many methods have verbs like `emit` in them.
> 
> Anyway, just a rant.  I don't expect you to make any changes.
Actually, it might be worth rewriting this code a bit. The vftable records refer to each other, so we do need some kind of map from metadata pointer to type index. However, they form a DAG, so we could topologically sort the vftables and then emit them in order and be confident that we will emit a vftable before it is referenced. I was just lazy and used memoized recursion instead, but a topological sort would make the output more understandable.

================
Comment at: test/DebugInfo/COFF/types-data-members.ll:389-422
@@ -375,36 +388,36 @@
 ; CHECK:   }
-; CHECK:   UdtSourceLine (0x101C) {
+; CHECK:   UdtSourceLine (0x101E) {
 ; CHECK:     TypeLeafKind: LF_UDT_SRC_LINE (0x1606)
-; CHECK:     UDT: Class::Nested (0x101B)
+; CHECK:     UDT: Class::Nested (0x101D)
 ; CHECK:     SourceFile: D:\src\llvm\build\t.cpp (0x1007)
 ; CHECK:     LineNumber: 23
 ; CHECK:   }
-; CHECK:   Pointer (0x101D) {
+; CHECK:   Pointer (0x101F) {
 ; CHECK:     TypeLeafKind: LF_POINTER (0x1002)
 ; CHECK:     PointeeType: DerivedClass (0x1011)
 ; CHECK:     PointerAttributes: 0x1000C
 ; CHECK:     PtrType: Near64 (0xC)
 ; CHECK:     PtrMode: Pointer (0x0)
 ; CHECK:     IsFlat: 0
 ; CHECK:     IsConst: 0
 ; CHECK:     IsVolatile: 0
 ; CHECK:     IsUnaligned: 0
 ; CHECK:     SizeOf: 8
 ; CHECK:   }
-; CHECK:   MemberFunction (0x101E) {
+; CHECK:   MemberFunction (0x1020) {
 ; CHECK:     TypeLeafKind: LF_MFUNCTION (0x1009)
 ; CHECK:     ReturnType: void (0x3)
 ; CHECK:     ClassType: DerivedClass (0x1011)
-; CHECK:     ThisType: DerivedClass* (0x101D)
+; CHECK:     ThisType: DerivedClass* (0x101F)
 ; CHECK:     CallingConvention: NearC (0x0)
 ; CHECK:     FunctionOptions [ (0x0)
 ; CHECK:     ]
 ; CHECK:     NumParameters: 0
 ; CHECK:     ArgListType: () (0x1000)
 ; CHECK:     ThisAdjustment: 0
 ; CHECK:   }
-; CHECK:   MemberFuncId (0x101F) {
+; CHECK:   MemberFuncId (0x1021) {
 ; CHECK:     TypeLeafKind: LF_MFUNC_ID (0x1602)
 ; CHECK:     ClassType: DerivedClass (0x1011)
-; CHECK:     FunctionType: void DerivedClass::() (0x101E)
+; CHECK:     FunctionType: void DerivedClass::() (0x1020)
 ; CHECK:     Name: DerivedClass::DerivedClass
----------------
majnemer wrote:
> Might be good to use FileCheck regexes here so that we don't have to update quite so much in the future.
I could go either way, I liked having one "total output" golden test that exercises a variety of stuff.


https://reviews.llvm.org/D22884