[PATCH] [ms-cxxabi] Get closer to emitting the correct set of destructors in each TU (PR12784).

[Timur Iskhodzhanov]

Hi Peter,

lib/AST/MicrosoftMangle.cpp
@@ -551,17 +551,18 @@ void
MicrosoftCXXNameMangler::manglePostfix(const DeclContext *DC,
 void MicrosoftCXXNameMangler::mangleCXXDtorType(CXXDtorType T) {
   switch (T) {
   case Dtor_VectorDeleting:
     Out << "?_E";
     return;
   case Dtor_Deleting:
     Out << "?_G";
     return;
   case Dtor_Base:
     Out << "?1";
     return;
   case Dtor_Complete:
     Out << "?_D";
     return;
   }
--
Why do you think Complete is "?_D" and Base is "?1" ?
I thought "?1" stands for the "usual" destructor (complete) whilst
"?_D" stands for vbase destructor?
See test below.

test/CodeGenCXX/microsoft-abi-static-initializers.cpp
@@ -11,7 +11,7 @@ struct S {
 // CHECK: ret void

 // CHECK: define internal void @"__dtor_\01?s@@3US@@A"() [[NUW]] {
-// CHECK: call x86_thiscallcc void @"\01??1S@@QAE at XZ"
+// CHECK: call x86_thiscallcc void @"\01??_DS@@QAE at XZ"
 // CHECK: ret void
 ---
I believe this is wrong.
CL does not emit a "??_DS@@" destructor on this file and it calls
"??1S@@..." when "s" is to be destroyed.


lib/AST/VTableBuilder.cpp
@@ -1951,7 +1951,7 @@ void VTableBuilder::dumpLayout(raw_ostream& Out) {
       if (IsComplete)
         Out << "() [complete]";
       else if (isMicrosoftABI())
-        Out << "() [scalar deleting]";
+        Out << "() [vector deleting]";
       else
         Out << "() [deleting]";
---
Hm, why do you think we should put a vector deleting dtor in the vftable?
Please look at http://llvm.org/bugs/show_bug.cgi?id=15058#c4 - if we
don't emit the deleting dtor definition, the linker error happens for
a scalar deleting dtor.
I think the "scalar" dtor deletes one object whilst the "vector" dtor
deletes multiple objects (e.g. when used with "delete []").
So I don't think we should change what's printed in the
VTableBuilder::dumpLayout.

The vector and scalar deleting dtors are usually aliases, right?
Are you sure the scalar dtor is an alias for a vector dtor, not vice versa?

lib/CodeGen/MicrosoftCXXABI.cpp
@@ -328,17 +331,18 @@ RValue
MicrosoftCXXABI::EmitVirtualDestructorCall(CodeGenFunction &CGF,
...
   ASTContext &Context = CGF.getContext();
   llvm::Value *ImplicitParam
-    = llvm::ConstantInt::get(llvm::IntegerType::getInt1Ty(CGF.getLLVMContext()),
-                             DtorType == Dtor_Deleting);
+   = llvm::ConstantInt::get(llvm::IntegerType::getInt32Ty(CGF.getLLVMContext()),
+                            DtorType == Dtor_Deleting);
---
Are you sure there's a 32-bit argument? I'm sure I put Int1/bool on
purpose because I saw zero extension somewhere. Is there any case
where something other than 0/1 is passed as an argument to
scalar/vector deleting dtor?


lib/CodeGen/CGCXX.cpp
@@ -230,15 +239,17 @@ CodeGenModule::GetAddrOfCXXConstructor(const
CXXConstructorDecl *ctor,
 }

 void CodeGenModule::EmitCXXDestructors(const CXXDestructorDecl *D) {
...
+  if (getTarget().getCXXABI().hasDestructorVariants()) {
---
Hm, is there any C++ ABI which doesn't have destructor variants? :)


2013/4/26 Peter Collingbourne <peter at pcc.me.uk>:
> Hi Timur,
>
> Patch attached -- please review.
>
> The main missing part of this patch is vector deleting
> destructors, which we don't need ourselves but may be
> required by MSVC TUs.
Do you have an example when we really need them?

I think they are only needed for polymorphic "operator delete[]",
which stackoverflow says to be an undefined behavior:
http://stackoverflow.com/questions/6171814/why-is-it-undefined-behavior-to-delete-an-array-of-derived-objects-via-a-base

If that's just "nice to have", I'd personally like to defer that until
we have a justification to use them and real-world example of code
using it. Otherwise we don't even know if we implement it correctly :)

--
Timur