[PATCH] Add MicrosoftVFTableContext to AST

[Timur Iskhodzhanov]

2013/7/25 John McCall <rjmccall at gmail.com>:
> 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:
>
> +// 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”.

Rephrased to:
//  2. Each subobject with a vfptr gets its own vftable rather than an address
//     point in a single vtable shared between all the subobjects.

Does this make sense now?

> +//  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.

Great text, done!
I changed the virtual base example in parenthesis though, as your
example doesn't necessarily requires a thunk.

> +//  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.

Ah, OK. I agree this is just a consequence of another clause, but I
think it was important enough to state it separately.
It's not needed now with your version of clauses 3 and 4, removed.

> 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.
>
> +  // 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.

Hm...

Let's consider
--------
  struct A {
    virtual TYPE* f();
  };
  struct B {
    virtual TYPE* g();
  };
  struct C: A, B {  <something>  }
--------
We'll have two vfptrs: for A at offset 0 and for B at offset 4
(assuming 32-bit arch).

Currently, for the B's vftable we'll do this:
  enter AddMethods(C)
    enter AddMethods(B)
    allocate a vftable slot for B::g
    leave AddMethods(B)
  update the B::g slot with this/return adjustment if C overrides it
  leave AddMethods(C)
[this somewhat reflects how Itanium's VTableContext works]

Basically what you want is
  enter AddMethods(B)
  allocate a vftable slot for B::g,
    if we have a (final) overrider for g() in C,
      calculate this/return adjustment right here*.
  leave AddMethods(B)

Ok, so at the (*) stage we can probably find the FinalOverrider to
just write the adjustments immediately...
This implies rewriting ComputeThisOffset to just take the final
overrider and return the this offset in a complete object.
There were a few minor complexities in rewriting it (e.g. API being
not convenient) that have overloaded my brain on Friday evening
though.

Do you think this is important for the first version? If so - I'll
continue trying to do this next week.

2013/7/26 John McCall <rjmccall at gmail.com>:
> On Jul 25, 2013, at 10:16 AM, Timur Iskhodzhanov <timurrrr at google.com>
> wrote:
>
>  Fixed wording in one of the comments.
>
>
> +visitAllOverriddenMethods(const CXXMethodDecl *MD,
> +                          VisitorTy &Visitor,
> +                          OverriddenMethodsSetTy &VisitedMethods) {
>
> Don’t have this take VisitedMethods.  Make the visitor that cares about
> collecting methods collect them.

Oh, that's a good idea - done.

> -ComputeAllOverriddenMethods(const CXXMethodDecl *MD,
> -                            OverriddenMethodsSetTy& OverriddenMethods) {
>
> You can actually leave this function around if you like.  It’s just:
>
> namespace {
>   struct MethodCollector {
>     OverriddenMethodsSetTy *Methods;
>     bool visit(const CXXMethodDecl *MD) {
>       Methods->push_back(MD);
>       return true;
>     }
>   };
> }
> void ComputeAllOverriddenMethods(const CXXMethodDecl *MD,
>                             OverriddenMethodsSetTy& OverriddenMethods) {
>   MethodCollector collector = { &OverriddenMethods };
>   visitAllOverriddenMethods(MD, collector);
> }
>
> +static const CXXMethodDecl *
>  FindNearestOverriddenMethod(const CXXMethodDecl *MD,
> -                            VTableBuilder::PrimaryBasesSetVectorTy &Bases)
> {
> +                            BasesSetVectorTy &Bases) {
>
> Sorry, I didn’t mean to ask you to rewrite this algorithm; I just wanted a
> way to
> re-use the recursive visitation without redundantly copying them out into an
> array
> if that wasn’t needed.

Oops, fixed!

> +    ThisAdjustment ThisAdjustmentOffset;
> +    // Check if this virtual member function overrides
> +    // a method in one of the visited bases.
> +    if (const CXXMethodDecl *OverriddenMD =
> +            FindDirectlyOverriddenMethodInBases(MD, VisitedBases)) {
> +      // Replace the method info of the overridden method with our own
> +      // method - only if it's in the vftable we're laying out.
> +      MethodInfoMapTy::iterator OverriddenMDIterator =
>
> I’m confused about what’s actually going into MethodInfoMap and what
> needs to be replaced, exactly.

OK, I've slightly changed the comments there too.

> John.