Page MenuHomePhabricator

Default arguments of an exported default constructor should be instantiated. Fix for Bug45811 - Failed assertion.
Needs ReviewPublic

Authored by zahiraam on May 7 2020, 10:47 AM.

Details

Summary

Compiler is crashing when instantiated default argument marked as not.

When a default constructor is dllexport-ed, a closure is generated. A closure is a first-class function along with its surrounding state. It is able (unlike lambda function)
to interact with the outside environment, i.e refers to variables in the outer scope. For the closure to be in a "close" state, it needs its own local parameters.
This patch adds instantiated parameters to the closure.

Diff Detail

Event Timeline

zahiraam created this revision.May 7 2020, 10:47 AM
erichkeane added inline comments.May 18 2020, 8:46 AM
clang/lib/Sema/SemaDeclCXX.cpp
571

What is the MS extension that makes this necessary? Why does the normal inherited default arg not do anything here?

Can you better explain in the commit message what you think is wrong here? Why would this happen only with dll_export, and not the other mechanisms for making 'foo' be emitted?

zahiraam updated this revision to Diff 264701.May 18 2020, 12:38 PM
zahiraam marked an inline comment as done.
zahiraam added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
571

@erichkeane Thanks for looking at it. Updated the commit message.
The default closure constructor is always in presence of a dllexport, I think.

erichkeane added inline comments.May 18 2020, 1:15 PM
clang/lib/Sema/SemaDeclCXX.cpp
575

Can MD BE a method-decl and have a parent that isn't a record? That seems the second condition there is ALWAYS true.

580

These seem overly constraining. It also seems like you'd want to check the CXXCtorType of the constructor, to make sure you're dealing with the closure, right?

Can this problem happen in an implicit specialization?

Added @majnemer since he implemented constructor closures in D8331 it looks, I don't think I know enough about them to review.

rnk added a subscriber: hans.May 19 2020, 11:30 AM

@hans is the resident dllexport expert at this moment.

My really surface-level feedback would be to try to find a way to do this with as few conditions as humanly possible. My mental model is that every added boolean check charges interest, and there are quite a few here, and it's hard for me to understand why they are all needed.

Starting this June, I won't be available for reviews until fall, as a heads up.

zahiraam updated this revision to Diff 265353.May 20 2020, 2:55 PM
zahiraam added a reviewer: hans.
zahiraam updated this revision to Diff 265856.May 23 2020, 6:52 AM

@majnemer Any feedback? Thanks.

hans added a comment.Jun 10 2020, 5:26 AM

I'm also not sure I understand what's going on here. Can you try to explain the patch a bit more?

clang/lib/Sema/SemaDeclCXX.cpp
572

From the code, it's not clear why you're checking MicrosoftExt here. The comment mentions dllexport.. is that what you should be checking for instead?

575

nit: indentation looks off here. Also, local variables like 'spec' and 'e' should start with a capital letter.

581

Could you just do OldParam->setDefaultArg(OldParam->getInit()) ?

But I don't really understand what's going on here. Is it okay to be modifying OldParam here? Isn't it NewParam that should be modified?

zahiraam updated this revision to Diff 269917.Jun 10 2020, 11:14 AM

@hans Thanks for looking at it. I think the updated patch is a better way to handle the crash.

