[PATCH] [-cxx-abi microsoft] Emit thunks for pointers to virtual member functions

[Timur Iskhodzhanov]

  I'm not very sure about the GenerateVirtualMemPtrThunk code.
  It's probably mostly copied from other similar functions, but it's a bit hard to reason about its correctness without firsthand knowledge of why each line is required.
  Can you tell what was function you've used as a reference while writing GenerateVirtualMemPtrThunk?


================
Comment at: include/clang/AST/Mangle.h:117
@@ -116,1 +116,3 @@
   virtual void mangleCXXName(const NamedDecl *D, raw_ostream &) = 0;
+  virtual void mangleVirtualMemPtrThunk(const CXXMethodDecl *MD,
+                                        int VTableIndex, raw_ostream &) = 0;
----------------
Do we need it for the Itanium ABI?
What do you think about moving this to MicrosoftMangleContext?

================
Comment at: lib/AST/MicrosoftMangle.cpp:1893
@@ +1892,3 @@
+  Mangler.getStream() << "$B";
+  Mangler.mangleNumber(VTableIndex * (Is64Bit ? 8 : 4));
+  Mangler.getStream() << "A";
----------------
We use "offset in bytes" rather than "index" for vtordisp thunks if they lookup a vbtable.
I think we should use OffsetInVFTable here as well.

================
Comment at: test/CodeGenCXX/microsoft-abi-virtual-member-pointers.cpp:25
@@ +24,3 @@
+
+// CHECK32: define void @"\01?f@@YAXXZ"() {{.*}} {
+// CHECK32: store i8* bitcast (void (%struct.C*)* @"\01??_9C@@$BA at AE" to i8*), i8** %ptr
----------------
Consider using CHECK*-LABEL: to get better scoping in case anything breaks.

Also, personally I just drop the part after ")" in lit CHECKs for function definitions. It tends to change every now and then...

================
Comment at: test/CodeGenCXX/microsoft-abi-virtual-member-pointers.cpp:39
@@ +38,3 @@
+// Thunk for calling the 1st virtual function in C with no parameters.
+// CHECK32: define x86_thiscallcc void @"\01??_9C@@$BA at AE"(%struct.C* %this) unnamed_addr {{.*}} {
+// CHECK32: %vfuncptr = getelementptr inbounds void (%struct.C*)** %vtable, i64 0
----------------
dumpbin demangles this as

  [thunk]: __thiscall C::`vcall'{0,{flat}}' }'

Do you know what "flat" stands for?

================
Comment at: test/CodeGenCXX/microsoft-abi-virtual-member-pointers.cpp:45
@@ +44,3 @@
+// CHECK32: }
+// CHECK64: define void @"\01??_9C@@$BA at AA"(%struct.C* %this) unnamed_addr {{.*}} {
+// CHECK64: %vfuncptr = getelementptr inbounds void (%struct.C*)** %vtable, i64 0
----------------
Inconsistent vertical whitespace between sections.
You can use just

  //

between CHECK32/CHECK64 sections to visually separate 32/64 sections and different sections from one another.

================
Comment at: lib/CodeGen/CGVTables.cpp:367
@@ +366,3 @@
+
+  CurGD = MD;
+
----------------
~~Are you sure we don't need to restore CurGD when we return?~~

~~I assume you used GenerateThunk/GenerateVarArgsThunk as a reference.~~
~~However, they are called when emitting VTables and/or after emitting the respective virtual method.~~
~~Can we call GenerateVirtualMemPtrThunk while generating code for some other function?~~

Ah, so you're creating a new CGF to call GenerateVirtualMemPtrThunk...
Should probably add a comment here?
Or maybe even better at the declaration in lib/CodeGen/CodeGenFunction.h ?

================
Comment at: lib/CodeGen/CGVTables.cpp:365
@@ +364,3 @@
+  CGM.SetLLVMFunctionAttributes(MD, FnInfo, ThunkFn);
+  CGM.SetLLVMFunctionAttributesForDefinition(MD, ThunkFn);
+
----------------
Why so many SetLLVMFunctionAttributes* calls?

================
Comment at: lib/CodeGen/CGVTables.cpp:389
@@ +388,3 @@
+  // Get to the vtable from 'this'.
+  llvm::Value *VTable =
+      GetVTablePtr(This, ThunkTy->getPointerTo()->getPointerTo());
----------------
This looks ABI-specific. Do we need this function in Itanium ABI?

Also, worth adding a test for vmemfun pointer with virtual inheritance.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1362
@@ +1361,3 @@
+    int VTableIndex =
+        CGM.getMicrosoftVTableContext().getMethodVFTableLocation(MD).Index;
+    CodeGenFunction CGF(CGM);
----------------
This drops VFTableOffset (hm, I think I should rename it to VFPtrOffset) and VBTableIndex.
Interestingly, this doesn't affect vmemfunptrs as they are executed with the correct vfptr as ECX.
Anyways, should probably add a comment here and a couple of simple tests with multiple and virtual inheritance to make sure we always do the right thing.


http://llvm-reviews.chandlerc.com/D2104