This is an archive of the discontinued LLVM Phabricator instance.

[dllexport] Instantiate default ctor default args for explicit specializations (PR45811)
ClosedPublic

Authored by hans on Nov 9 2020, 10:28 AM.

Details

Summary

For dllexported default constructors with default arguments, we export default constructor closures which pass in the default args. (See D8331 for a good explanation.)

For templates, that means those default args must be instantiated even if the function isn't called. That is done by the InstantiateDefaultCtorDefaultArgs() function, but it wasn't done for explicit specializations, causing asserts (see bug).

Diff Detail

Event Timeline

hans created this revision.Nov 9 2020, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2020, 10:28 AM
hans requested review of this revision.Nov 9 2020, 10:28 AM
hans added a comment.Nov 9 2020, 10:33 AM

Please take a look.

clang/lib/Sema/SemaDecl.cpp
13996

I was searching for the right place to do this, looking mostly in the sema template code, but ended up here as an explicit specialization really works out a lot like a function definition.

clang/test/CodeGenCXX/dllexport-ctor-closure.cpp
53

This should also work when dllexport is on the member function rather than the class, but it turns out we currently don't export the function in that case; that's a bug I want to handle separately.

Please take a look.

@hans Thanks for looking at this.
This test case is still crashing with the patch:

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

foo(int x = 0);

};
template <>
foo<int>::foo(int);
ksh-3.2$

clang.exe -c test.cpp
Assertion failed: !hasUninstantiatedDefaultArg() && "Default argument is not yet instantiated!", file D:\IUSERS\zahiraam\llorg_ws\ws1\llvm\clang\lib\AST\Decl.cpp, line 2719
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.

hans added a comment.Nov 10 2020, 7:51 AM

clang.exe -c test.cpp
Assertion failed: !hasUninstantiatedDefaultArg() && "Default argument is not yet instantiated!", file D:\IUSERS\zahiraam\llorg_ws\ws1\llvm\clang\lib\AST\Decl.cpp, line 2719
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.

Thanks for catching that!

What's happening is that Clang tries to emit the ctor closure even though the ctor is just declared, but not defined.

I don't think it makes sense to try to emit the closure until we have the ctor definition. I'll update the patch to handle this.

hans updated this revision to Diff 304190.Nov 10 2020, 7:53 AM

Handling the decl only case.

clang.exe -c test.cpp
Assertion failed: !hasUninstantiatedDefaultArg() && "Default argument is not yet instantiated!", file D:\IUSERS\zahiraam\llorg_ws\ws1\llvm\clang\lib\AST\Decl.cpp, line 2719
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.

Thanks for catching that!

What's happening is that Clang tries to emit the ctor closure even though the ctor is just declared, but not defined.

I don't think it makes sense to try to emit the closure until we have the ctor definition. I'll update the patch to handle this.

Ok. That's a bit different than what MSVC is doing. It generates a closure constructor even if there is only a declaration. But I guess we are not aligning specifically with MSVC!
Look good.

zahiraam accepted this revision.Nov 10 2020, 8:54 AM
This revision is now accepted and ready to land.Nov 10 2020, 8:54 AM
hans added a comment.Nov 12 2020, 4:28 AM

I don't think it makes sense to try to emit the closure until we have the ctor definition. I'll update the patch to handle this.

Ok. That's a bit different than what MSVC is doing. It generates a closure constructor even if there is only a declaration. But I guess we are not aligning specifically with MSVC!
Look good.

Right, I noticed that MSVC does this, but I couldn't come up with any reason why it would be necessary for us. Emitting the closure with the ctor definition should be sufficient.