hans added inline comments.Jun 10 2020, 11:48 AM
clang/lib/AST/DeclBase.cpp
346 ↗(On Diff #269917)

(Indentation is off here.)

But mainly, I think you need to explain this. Why does a class having a default constructor and being dllexport imply that any decl in that class has local scope?

zahiraam updated this revision to Diff 269955.Jun 10 2020, 12:59 PM
zahiraam marked an inline comment as done.
zahiraam added inline comments.
clang/lib/AST/DeclBase.cpp
346 ↗(On Diff #269917)

When the default constructor is exported, then a thunk function is created and becomes the default constructor closure.
The default arguments of the constructor are added to the list of local arguments of the newly created thunk.

hans added a comment.Jun 15 2020, 1:02 AM

I'm sorry, but I still don't understand, and neither will anyone who reads the new code, I think. It needs an explanation.

I'm sorry, but I still don't understand, and neither will anyone who reads the new code, I think. It needs an explanation.

@hans thanks for the feedback.

Let's see. It is difficult to explain, but let me try.
A constructor that is dllexport-ed is a closure. A closure is a first-class function along with its surrounding state. It is able (unlike lambda function)
to interact with the outside environment, i.e refers to variables in the outer scope. The closure of an "open" thunk (the default constructor) needs its own
local variables to put it in a "closing" state.

hans added a comment.Jun 16 2020, 6:14 AM

I'm sorry, but I still don't understand, and neither will anyone who reads the new code, I think. It needs an explanation.

@hans thanks for the feedback.

Let's see. It is difficult to explain, but let me try.
A constructor that is dllexport-ed is a closure. A closure is a first-class function along with its surrounding state. It is able (unlike lambda function)
to interact with the outside environment, i.e refers to variables in the outer scope. The closure of an "open" thunk (the default constructor) needs its own
local variables to put it in a "closing" state.

Maybe you could provide an example to make it clearer? In particular I'm wondering what decls need to be considered local (and what that means).

My concern is that the change to Decl::isInLocalScope() seems very broad. It seems that you're making it return true for all Decls that are inside a dllexported class with default constructor, and I don't understand if that's the intention and why.

hans added a comment.Jun 16 2020, 6:19 AM

For example, I think in the current version of the patch, S::x would be considered local, and I'm wondering if that's intentional and why:

struct __declspec(dllexport) S {
  S() {}
  int x;
};

And also, the code does not seem like it would do anything in this case, where the class is not dllexported but the constructor is:

template <typename> struct S {
  S(int x = 0) __declspec(dllexport);
};
template <> S<int>::S(int);

In general I think that both the title and the description could use some attention. For example:
Title: Fix for Bugxxx - Failed assertion => [clang][more specific tag if appropriate] Don't do bar when frob

Description: Compiler is crashing when instantiated default argument marked as not. =>

A short paragraph explaining the current situation and *why* there is a problem.

A short paragraph explaining the fix and *why* this is the right solution, and possibly
why some other solution would not be better.

Fixes PRxxxx

You have to sell the patch and make the reviewer think: "Oh this is absolutely the right fix and I want my name on this code!"

zahiraam edited the summary of this revision. (Show Details)Jun 16 2020, 1:20 PM

@hans Thanks for the feedback.
@riccibruno Thanks for the feedback.

@hans for the first test case, the code doesn't enter the function substParmVarDecl, neither does it enter getAddrOfCXXCtorClosure.
This latter fnction call is guarded by the number of parameters of the Record (S). In this case S doen't have any parameters, so it
skips the creation of the thunk. The IRs of this test case with this and without this patch are the same. And the symbols generated
are also the same (and pretty much the same than MSVC).

For the second one, the function isInLocalScope returns false because the CXXRecordDecl (S) has a default constructor but is
not exported, so the params remain unchanged and it doesn't enter the getAddrOfCXXCtorClosure because S is not exported.
So no closure is created. With and without this patch this test case has same control flow path and same beahavior. And there
is no generation of the ctor closure.
It is a bug (last paragraph), but I think unrelated to the crash we are trying to fix here?

bash-3.2$ cat hans2.cpp
template <typename> struct S {

__declspec(dllexport) S(int x = 0);
};

template <> S<int>::S(int);

bash-3.2$ cl -c hans2.cpp
bash-3.2$ dumpbin /symbols hans2.obj | grep "decl"
008 00000000 UNDEF notype () External | ??0?$S@H@@QEAA@H@Z (public: cdecl S<int>::S<int>(int))
009 00000000 SECT3 notype () External | ??_F?$S@H@@QEAAXXZ (public: void
cdecl S<int>::`default constructor closure'(void))
bash-3.2$ clang -c hans2.cpp
bash-3.2$ dumpbin /symbols hans2.o | grep "decl"
bash-3.2$

Adding two other variants of the test case in the patch. Both of these test cases pass by the function EmitCXXConstructors
but not inside getAddrOfCXXCtorClosure because they are not isDefaultConstructor().
test4.cpp returns false for isInLocalScope because hasDefaultConstructor returns false.
test5.cpp doesn't pass by isInLocalScope.

They can potentially be added to the patch, if you think they are worth adding.

bash-3.2$ cat test4.cpp
template <typename>
class __declspec(dllexport) foo {

foo(int a, int x = 0);

};
template <>
foo<int>::foo(int, int);

bash-3.2$ cl -c test4.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27035 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

test4.cpp
bash-3.2$ clang -c test4.cpp
bash-3.2$ dumpbin /symbols test4.obj | grep foo
00A 00000000 SECT4 notype () External | ??4?$foo@H@@QEAAAEAV0@AEBV0@@Z (public: class foo<int> & cdecl foo<int>::operator=(class foo<int> const &))
00B 00000000 SECT3 notype () External | ??4?$foo@H@@QEAAAEAV0@$$QEAV0@@Z (public: class foo<int> &
cdecl foo<int>::operator=(class foo<int> &&))
bash-3.2$ dumpbin /symbols test4.o | grep foo
008 00000000 SECT4 notype () External | ??4?$foo@H@@QEAAAEAV0@AEBV0@@Z (public: class foo<int> & cdecl foo<int>::operator=(class foo<int> const &))
00D 00000000 SECT5 notype () External | ??4?$foo@H@@QEAAAEAV0@$$QEAV0@@Z (public: class foo<int> &
cdecl foo<int>::operator=(class foo<int> &&))
bash-3.2$

bash-3.2$ cat test5.cpp
template <typename>
class __declspec(dllexport) foo {

foo(int a);

};
template <>
foo<int>::foo(int);

bash-3.2$ cl -c test5.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27035 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

test5.cpp
bash-3.2$ clang -c test5.cpp
bash-3.2$ dumpbin /symbols test5.obj | grep foo
00A 00000000 SECT4 notype () External | ??4?$foo@H@@QEAAAEAV0@AEBV0@@Z (public: class foo<int> & cdecl foo<int>::operator=(class foo<int> const &))
00B 00000000 SECT3 notype () External | ??4?$foo@H@@QEAAAEAV0@$$QEAV0@@Z (public: class foo<int> &
cdecl foo<int>::operator=(class foo<int> &&))
bash-3.2$ dumpbin /symbols test5.o | grep foo
008 00000000 SECT4 notype () External | ??4?$foo@H@@QEAAAEAV0@AEBV0@@Z (public: class foo<int> & cdecl foo<int>::operator=(class foo<int> const &))
00D 00000000 SECT5 notype () External | ??4?$foo@H@@QEAAAEAV0@$$QEAV0@@Z (public: class foo<int> &
cdecl foo<int>::operator=(class foo<int> &&))
bash-3.2$

Now the other thing that I was going to mention: while looking at this issue I have found some cases
that are not generating the ctor closure, while MSVC is. That I think is a bug. This patch is trying
to fix the crash only (although it is closely related to the bug). But there is definitely need for more work
to understand why the cases below don't generate the closure.

bash-3.2$ cat test1.cpp
// Constructor closure is not generated.
class __declspec(dllexport) foo {

foo(int x = 0);

};

foo f();

bash-3.2$
bash-3.2$ cl -c test1.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

test1.cpp
bash-3.2$ clang -c test1.cpp
bash-3.2$ dumpbin /symbols test1.obj | grep foo
00C 00000000 UNDEF notype () External | ??0foo@@AEAA@H@Z (private: cdecl foo::foo(int))
00D 00000000 SECT4 notype () External | ??4foo@@QEAAAEAV0@AEBV0@@Z (public: class foo &
cdecl foo::operator=(class foo const &))
00E 00000000 SECT3 notype () External | ??4foo@@QEAAAEAV0@$$QEAV0@@Z (public: class foo & cdecl foo::operator=(class foo &&))
00F 00000000 SECT5 notype () External | ??_Ffoo@@QEAAXXZ (public: void
cdecl foo::`default constructor closure'(void))
013 00000000 SECT6 notype Static | $unwind$??_Ffoo@@QEAAXXZ
016 00000000 SECT7 notype Static | $pdata$??_Ffoo@@QEAAXXZ
bash-3.2$ dumpbin /symbols test1.o | grep foo
008 00000000 SECT4 notype () External | ??4foo@@QEAAAEAV0@AEBV0@@Z (public: class foo & cdecl foo::operator=(class foo const &))
00D 00000000 SECT5 notype () External | ??4foo@@QEAAAEAV0@$$QEAV0@@Z (public: class foo &
cdecl foo::operator=(class foo &&))
bash-3.2$

bash-3.2$ cat test3.cpp
// Constructor closure is not generated.
template <typename T>
class __declspec(dllexport) foo {
public:

foo(T o = 0);

};

foo<int> S (3);
bash-3.2$
bash-3.2$ cl -c test3.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

test3.cpp
bash-3.2$ clang -c test3.cpp
bash-3.2$ dumpbin /symbols test3.obj | grep foo
00E 00000000 UNDEF notype () External | ??0?$foo@H@@QEAA@H@Z (public: cdecl foo<int>::foo<int>(int))
00F 00000000 SECT4 notype () External | ??4?$foo@H@@QEAAAEAV0@AEBV0@@Z (public: class foo<int> &
cdecl foo<int>::operator=(class foo<int> const &))
010 00000000 SECT3 notype () External | ??4?$foo@H@@QEAAAEAV0@$$QEAV0@@Z (public: class foo<int> & cdecl foo<int>::operator=(class foo<int> &&))
011 00000000 SECT5 notype () External | ??_F?$foo@H@@QEAAXXZ (public: void
cdecl foo<int>::`default constructor closure'(void))
016 00000000 SECT7 notype Static | $unwind$??_F?$foo@H@@QEAAXXZ
019 00000000 SECT8 notype Static | $pdata$??_F?$foo@H@@QEAAXXZ
022 00000000 SECTB notype External | ?S@@3V?$foo@H@@A (class foo<int> S)
bash-3.2$ dumpbin /symbols test3.o | grep foo
008 00000000 SECT4 notype () External | ??4?$foo@H@@QEAAAEAV0@AEBV0@@Z (public: class foo<int> & cdecl foo<int>::operator=(class foo<int> const &))
00D 00000000 SECT5 notype () External | ??4?$foo@H@@QEAAAEAV0@$$QEAV0@@Z (public: class foo<int> &
cdecl foo<int>::operator=(class foo<int> &&))
020 00000000 SECT3 notype External | ?S@@3V?$foo@H@@A (class foo<int> S)
021 00000000 UNDEF notype External | ??0?$foo@H@@QEAA@H@Z (public: __cdecl foo<int>::foo<int>(int))
bash-3.2$

hans added a comment.Jun 18 2020, 4:41 AM

@hans for the first test case, the code doesn't enter the function substParmVarDecl, neither does it enter getAddrOfCXXCtorClosure.
This latter fnction call is guarded by the number of parameters of the Record (S). In this case S doen't have any parameters, so it
skips the creation of the thunk. The IRs of this test case with this and without this patch are the same. And the symbols generated
are also the same (and pretty much the same than MSVC).

Okay, but even if those functions are not called, it seems to me that your change makes Decl::isInLocalScope() consider S::x a local variable which seems confusing.

For the second one, the function isInLocalScope returns false because the CXXRecordDecl (S) has a default constructor but is
not exported, so the params remain unchanged and it doesn't enter the getAddrOfCXXCtorClosure because S is not exported.
So no closure is created. With and without this patch this test case has same control flow path and same beahavior. And there
is no generation of the ctor closure.
It is a bug (last paragraph), but I think unrelated to the crash we are trying to fix here?

It seems related because the case is very similar except it's just the constructor that's exported and not the whole class.

Now the other thing that I was going to mention: while looking at this issue I have found some cases
that are not generating the ctor closure, while MSVC is. That I think is a bug. This patch is trying
to fix the crash only (although it is closely related to the bug). But there is definitely need for more work
to understand why the cases below don't generate the closure.

bash-3.2$ cat test1.cpp
// Constructor closure is not generated.
class __declspec(dllexport) foo {

foo(int x = 0);

};

foo f();

This looks like a pretty basic test. Maybe starting here is a good way of finding out how Clang's constructor closure emission should be fixed?

zahiraam updated this revision to Diff 281978.Jul 30 2020, 10:42 AM
zahiraam retitled this revision from Fix for Bug45811 - Failed assertion to Default arguments of an exported default constructor should be instantiated. Fix for Bug45811 - Failed assertion..

This patch is only to fix the compiler crash.
Would you mind commenting/reviewing this patch as it is a crash with (internal) impact?
There would still be another bug that needs fixing (would prefer to open another bug for it), for this case:
class __declspec(dllexport) foo {

foo(int x = 0);

};

template <>
foo<int>::foo(int);

Where the default constructor closure symbol is not generated.
MSVC generates one:
00F 00000000 SECT5 notype () External | ??_Ffoo@@QEAAXXZ (public: void __cdecl foo::`default constructor closure'(void))

Review anyone please? Thanks.

hans added a comment.Nov 9 2020, 10:34 AM

I was looking into this today, and sent out what I think is a better approach: https://reviews.llvm.org/D91089