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

[Hans Wennborg]

================
Comment at: lib/CodeGen/CGVTables.cpp:389
@@ +388,3 @@
+  // Get to the vtable from 'this'.
+  llvm::Value *VTable =
+      GetVTablePtr(This, ThunkTy->getPointerTo()->getPointerTo());
----------------
Reid Kleckner wrote:
> Hans Wennborg wrote:
> > Timur Iskhodzhanov wrote:
> > > This looks ABI-specific. Do we need this function in Itanium ABI?
> > > 
> > > Also, worth adding a test for vmemfun pointer with virtual inheritance.
> > About looking ABI-specific: yes, I'll change this to use CGM.getCXXABI().getVirtualFunctionPointer()
> > 
> > Re needing this in Itanium ABI: no, we don't have thunks like this in Itanium. I first tried to generate the thunk in the MicrosoftCXXABI class, but it was hard without access to the stuff in CodeGenFunction so I moved it here instead. Do you think that's OK?
> > 
> > I talked to Reid about the virtual inheritance case. We already don't support taking pointers to members of virtual bases (C++ doesn't allow it, though msvc supports it). We could end up in a situation where the derived class is overriding a function from a virtual base. With my code, we would end up doing the vbase adjustment to get to the vftable in the thunk, whereas msvc puts the vbase adjustment as separate parameter on the member pointer. Reid agreed we could punt on supporting this for now, so I've added an error message for that.
> > 
> > With those cases out of the way, I'm not sure what to test for regarding virtual inheritance?
> I'd like to push to keep this in MicrosoftCXXABI.  We should talk about the hard parts.
> 
> We should probably make sure we reject (maybe just with CGM.ErrorUnsupported) taking the address of a member function overridden from a virtual base.  Sema will probably allow that under Itanium, but I don't think it will work in the MS ABI.
> 
> We should be able to handle this test case:
> 
>   struct A { virtual void f(); };
>   struct B : virtual A { virtual void f(); virtual void g(); };
>   int main() {
>     // if this is in A's vftable, we'll either have to error 
>     // or fill in the VBTableOffset field of the memptr
>     &B::f;
> 
>     // should work, since g is easily callable through
>     // B's vftable without vbase adjustment
>     &B::g;
>   }
> I'd like to push to keep this in MicrosoftCXXABI. We should talk about the hard parts.

OK, I'll go to work on that.

> We should be able to handle this test case:

Right, the current patch does exactly that (errors out for &B::f, builds thunk for &B::g). Do you think we should check in a test for the error case? I don't know if we've been doing this so far, but I guess it could be nice to have here.

================
Comment at: lib/CodeGen/CGVTables.cpp:361
@@ +360,3 @@
+
+  CurGD = MD;
+
----------------
Timur Iskhodzhanov wrote:
> Looks like you wanted to add two assertions, but only added one?
Fixed.

================
Comment at: lib/CodeGen/CodeGenFunction.h:1161
@@ +1160,3 @@
+  /// \brief Generate a thunk for calling virtual member function MD, which
+  /// is at position VTableIndex in the vtable.
+  llvm::Function *GenerateVirtualMemPtrThunk(const CXXMethodDecl *MD,
----------------
Timur Iskhodzhanov wrote:
> Needs updating
Done.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1373
@@ -1372,5 +1372,3 @@
   llvm::Constant *FirstField;
-  if (MD->isVirtual()) {
-    // FIXME: We have to instantiate a thunk that loads the vftable and jumps to
-    // the right offset.
-    CGM.ErrorUnsupported(MD, "pointer to virtual member function");
+  if (MD->isVirtual() && MD->isVariadic()) {
+    CGM.ErrorUnsupported(MD, "pointer to variadic virtual member function");
----------------
Timur Iskhodzhanov wrote:
> How about
> 
>   if (!isVirtual) {
>   } else if (virtual-subcase-1) {
>   } else if (virtual-subcase-2) {
>   } ...
> 
> ?
Yup, that's probably a good idea.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1384
@@ +1383,3 @@
+                                    .VBase) {
+    CGM.ErrorUnsupported(MD, "pointer to virtual member function overriding "
+                             "member function in virtual base class");
----------------
Timur Iskhodzhanov wrote:
> Are you sure overriding is required in order to get a "complex" vmemfun ptr?
Trying to take a pointer to a member inherited from a virtual base is rejected by Sema.

================
Comment at: test/CodeGenCXX/microsoft-abi-virtual-member-pointers.cpp:26
@@ +25,3 @@
+// CHECK32-LABEL: define void @"\01?f@@YAXXZ"()
+// CHECK32: store i8* bitcast (void (%struct.C*)* @"\01??_9C@@$BA at AE" to i8*), i8** %ptr
+// CHECK32: store i8* bitcast (i32 (%struct.C*, i32, double)* @"\01??_9C@@$B3AE" to i8*), i8** %ptr2
----------------
Timur Iskhodzhanov wrote:
> Please try to run this both with Release and Debug builds.
> I'm pretty sure you'll need to replace all %xyz with %{{.*}} or %[[VARNAME:.*]]
You're right. Fixing this.

================
Comment at: lib/CodeGen/CGVTables.cpp:359
@@ +358,3 @@
+  CGM.SetLLVMFunctionAttributes(MD, FnInfo, ThunkFn);
+  CGM.SetLLVMFunctionAttributesForDefinition(MD, ThunkFn);
+
----------------
Timur Iskhodzhanov wrote:
> Can the preparation / finalization stuff be shared with GenerateThunk and friends?
Hmm, let's see if some of that can fall out from moving this code into MicrosoftCXXABI.cpp.


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