Page MenuHomePhabricator

Suppress generation of a deleting destructor for a dllimport record.
Needs ReviewPublic

Authored by zahiraam on Oct 11 2017, 8:04 AM.

Details

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.

Diff Detail

Event Timeline

zahiraam created this revision.Oct 11 2017, 8:04 AM
rnk added a subscriber: hiraditya.Oct 12 2017, 3:39 PM
rnk added inline comments.
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 added inline comments.Oct 12 2017, 5:57 PM
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 added inline comments.Oct 16 2017, 7:09 AM
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$

@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 updated this revision to Diff 134624.Feb 16 2018, 8:03 AM
zahiraam marked 3 inline comments as done.
rnk added inline comments.Feb 16 2018, 11:59 AM
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 updated this revision to Diff 134806.Feb 17 2018, 10:04 AM
zahiraam marked 2 inline comments as done.

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 added a comment.Mar 7 2018, 5:38 PM

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.

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?

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.

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#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 updated this revision to Diff 137630.Mar 8 2018, 11:58 AM

Test case to add.

zahiraam marked an inline comment as done and an inline comment as not done.Mar 8 2018, 11:59 AM

Attached the test case.