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

[John McCall]

On Apr 8, 2013, at 12:29 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> 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?

Sure.

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

More or less.

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

Yeah, that's fine.

> 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).

Abstractly, yes, but it's such a minor win that I'm not sure it's worthwhile.

John.