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

[Timur Iskhodzhanov]

I've renamed the -constructors test to -structors in r174966,
updating the patch accoridngly.

2013/2/9 Timur Iskhodzhanov <timurrrr at google.com>:
> 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



--
Timur Iskhodzhanov,
Google Russia
Timur Iskhodzhanov,
Google Russia



2013/2/9 Timur Iskhodzhanov <timurrrr at google.com>:
> 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_1.patch
Type: application/octet-stream
Size: 28296 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130212/bd6dc307/attachment.obj>