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

[John McCall]

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.

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,
and the mangler should never be asked to mangle one.  You can and
should assert on that.

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.

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

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

Out of curiosity, why exactly is this parameter there?  Does the v-table
only contain a pointer to the deleting destructor, and this parameter is
passed to distinguish between 'delete p' and 'p->~A()'?

John.