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

[John McCall]

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.