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

[Hans Wennborg]

  Thanks for the comments! Sorry I missed some before, I blame Phabricator.

  New patch coming up.


================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1972
@@ +1971,3 @@
+  CGF.FinishFunction();
+  CGM.setFunctionLinkage(MD, ThunkFn); // XXX: Do we need this?
+
----------------
Hans Wennborg wrote:
> 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?
> I don't see this in the lit test expectations:
>
> define x86_thiscallcc void @"\01??_9C@@$B7AE"

Right, I failed to update the test to check for the linkage :(

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1972
@@ +1971,3 @@
+  CGF.FinishFunction();
+  CGM.setFunctionLinkage(MD, ThunkFn); // XXX: Do we need this?
+
----------------
Hans Wennborg wrote:
> Hans Wennborg wrote:
> > 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?
> > I don't see this in the lit test expectations:
> >
> > define x86_thiscallcc void @"\01??_9C@@$B7AE"
> 
> Right, I failed to update the test to check for the linkage :(
> 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.

Doing this.

================
Comment at: lib/CodeGen/CGVTables.cpp:243
@@ +242,3 @@
+                                 const CGFunctionInfo &FnInfo) {
+  assert(!CurGD.getDecl() && "CurGD was already set!");
+  CurGD = GD;
----------------
Timur Iskhodzhanov wrote:
> How about "Please use a new CGF for this thunk" instead?
> I think such a string would make it clearer what one is doing wrong if this fires.
Done.

================
Comment at: lib/CodeGen/CodeGenFunction.h:1159
@@ +1158,3 @@
+  void EmitCallAndReturnForThunk(GlobalDecl GD, llvm::Value *Callee,
+                                 llvm::Value *AdjustedThisPtr,
+                                 const ThunkInfo *Thunk);
----------------
Timur Iskhodzhanov wrote:
> Is there any strong reason why "this" adjustment can't be performed by this function?
Yes, the vcall thunks shouldn't do this-adjustment.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1005
@@ +1004,3 @@
+  CGF.EmitCallAndReturnForThunk(MD, Callee, This, 0);
+
+  CGF.FinishFunction();
----------------
Timur Iskhodzhanov wrote:
> Just curious: there's an AutoreleaseResult assignment before FinishFunction when emitting standard thunks.
> Have you checked whether we need that for vmemfunptr thunks?
Hmm, we probably should actually.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1434
@@ +1433,3 @@
+      FirstField = llvm::Constant::getNullValue(CGM.VoidPtrTy);
+    } else if (CGM.getMicrosoftVTableContext()
+                   .getMethodVFTableLocation(MD)
----------------
Reid Kleckner wrote:
> Timur Iskhodzhanov wrote:
> > how about
> > 
> >   } else {
> >     ... &ML = CGM.getMicrosoftVTableContext().getMethodVFTableLocation(MD);
> >     if (ML.VBase) {
> >        ...
> >     } else {
> >        ...
> >     }
> >   }
> > 
> > ?
> +1
The nesting is starting to get deep here :) Pulling out the definition of ML seems like a good idea though, I'll do that.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1446
@@ +1445,3 @@
+      bool Is64Bit = getContext().getTargetInfo().getPointerWidth(0) == 64;
+      int OffsetInVFTable = ML.Index * (Is64Bit ? 8 : 4);
+      llvm::raw_svector_ostream Out(ThunkName);
----------------
Timur Iskhodzhanov wrote:
> Earlier comment by Reid says:
> 
> IMO the right way to write this is something like:
> 
>   ML.Index * getContext().getTypeSizeInChars(getContext().VoidPtrTy)
> 
Done. Sorry I missed that comment before.


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