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

[zahiraam]

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.

[erichkeane]

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]

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]

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]

@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]

@majnemer Any feedback? Thanks.

[hans]

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]

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

[hans]

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]

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.

[zahiraam]

[hans]

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

[zahiraam]

In D79591#2092223, @hans wrote:

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]

In D79591#2095508, @zahiraam wrote:

In D79591#2092223, @hans wrote:

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]

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);

[riccibruno]

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]

@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]

In D79591#2096642, @zahiraam wrote:

@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]

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))

[zahiraam]

Review anyone please? Thanks.

[hans]

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

clang/lib/Sema/SemaDeclCXX.cpp

if (OldParamHasDfl && NewParamHasDfl) {
         << OldParam->getDefaultArgRange();
     } else if (OldParamHasDfl) {
       // Merge the old default argument into the new parameter unless the new
       // function is a friend declaration in a template class. In the latter
       // case the default arguments will be inherited when the friend
       // declaration will be instantiated.
       if (New->getFriendObjectKind() == Decl::FOK_None ||
           !New->getLexicalDeclContext()->isDependentContext()) {
         // It's important to use getInit() here;  getDefaultArg()
         // strips off any top-level ExprWithCleanups.
         NewParam->setHasInheritedDefaultArg();
         if (OldParam->hasUnparsedDefaultArg())
           NewParam->setUnparsedDefaultArg();
         else if (OldParam->hasUninstantiatedDefaultArg())
           NewParam->setUninstantiatedDefaultArg(
                                        OldParam->getUninstantiatedDefaultArg());
         else
           NewParam->setDefaultArg(OldParam->getInit());
       }
     } else if (NewParamHasDfl) {
       if (New->getDescribedFunctionTemplate()) {
         // Paragraph 4, quoted above, only applies to non-template functions.
         Diag(NewParam->getLocation(),
              diag::err_param_default_argument_template_redecl)
           << NewParam->getDefaultArgRange();
         Diag(PrevForDefaultArgs->getLocation(),
              diag::note_template_prev_declaration)
for (Decl *Member : Class->decls()) {
     if (!MD)
       continue;
 
     if (Member->getAttr<DLLExportAttr>()) {
       if (MD->isUserProvided()) {
         // Instantiate non-default class member functions ...
 
         // .. except for certain kinds of template specializations.
-        if (TSK == TSK_ImplicitInstantiation && !ClassAttr->isInherited())
+        if (TSK == TSK_ImplicitInstantiation && !ClassAttr->isInherited()) {
+          // But before that, make sure to instantiate default args for
+          // defaulted constructor.
+          CXXConstructorDecl *Ctor = dyn_cast<CXXConstructorDecl>(Member);
+          if (Ctor && Ctor->isDefaultConstructor() &&
+              Ctor->hasOneParamOrDefaultArgs()) {
+            for (unsigned I = 0; I != Ctor->getNumParams(); ++I) {
+              S.CheckCXXDefaultArgExpr(
+                  Member->getAttr<DLLExportAttr>()->getLocation(), Ctor,
+                  Ctor->getParamDecl(I));
+              S.DiscardCleanupsInEvaluationContext();
+            }
+          }
           continue;
-
+        }
         S.MarkFunctionReferenced(Class->getLocation(), MD);
 
         // The function will be passed to the consumer when its definition is
         // encountered.
       } else if (!MD->isTrivial() || MD->isExplicitlyDefaulted() ||
                  MD->isCopyAssignmentOperator() ||
                  MD->isMoveAssignmentOperator()) {
         // Synthesize and instantiate non-trivial implicit methods, explicitly

clang/test/SemaCXX/default-arg-closures.cpp

 
 struct Default {
   Default(const Default &o, int d = (HasCleanup(), 42));
 };
 
 void f(const Default &d) {
   throw d;
 }
+
+template <typename>
+class __declspec(dllexport) foo {
+  foo(int x = 0);
+};
+template <>
+foo<int>::foo(int);