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

[Timur Iskhodzhanov]

OK, will do that.

When I have the patch ready - should I wait for another review or can
go ahead and commit?
(if the latter - I'm willing to fix post-commit issues, if any)
Timur Iskhodzhanov,
Google Russia



2013/1/18 John McCall <rjmccall at apple.com>:
> On Jan 17, 2013, at 8:15 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>> 2013/1/17 John McCall <rjmccall at apple.com>:
>>> On Jan 16, 2013, at 6:11 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>>
>>> 2013/1/16 John McCall <rjmccall at apple.com>
>>>>
>>>> On Dec 29, 2012, at 6:48 AM, Timur Iskhodzhanov <timurrrr at google.com>
>>>> wrote:
>>>>> Please review the new patch attached.
>>>>>
>>>>> I've put an extra FIXME for the RTTI handling
>>>>> and added a new check to the constructor CodeGen test.
>>>>
>>>> Sorry about the delay.
>>>
>>> Please tell me if there's something actionable for me to improve my
>>> emails/patches to reduce delays/number of iterations.
>>> e.g. read these and these docs, read XYZ more carefully, do ABC rather than
>>> KLM, etc.
>>>
>>>
>>> It's really just that I've been very busy recently, as you can probably tell
>>> from my commit history. :)  I don't have an immediate solution.
>>
>> I'm curious if I could write simpler/shorter patches to not exceed
>> your time slice and get the reviews scheduled more often :)
>>
>>>> Instead of repeating
>>>>  Context.getTargetInfo().getCXXABI() != CXXABI_Microsoft
>>>> all over the place, please introduce an isItaniumABI() predicate.
>>>> Also, consider caching it as a field of VTableContext.
>>>
>>> Unfortunately, I'm afraid havin isItaniumABI() is not very helpful as we may
>>> also see CXXABI_ARM.
>>> Am I right?
>>>
>>>
>>> The ARM ABI is a variant of the Itanium ABI;  my intent was that
>>> isItaniumABI would return true for it.  But it's not that important;
>>> caching IsMicrosoftABI is fine for now.
>>
>> OK
>>
>>>> +    assert(!isMicrosoft &&
>>>> +           "Implicit virtual dtors are not yet supported in MS ABI");
>>>> +
>>>>
>>>> This is not an appropriate use of assert.
>>>
>>> I've switched back to llvm_unreachable.
>>> Please tell me if I'm doing it wrong too!
>>>
>>>
>>> Neither of these is appropriate!  The compiler should never crash just
>>> because something is unsupported.  Consider adding a method to emit
>>> an "unsupported" diagnostic here (you can follow IRGen's lead).
>>
>> Oh I see now. Actually this was my intent to crash early rather than
>> continuing (works better in my setup) but I agree with a more general
>> practice.
>>
>> Please see a patch with diagnostics attached.
>
> -    uint64_t AddressPoint = AddressPoints.lookup(Base);
> -    assert(AddressPoint && "Address point must not be zero!");
> -
> -    return AddressPoint;
> +    return AddressPoints.lookup(Base);
>
> Since you have the isMicrosoftABI() bit now, could you just tweak the
> assert to (AddressPoint != 0 || isMicrosoftABI())?
>
> +  void Error(SourceLocation loc, StringRef error);
>
> I suggest calling this ErrorUnsupported and making the 'error'\
> text a brief description of what's not supported.  So you'd call this like:
>   ErrorUnsupported(loc, "implicit virtual destructors in the Microsoft ABI")
> and the diagnostic string you register would be something like:
>   "v-table layout for %0 is unsupported"
>
> This makes these diagnostics more consistent (not that that's *so*
> important, but still) and also makes it obvious that this is just intended
> as a way to report an incomplete implementation and is not intended
> as a general error-reporting mechanism.
>
> Otherwise this looks good.
>
> John.