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