Suppress generation of a deleting destructor for a dllimport record.

[zahiraam]

Summary

This is a bug that showed up while trying to fix this bug:

https://bugs.llvm.org/show_bug.cgi?id=32990

In the MS ABI, a deleting destructor is not generated when a record has dllimport attribute. The path is to suppress the generation of a deleting destructor.
This patch doesn't fix the bug mentioned in the link above.

[rnk]

lib/CodeGen/CGCXX.cpp

142

I think this was just a bug introduced in rL283063 (@hiraditya). Rather than adding special cases for MS ABI, we should revert that revision and fix it a different way. Let me take a quick look...

[rnk]

lib/CodeGen/CGCXX.cpp

142

I came up with rL315656, which hopefully fixes PR32990, which I think was a regression introduced in rL283063.

lib/CodeGen/MicrosoftCXXABI.cpp

3806

I didn't implement this logic, please rebase this on rL315656 and update the test case in it.

[zahiraam]

lib/CodeGen/CGCXX.cpp

142

This doesn't fix the PR32990.
ksh-3.2$ cat t5.cpp
struct BaseClass
{

~BaseClass();

};

struct __declspec(dllimport) Concrete : virtual BaseClass
{
};

Concrete c;

ksh-3.2$
ksh-3.2$ clang -c t5.cpp
ksh-3.2$ dumpbin /symbols t5.o | grep imp
015 00000000 UNDEF notype External | imp_??0Concrete@@QEAA@XZ (declspec(dllimport) public: cdecl Concrete::C
oncrete(void))
018 00000000 UNDEF notype External | imp_??1Concrete@@QEAA@XZ (declspec(dllimport) public: cdecl Concrete::~
Concrete(void))
ksh-3.2$ dumpbin /symbols t5.obj | grep imp
00B 00000000 UNDEF notype External | imp_??0Concrete@@QEAA@XZ (declspec(dllimport) public: cdecl Concrete::C
oncrete(void))
00C 00000000 UNDEF notype External | imp_??_DConcrete@@QEAAXXZ (declspec(dllimport) public: void cdecl Concr
ete::`vbase destructor'(void))
ksh-3.2$

[erichkeane]

@zahiraam : See @rnk's comment here for steps that need to be taken: I didn't implement this logic, please rebase this on rL315656 and update the test case in it.

[zahiraam]

Added my patch to https://reviews.llvm.org/rL315656 .

[rnk]

lib/CodeGen/MicrosoftCXXABI.cpp

3806

Why is this correct? We cannot alias the complete destructor to the base constructor for a class with virtual bases just because it is dllimport. It wouldn't destroy the virtual bases, right?

test/CodeGenCXX/dllimport-base.cpp

12

Can you remove the semicolons after virtual method definitions here and elsewhere in this test case? They obscure the meaning of the test.

22–23

Generally in test cases we prefer to just plain structs so that we get a default access control level of 'public' so that we can then omit 'public:' from the class body and all of the base specifiers. Can you apply that here and below?

[zahiraam]

Rebasing on rL315656 actually fixes the problem. No deleting destructor is generated. So adding the test case only to insure that is enough. The code of this release is already committed.

lib/CodeGen/MicrosoftCXXABI.cpp

3806

This is right.
However rebasing the code on rL315656 made this test unnecessary. The deleting destructor is not being generated. So adding only the test case to this review is enough.

[rnk]

Can you check the base revision you used to upload the patch? I think there should just be new test case code now.

lib/CodeGen/MicrosoftCXXABI.cpp

3806

It looks like the wrong diff base revision was used for the uploaded diff. This is the code that's already there:

// The complete destructor is equivalent to the base destructor for
// classes with no virtual bases, so try to emit it as an alias.
if (!dtor->getParent()->getNumVBases() &&
    (dtorType == StructorType::Complete || dtorType == StructorType::Base)) {

So, we don't need to change anything. We just want to commit the test case below.

[zahiraam]

In D38803#1030905, @rnk wrote:

Can you check the base revision you used to upload the patch? I think there should just be new test case code now.

Yes. Can you please commit the test case?

[erichkeane]

In D38803#1031303, @zahiraam wrote:

In D38803#1030905, @rnk wrote:

Can you check the base revision you used to upload the patch? I think there should just be new test case code now.

Yes. Can you please commit the test case?

I believe he's saying you should rebase, since an additional testcase has already been added.

[zahiraam]

In D38803#1031304, @erichkeane wrote:

In D38803#1031303, @zahiraam wrote:

In D38803#1030905, @rnk wrote:

Can you check the base revision you used to upload the patch? I think there should just be new test case code now.

Yes. Can you please commit the test case?

I believe he's saying you should rebase, since an additional testcase has already been added.

Oops! I thought I had done it already. Sorry. On it.

I have already done a rebase ...

In D38803#1031304, @erichkeane wrote:

In D38803#1031303, @zahiraam wrote:

In D38803#1030905, @rnk wrote:

Can you check the base revision you used to upload the patch? I think there should just be new test case code now.

Yes. Can you please commit the test case?

I believe he's saying you should rebase, since an additional testcase has already been added.

[zahiraam]

Test case to add.

[zahiraam]

Attached the test case.

lib/CodeGen/CGCXX.cpp

bool CodeGenModule::TryEmitDefinitionAsAlias(GlobalDecl AliasDecl,
   if (!llvm::GlobalAlias::isValidLinkage(Linkage))
     return true;
 
   llvm::GlobalValue::LinkageTypes TargetLinkage =
       getFunctionLinkage(TargetDecl);
 
   // available_externally definitions aren't real definitions, so we cannot
   // create an alias to one.
-  if (TargetLinkage == llvm::GlobalValue::AvailableExternallyLinkage)
+  // Declarations with dllimport attribute have their linkage set to
+  // availableExternallyLinkage. In the MS ABI these represent real
+  // definitions, so an alias can be created.
+  if (TargetLinkage == llvm::GlobalValue::AvailableExternallyLinkage &&
+      !Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+      !AliasDecl.getDecl()->hasAttr<DLLImportAttr>())
     return true;
 
   // Check if we have it already.
   StringRef MangledName = getMangledName(AliasDecl);
   llvm::GlobalValue *Entry = GetGlobalValue(MangledName);
   if (Entry && !Entry->isDeclaration())
     return false;
   if (Replacements.count(MangledName))

lib/CodeGen/MicrosoftCXXABI.cpp

static void emitCXXConstructor(CodeGenModule &CGM,
   // There are no constructor variants, always emit the complete destructor.
   llvm::Function *Fn = CGM.codegenCXXStructor(ctor, StructorType::Complete);
   CGM.maybeSetTrivialComdat(*ctor, *Fn);
 }
 
 static void emitCXXDestructor(CodeGenModule &CGM, const CXXDestructorDecl *dtor,
                               StructorType dtorType) {
   // The complete destructor is equivalent to the base destructor for
-  // classes with no virtual bases, so try to emit it as an alias.
-  if (!dtor->getParent()->getNumVBases() &&
+  // classes with no virtual bases, or classes that have dllimport attribute,
+  // so try to emit it as an alias.
+  if ((!dtor->getParent()->getNumVBases() || dtor->hasAttr<DLLImportAttr>()) &&
       (dtorType == StructorType::Complete || dtorType == StructorType::Base)) {
     bool ProducedAlias = !CGM.TryEmitDefinitionAsAlias(
         GlobalDecl(dtor, Dtor_Complete), GlobalDecl(dtor, Dtor_Base), true);
     if (ProducedAlias) {
       if (dtorType == StructorType::Complete)
         return;
       if (dtor->isVirtual())
         CGM.getVTables().EmitThunks(GlobalDecl(dtor, Dtor_Complete));

test/CodeGenCXX/dllimport-base.cpp

+// RUN: %clang_cc1 -triple i386-pc-windows -emit-llvm -fms-compatibility %s -x c++ -o - | FileCheck %s
+
+// CHECK: declare dllimport x86_thiscallcc void @"\01??1VirtualClass@test1@@UAE@XZ"
+// CHECK: declare dllimport x86_thiscallcc void @"\01??1VirtualClass@test2@@UAE@XZ"
+// CHECK:  declare dllimport x86_thiscallcc void @"\01??1VirtualClass@test3@@UAE@XZ"
+// CHECK-NOT: ??_DVirtualClass@@QEAAXXZ
+
+namespace test1 {
+struct __declspec(dllimport) VirtualClass {
+public:
+  virtual ~VirtualClass(){};
+};
+
+int main() {
+  VirtualClass c;
+  return 0;
+}
+} // namespace test1
+
+namespace test2 {
+class BaseClass {
+public:
+  virtual ~BaseClass(){};
+};
+
+class __declspec(dllimport) VirtualClass : public BaseClass {
+public:
+  virtual ~VirtualClass(){};
+};
+
+int main() {
+  VirtualClass c;
+  return 0;
+}
+} // namespace test2
+
+namespace test3 {
+class BaseClass {
+public:
+  virtual ~BaseClass(){};
+};
+
+class __declspec(dllimport) VirtualClass : public virtual BaseClass {
+public:
+  virtual ~VirtualClass(){};
+};
+
+int main() {
+  VirtualClass c;
+  return 0;
+}
+} // namespace test3