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 <rjmcc...@apple.com>:
> On Feb 5, 2013, at 7:48 AM, Timur Iskhodzhanov <timur...@google.com> wrote:
>> 2013/2/2 John McCall <rjmcc...@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
bug_15058_p1.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
