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

[Hans Wennborg]

  Thanks for the comments! New patch coming up.


================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:355
@@ +354,3 @@
+  /// \brief Generate a thunk for calling a virtual member function MD.
+  llvm::Function *EmitVirtualMemPtrThunk(const CXXMethodDecl *MD,
+                                         StringRef ThunkName);
----------------
Timur Iskhodzhanov wrote:
> Does this need to be public?
Nope. Fixed.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1932
@@ +1931,3 @@
+       I != E; ++I)
+    FunctionArgs.push_back(*I);
+
----------------
Timur Iskhodzhanov wrote:
> Can this be shared with the general thunk emission code?
> I'm worried by the duplication of non-trivial code.
> 
> I think pretty much everything except this/return adjustment, call target and the particular linkage type can be shared.
I've tried to break it out into two functions: one for setting up the function and one for making the call and return.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1972
@@ +1971,3 @@
+  CGF.FinishFunction();
+  CGM.setFunctionLinkage(MD, ThunkFn); // XXX: Do we need this?
+
----------------
Reid Kleckner wrote:
> Timur Iskhodzhanov wrote:
> > Reid Kleckner wrote:
> > > Timur Iskhodzhanov wrote:
> > > > Isn't this copying the function's linkage over to the thunk?
> > > > Don't you get "symbol already defined" if you link two TUs generating the same thunk together?
> > > These should be merged like inline functions.  They should be linkonce_odr for externally visible types and internal for internal types.  We probably have logic for that somewhere.
> > I don't see this in the lit test expectations:
> > 
> >   define x86_thiscallcc void @"\01??_9C@@$B7AE"
> Yes, I agree with you, the logic needs to change.  :)  I'm just adding that if the type is internal than we can make the thunk internal too, which is slightly better for the optimizers.  We can probably reuse some existing code for this.
Yeah, I think the answer to "Do we need this?" is "no" :)

That means they get linkonce_odr linkage, which should work right?


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