[cfe-dev] Adding support for multiple non-virtual inheritance for -cxx-abi microsoft

[Timur Iskhodzhanov]

2013/4/8 John McCall <rjmccall at apple.com>:
> On Apr 8, 2013, at 11:02 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>> 2013/4/8 John McCall <rjmccall at apple.com>:
>>> On Apr 8, 2013, at 9:29 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>>> I wanted to work on the multiple non-virtual inheritance support for
>>>> the Microsoft ABI.
>>>> I already have a local patch that generates code that works, but I
>>>> think it requires quite some polishing.
>>>>
>>>> There are a few general questions I'd like to ask first as the Clang
>>>> architecture assumes some high-level things that are not true in the
>>>> Microsoft ABI.
>>>>
>>>> Let's assume we have
>>>> struct A { virtual void a(); }
>>>> struct B { virtual void b(); }
>>>> struct C : A, B { virtual void b(); }
>>>>
>>>> In Itanium ABI, C::b takes C* as an implicit "this" parameter;
>>>> in order to work with B* pointers, there is an adjusting thunk in the
>>>> C's vtable slot for "b".
>>>>
>>>> In Microsoft ABI, C::b takes B* as an implicit "this" parameter.
>>>> No thunks are needed for vtable generation.
>>>> I've only observed thunks when I took a member pointer for a class
>>>> with multiple inheritance, but it was not specific to any particular
>>>> method.
>>>
>>> Interesting.  Does MSVC make a thunk for B's vtable in this, or does it
>>> just double-emit the function body?
>>>  struct A { virtual void a(); };
>>>  struct B { virtual void a(); };
>>>  struct C : A, B { virtual void a(); };
>> Great question - it does emit a B-to-(A,C) adjusting thunk for the "C
>> vtable for the B part"!
>>
>>> I also find it curious that MSVC uses a thunk for member pointers, since
>>> the required this-adjustment is already plainly expressible in the member
>>> pointer value.
>> Me too actually.
>> Reid, wdyt?
>>
>>>> Q1) Passing this to overriden methods
>>>> I assume the type of C::b should be "void <...>(%struct.B* %this)" -
>>>> does this look reasonable?
>>>
>>> Yes, that seems reasonable.
>>>
>>>> Can you suggest how to change the CodeGen to handle this appropriately?
>>>> Do you know a non-painful way to do that?
>>>> I think I'll need to:
>>>> a) adjust CodeGenTypes::arrangeCXXMethodType
>>>> ... and now it needs not only the RD and FTP, but MD also.
>>>> Instead of "argTypes.push_back(GetThisType(Context, RD));"
>>>> I'll probably need to ask CGCXXABI on the this type?
>>>
>>> I would just make sure to pass the right RD down.
>> Ah, yeah, I've missed this idea :)
>>
>>> The code calling
>>> this will need to be significantly different anyway, since the call algorithm
>>> is different.
>>>
>>>> Is there an easy way to tell the most base class which declared a
>>>> given virtual method?
>>>
>>> That doesn't have a unique answer.
>>>
>>> You can walk up the override chains, although it's possible that you'll
>>> need to stop at a virtual base boundary.
>>>
>>>> b) adjust all the places where the method may be called.
>>>> btw, is there an easy way to know the base class offset in a given
>>>> class in the CGCall ?
>>>
>>> You'll need to construct a base path as you walk the override chains,
>>> and then basically do that derived-to-base conversion.
>>>
>>>> We do the same in Clang, but that'd require some API changes to
>>>> VTableBuilder - i.e. you'll need to know a pair of classes to generate
>>>> a vtable, not just one.
>>>
>>> As Reid suggested, I think the way to go here is to make VTableBuilder
>>> become ItaniumVTableBuilder and make a new MicrosoftVTableBuilder.
Do you think it's OK to do the rename as the first step, with
post-commit review?

>>> There should be a lot of common behavior you can share between
>>> the implementations.
>> Should they have a common base / interface or just separate at first?
>
> If you think there's something that can be interestingly abstracted out, feel
> free, but I'm not sure what it would be off-hand.

Just wanted to clarify if I'm reading this correctly: your suggested
default approach is "just write a new separate MicrosoftVTableBuilder
from scratch", right?
Do you think it's OK to put it into the same VTableBuilder.cpp file so
it's easier to reuse some handy static functions out there?
Probably a better filename would be VTableBuilders.cpp as it'll be
building different kinds of vtables (one-for-all vtable in ItaniumABI,
vftables and maybe vbtables in Microsoft ABI).