[PATCH] Add MicrosoftVFTableContext to AST

[John McCall]

On Jul 25, 2013, at 10:20 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> 2013/7/25 John McCall <rjmccall at gmail.com>:
>> On Jul 22, 2013, at 6:11 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>> 
>> While working on CodeGen, finally realized why we should only put the
>> overriders from the most derived class into MethodVFTableLocations
>> 
>> 
>> --- lib/AST/MicrosoftMangle.cpp
>> +++ lib/AST/MicrosoftMangle.cpp
>> @@ -89,6 +89,8 @@
>>   void mangleNumber(const llvm::APSInt &Value);
>>   void mangleType(QualType T, SourceRange Range,
>>                   QualifierMangleMode QMM = QMM_Mangle);
>> +  void mangleFunctionType(const FunctionType *T, const FunctionDecl *D,
>> +                          bool IsStructor, bool IsInstMethod);
>> 
>> Is there a reason the mangling changes are part of this patch?
> 
> Yes - the still-used Itanium VTableBuilder tries to emit thunks for
> its vtables from CreateVTableInitialize/GetAddrOfThunk.

Okay.

>> +// The main differences are:
>> +//  1. Separate vftable and vbtable.
>> +//  2. Each non-primary base class that has new virtual methods gets its
>> +//     own vfptr and vftable, all the address points are zero.
>> 
>> This is not a difference.
> 
> As far as I understand, in Itanium ABI each subobject that adds new
> virtual methods to its bases gets a new *address point* in the shared
> vtable, but not a new vtable (at least no new sections are generated),
> which IS different from the Microsoft ABI.

I’m not sure what it means for a subobject to "add virtual methods to its bases”.

>> +//  3. Rather than using the most derived class as the type of the hidden
>> 'this'
>> +//     method parameter, it tries to stick with the original 'this' type as
>> +//     long as possible.
>> +//     This yields the following consequences:
>> +//     - Don't need to add all overriders to the vftables of their parents;
>> +//     - Don't need adjusting thunks as long as there are no methods
>> overriding
>> +//       multiple methods at the same time.
>> 
>> Again, this has things a little backwards.  You can *never* change the
>> expected
>> address point of an existing entry in a v-table, or else you wouldn’t be
>> able
>> to use the entry correctly from a base subobject pointer.
> 
> I don't see any conflict with my comment here.
> Can you please suggest your variant of this clause?

3. Virtual method definitions expect their ‘this’ parameter to point to the
first vfptr whose table provides a compatible overridden method.  In many
case, this permits the original vf-table entry to directly call the method
instead of passing through a thunk.

A compatible overridden method is one which does not have a non-trivial
covariant-return adjustment.

The first vfptr is the one with the lowest offset in the complete-object layout
of the defining class, and the method definition will subtract that constant
offset from the parameter value to get the real ‘this’ value.  Therefore, if the
offset isn’t really constant (i.e. if the first vfptr is contained in a virtual base
subobject), the vf-table may require a this-adjustment thunk.

4. vf-tables do not contain new entries for overrides that merely require
this-adjustment.  Together with #3, this keeps vf-tables smaller and
eliminates the need for this-adjustment thunks in many cases, at the cost
of often requiring redundant work to adjust the "this" pointer.

>> +//  4. At entry, the 'this' parameter always points to the vfptr used
>> +//     for the vftable lookup.
>> 
>> This is also not a difference.
> 
> It is different in these ABIs.
> 
> Probably we're using different terms, so let's consider this example:
> -----------
>  class A {
>    virtual void f() {}
>  };
> 
>  class B : virtual A {
>    virtual void f();
>  };
> 
>  void foo(void*);
> 
>  void B::f() {
>    foo(this);
>  }
> -----------
> In the Itanium ABI, B::f expects the top of the stack to hold (B*)this at entry:
> -----------
>  __ZN1B1fEv:
>        jmp     __Z3fooPv               # TAILCALL
> -----------
> thus needing a thunk in the vtable.
> 
> In Microsoft ABI, B::f expects ECX to hold (A*)this and adjusts in the preamble.
> -----------
>  "?f at B@@EAEXXZ":
>        pushl   %eax
>        addl    $-4, %ecx    # Adjust (A*) to (B*)
>        movl    %ecx, (%esp)
>        calll   "?foo@@YAXPAX at Z"
>        popl    %eax
>        ret
> -----------
> thus a thunk is not needed.
> 
> I've rephrased it to "at virtual function entry" - does this make sense now?

(This example is misleading because A is actually the primary base of B
under the Itanium ABI, but I think I see what you’re getting at.)

This is still not a difference.  What you’re describing here is just the effect of
a previously-enumerated difference.

In both ABIs, the algorithm for performing a virtual function call is to adjust
the base pointer to a subobject which contains the method in its primary
v-table, load a function pointer from the v-table, and call it.

In your example, the difference is just that, under MSVC, B doesn’t have
an entry for f() in its vf-table.  (In fact, it doesn’t have a vf-table.)  So the
caller has to adjust to something that actually does have an entry for f(),
namely A.

> Phew, actually changing the FindNearestOverriddenMethod code turned
> out not to be as trivial, at least I can't justify the change to
> myself...
> 
> Please take a look at the code - I have several ideas how to change
> the code there, would be glad if you can suggest which one looks
> better to you.

I’ll take a look.

>> +  // See if this class expands a vftable of the base we look at, which is
>> either
>> +  // the one defined by the vfptr base path or the primary base.
>> 
>> Still not sure why you’re not just starting from the right base and then
>> walking up the primary base chain.
> 
> That's not enough even in the simple case of "B: A".
> 
> The vfptr is in the A layout, so the "right base" is A.
> If we don't go to the most derived class (B) from "the right base"
> (A), we forget to add the more derived class's own new methods (and
> probably return-adjusting thunks).

My point is that you should just start recursing from B instead of this weird
combination of walking the path and then falling back on climbing the
primary base chain.

Finding B (the most-derived subobject that A is in the primary-base chain of)
should be really easy — it’s just a depth-first search through the complete
object’s layout, stopping at the first thing with the same offset as A.

>> +    ThisAdjustment ThisAdjustmentOffset;
>> +    // Check if this virtual member function overrides
>> +    // a method in one of the visited bases.
>> +    if (const CXXMethodDecl *OverriddenMD =
>> +            FindNearestOverriddenMethod(MD, VisitedBases)) {
>> 
>> I don’t think you need to compute the nearest overridden method.
>> The way MSVC does this, you only get an entry if the method doesn’t
>> override a method with a trivial (or no) conversion on the return type.
>> So you can just walk the directly-overridden methods
> 
> Good point, change the "iterate over ALL overridden methods" to
> "iterate over directly overridden methods", though this has spawned
> some more duplicate code. Is that OK now?

I’ll look at the patch.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130725/b3ff5852/attachment.html>