[Review, -cxx-abi microsoft] Implement proper handling of virtual destructors (single-inheritance case)
rjmccall at apple.com
Tue Feb 5 11:52:40 PST 2013
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.
>> 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.
>> + 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
>> + // 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.
> 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
> 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.
More information about the cfe-commits