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

[Hans Wennborg]

  Thanks for the review! New patch coming up.

  > 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?

  It's based on three functions: CodeGenVTables::emitThunk, CodeGenModule::getAddrOfThunk, and CodeGenFunction::GenerateThunk (this is the meaty one).


================
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;
----------------
Timur Iskhodzhanov wrote:
> Do we need it for the Itanium ABI?
> What do you think about moving this to MicrosoftMangleContext?
Nope, we don't need it for the Itanium ABI. Moving to MicrosoftMangleContext works nicely.

================
Comment at: lib/AST/MicrosoftMangle.cpp:1893
@@ +1892,3 @@
+  Mangler.getStream() << "$B";
+  Mangler.mangleNumber(VTableIndex * (Is64Bit ? 8 : 4));
+  Mangler.getStream() << "A";
----------------
Timur Iskhodzhanov wrote:
> 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.
Done.

================
Comment at: lib/CodeGen/CGVTables.cpp:365
@@ +364,3 @@
+  CGM.SetLLVMFunctionAttributes(MD, FnInfo, ThunkFn);
+  CGM.SetLLVMFunctionAttributesForDefinition(MD, ThunkFn);
+
----------------
Timur Iskhodzhanov wrote:
> Why so many SetLLVMFunctionAttributes* calls?
I'm following CodeGenModule::GetOrCreateLLVMFunction, which calls SetLLVMFunctionAttributes and CodeGenVTables::emitThunk which calls SetLLVMFunctionAttributesForDefinition.

But I now see that I'm calling one of these twice - that was unintentional :)

================
Comment at: lib/CodeGen/CGVTables.cpp:367
@@ +366,3 @@
+
+  CurGD = MD;
+
----------------
Timur Iskhodzhanov wrote:
> ~~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 ?
Even better, I'll assert that CurGD wasn't already set.

================
Comment at: lib/CodeGen/CGVTables.cpp:389
@@ +388,3 @@
+  // Get to the vtable from 'this'.
+  llvm::Value *VTable =
+      GetVTablePtr(This, ThunkTy->getPointerTo()->getPointerTo());
----------------
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?

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1362
@@ +1361,3 @@
+    int VTableIndex =
+        CGM.getMicrosoftVTableContext().getMethodVFTableLocation(MD).Index;
+    CodeGenFunction CGF(CGM);
----------------
Timur Iskhodzhanov wrote:
> 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.
Yes, I think it's better to use getVirtualFunctionPointer(), as mentioned above, instead of doing this manually based on the index.

================
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
----------------
Timur Iskhodzhanov wrote:
> 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...
Done.

================
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
----------------
Timur Iskhodzhanov wrote:
> 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.
Done.

================
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
----------------
Timur Iskhodzhanov wrote:
> dumpbin demangles this as
> 
>   [thunk]: __thiscall C::`vcall'{0,{flat}}' }'
> 
> Do you know what "flat" stands for?
No :(


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