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

[John McCall]

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.

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.

>> +  // 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.

John.