[Review, -cxx-abi microsoft] Implement proper handling of virtual destructors (single-inheritance case)

[Timur Iskhodzhanov]

John,

To save us both some time on the review, I've decided to split the
patch into a few parts.

Attached is a patch that only generates the deleting destructors.

I've intentionally skipped ABI-specific emission of vdtor calls and
the mangling/handling of base ctors/dtors.
I do plan to work on these in later reviews when this one is done.
[Please note that skipped stuff is broken already, so nothing should degrade]

Can you please review?

Thanks!

2013/2/5 John McCall <rjmccall at apple.com>:
> On Feb 5, 2013, at 7:48 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>> 2013/2/2 John McCall <rjmccall at apple.com>:
>>> The Microsoft ABI doesn't distinguish between complete-object and
>>> base-subobject constructors and destructors, so something like
>>> GetAddrOfCXXDestructor should really just never be passed Dtor_Base,
>> I'd like to change the Dtor_Base behavior in the next patches to
>> reduce this one.
>
> As you like.
>
>>> and the mangler should never be asked to mangle one.  You can and
>>> should assert on that.
>> You mean like this:
>>  ...
>>  case Dtor_Base:
>>    llvm_unreachable("Mangling of base dtor should never happen in
>> Microsoft ABI");
>>  ...
>> ?
>
> That's what I meant, yes.

To avoid breaking other tests I've temporarily left the fall-through
but added a FIXME asking to add llvm_unreachable when base dtor
handling is fixed.

>>> Instead, an API like EmitCXXDestructorCall should, for the Microsoft
>>> ABI, just add the implicit extra argument as appropriate for the dtor kind,
>>> the same way that the Itanium implementation considers it when
>>> deciding how to build the VTT parameter.
>> Absolutely; just wanted to work on that as a separate "fix ctor/dtor
>> handling vs inheritance" patch.
>
> Sure.
>
>>> +  bool IsMicrosoftABI;
>>> +
>>>
>>> Please don't do this.  Any place where you're tempted to explicitly
>>> check ABI kind should instead be abstracted into the CGCXXABI class.
>> Hm, can you suggest how to do this without copying/duplicating lots of
>> code and/or give some more hints so we don't spend the first few
>> review iterations copying dozens LOC between files?
>>
>> e.g. it's not clear to me how to abstract stuff out of
>> CodeGenFunction::EmitCXXMemberCallExpr and EmitObjectDelete.
>
> I would just abstract out the entire operation of calling a virtual destructor.
> That is, if the destructor is virtual, and you can't devirtualize, ask the ABI
> to do it.  Destructors end up essentially their own code path through
> EmitCXXMemberCallExpr anyway.

OK, sounds good - this will be in the next patch when this one is committed.

>>> +  // CXXShouldDeleteDecl - When generating code for a C++ scalar deleting
>>> +  // destructor in the Microsoft ABI, this will hold the implicit boolean
>>> +  // telling whether to call operator delete at the end of the destructor.
>>> +  ImplicitParamDecl *CXXShouldDeleteDecl;
>>> +  llvm::Value *CXXShouldDeleteValue;
>>>
>>> I don't think you actually need this.  Instead, in the prologue to a deleting
>>> destructor, just pull this value off the argument list and push a cleanup.
>> Hm.
>> I need to pass the implicit param value to the
>> CallDtorDeleteConditional cleanup.
>>
>> Currently, I get the value in
>> MicrosoftCXXABI::EmitInstanceFunctionProlog, store it in CGF and pass
>> it to the cleanups in CGF::EmitDtorCleanups.
>>
>> Do you suggest to
>> a) get the value in MicrosoftCXXABI::EmitInstanceFunctionProlog and
>> immediately push a cleanup there
>> or
>> b) get the value in CGF::EmitDtorCleanups and immediately push a cleanup there?
>
> Actually, on second thought, it's not unreasonable to store this in CGF.
> Instead of making a new variable, though, please generalize the way
> that the VTT parameter is handled.  You're already moving EmitCXXMethodCall
> towards a general concept of an extra implicit argument;  it makes sense
> for CGF to similarly generalize the extra implicit parameter variable.
Done.
Maybe not perfectly though - I feel a little awkward that now we have
a "generic" variable thas has one meaning in one ABI and a different
one in the other; especially in CodeGenFunction::EnterDtorCleanups
where we decide what cleanup to push.
Though probably with enough tests this shouldn't be a concern.

What do you think?

> John.



--
Timur Iskhodzhanov,
Google Russia
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_15058_p1.patch
Type: application/octet-stream
Size: 28309 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130209/04797f5e/attachment.obj>