================
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@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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
