[cfe-commits] [Review] First step towards PR13231 - vftable generation with -cxx-abi microsoft

[Timur Iskhodzhanov]

On Thu, Nov 29, 2012 at 2:52 AM, John McCall <rjmccall at apple.com> wrote:
> On Nov 15, 2012, at 8:43 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>> Can you please review the attached patch?
>
> Sorry about the delay.
This time my apologies :)
Please take a look at an updated patch.

>> With this patch, I can compile and run simple single-inheritance tests;
>> even method calls from Clang-generated obj files to CL-generated
>> methods work (and vice versa).
>>
>> Some notes:
>> a) It only works with -fno-rtti since we can't mangle RTTI yet,
>
> Okay, but I'm curious about something here.  It looks like you're not even
> allocating space in the v-table for the RTTI pointer if RTTI is disabled?
Yes - but it turned out this is not necessary.
When I was working on the patch initially the COFF writer formatted
vftables wrong, so I thought this was a problem.
Now that COFF is fixed, turns out I can put RTTI back - thanks for noticing!

> That means that every translation unit using the class needs to agree
> about whether RTTI is enabled, or else they might miscompile.  If that's
> what MSVC does, fine, but if not and what you're really trying to do is
> just punt on RTTI for now, why not just always emit RTTI pointers as null?
>
>> b) I had to remove "AddressPoint != 0" assertion since AddressPoint
>> should be 0 for some classes in -cxx-abi microsoft,
>
> You could weaken the assertion instead of removing it.
it's uint64_t, so weakening the "x != 0" assertion is just assert('x
is any'), right? :)
However, now that I've put RTTI pointer back, I can put the assertion
back as well.

>> c) I've added conditionals rather than a layer of abstraction per
>> http://llvm.org/bugs/show_bug.cgi?id=13231#c5,
>>
>> d) The complete/deleting dtor logic will change when we support MSVC
>> dtor types, but not now
>>    [shouldn't matter now that we still don't support virtual inheritance]
>
> You're thinking about complete vs. base destructors.  Complete vs. deleting
> destructors isn't about virtual inheritance;  it's about whether the destructor
> calls operator delete.
Yeah, now I see it... [see below]

> (This is important because the 'delete' operator uses
> the operator delete from the most-derived class.)
>
> How does MSVC handle the difference between
>   foo->~Foo(); // virtually dispatched but doesn't call operator delete
> and
>   delete foo; // virtually dispatched and calls operator delete
> ?
Clang with my patch behaves badly on such code.
It seems to work fine at least on simple code where you don't use
virtual destructors.

Actually, the whole idea of base vs complete dtors doesn't apply to
-cxx-abi microsoft.
Anyways, had plans to work on ctor and dtor compatibility as a
separate patch - is that OK?
I've put a couple of more FIXME into the code for that. (in
::AddMethod and ::ComputeMethodVTableIndices).

> Code notes:
>
> +
> +  // FIXME: Should probably add a layer of abstraction for vtable
> +  // generation, see http://llvm.org/bugs/show_bug.cgi?id=13231#c5
> +  bool MsABI = (Context.getTargetInfo().getCXXABI() == CXXABI_Microsoft);
>
> Please don't litter the code with PR references.
Done

> Also, please name this isMicrosoft or something.
Done

> This looks like "Ms. ABI".
:)

> +    if (MsABI)
> +      llvm_unreachable("Haven't seen an implicit virtual dtor in MS mode");
> +
>
> If this is right, then this should be an assert.  But I don't understand if it's
> right, because I don't understand what the message is trying to say.
> Is this an invariant?  A language rule? An unimplemented feature?
The section has a "Itanium C++ ABI" comment, and I haven't seen any
code yet that goes inside that if() with -cxx-abi microsoft
so I've decided to put an unreachable rather than adding untested code.
Again, we'll need to resolve the virtual dtors anyway...
I've put a more descriptive assert() into the code there.

> John.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_13231_2.patch
Type: application/octet-stream
Size: 11450 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121204/550c3cbb/attachment.obj>