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

[Timur Iskhodzhanov]

2013/2/2 John McCall <rjmccall at apple.com>:
>
> On Feb 1, 2013, at 9:18 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>
>> Ah, I see.
>> I should probably look at the ItaniumMangler more often :)
>>
>> Attached is the patch that resolves both issuew Charles has asked me to fix.
>> Anything else I should change?
>
> +void MicrosoftCXXNameMangler::mangleCXXCtorType(CXXCtorType T) {
> +  switch (T) {
> +  case Ctor_Base:
> +    // FIXME: base ctors are not supported yet. Even though the current
> +    // codegen won't emit base ctors, we still need to mangle a base
> +    // ctor for a base class construction in case of inheritance.
> +    // For now, just mangle a base ctor the same way as a complete ctor.
> +  case Ctor_Complete:
> +    Out << "?0";
> +    break;
> +  default:
> +    DiagnosticsEngine &Diags = Context.getDiags();
> +    unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
> +      "cannot mangle this ctor type yet");
> +    Diags.Report(Structor->getLocation(), DiagID);
> +  }
> +}
> +
> +void MicrosoftCXXNameMangler::mangleCXXDtorType(CXXDtorType T) {
> +  switch (T) {
> +  case Dtor_Deleting:
> +    Out << "?_G";
> +    break;
> +  case Dtor_Base:
> +    // FIXME: base dtors are not supported yet. Even though the current
> +    // codegen won't emit base dtors, we still need to mangle a base
> +    // dtor for a base class destruction in case of inheritance.
> +    // For now, just mangle a base dtor the same way as a complete dtor.
> +  case Dtor_Complete:
> +    Out << "?1";
> +    break;
> +  default:
> +    llvm_unreachable("Unsupported dtor type?");
> +  }
> +}
>
> As a general matter, prefer using exhaustive switches without 'default' cases.
OK, will do.

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

> 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");
  ...
?

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

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

> Out of curiosity, why exactly is this parameter there?  Does the v-table
> only contain a pointer to the deleting destructor,
Yes, as we've discussed in the previous change.

> and this parameter is
> passed to distinguish between 'delete p' and 'p->~A()'?
Yes, you need a param to distinguish two behaviors accessible using
only one function pointer.

--
Timur Iskhodzhanov,
Google Russia