Differential D79591
Default arguments of an exported default constructor should be instantiated. Fix for Bug45811 - Failed assertion. Authored by zahiraam on May 7 2020, 10:47 AM.
Details 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)
Diff Detail Event Timeline
Comment Actions @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. Comment Actions I'm also not sure I understand what's going on here. Can you try to explain the patch a bit more?
Comment Actions @hans Thanks for looking at it. I think the updated patch is a better way to handle the crash.
Comment Actions I'm sorry, but I still don't understand, and neither will anyone who reads the new code, I think. It needs an explanation. Comment Actions @hans thanks for the feedback. Let's see. It is difficult to explain, but let me try. Comment Actions 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. Comment Actions 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);Comment Actions In general I think that both the title and the description could use some attention. For example: 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!" Comment Actions @hans Thanks for the feedback. @hans for the first test case, the code doesn't enter the function substParmVarDecl, neither does it enter getAddrOfCXXCtorClosure. For the second one, the function isInLocalScope returns false because the CXXRecordDecl (S) has a default constructor but is bash-3.2$ cat hans2.cpp __declspec(dllexport) S(int x = 0); }; template <> S<int>::S(int); bash-3.2$ cl -c hans2.cpp Adding two other variants of the test case in the patch. Both of these test cases pass by the function EmitCXXConstructors They can potentially be added to the patch, if you think they are worth adding. bash-3.2$ cat test4.cpp foo(int a, int x = 0); }; bash-3.2$ cl -c test4.cpp test4.cpp bash-3.2$ cat test5.cpp foo(int a); }; bash-3.2$ cl -c test5.cpp test5.cpp Now the other thing that I was going to mention: while looking at this issue I have found some cases bash-3.2$ cat test1.cpp foo(int x = 0); }; foo f(); bash-3.2$ test1.cpp bash-3.2$ cat test3.cpp foo(T o = 0); }; foo<int> S (3); test3.cpp Comment Actions 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.
It seems related because the case is very similar except it's just the constructor that's exported and not the whole class.
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? Comment Actions This patch is only to fix the compiler crash. foo(int x = 0); }; template <> Where the default constructor closure symbol is not generated. Comment Actions I was looking into this today, and sent out what I think is a better approach: https://reviews.llvm.org/D91089 | |||||||||||||||||||||||||||||||||||||||||||||
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?