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 ↗

(On Diff #118622)

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 ↗

(On Diff #118622)

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

lib/CodeGen/MicrosoftCXXABI.cpp

3806 ↗

(On Diff #118622)

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

[zahiraam]

lib/CodeGen/CGCXX.cpp

142 ↗

(On Diff #118622)

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

3829 ↗

(On Diff #134624)

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

3829 ↗

(On Diff #134624)

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

3829 ↗

(On Diff #134624)

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.