[Clang] Export inline move constructors in dllexport-ed template instantiations on non-MSVC targets

[kikairoya]

Previously, even when MSVC compatibility was not requested, inline move constructors in dllexport-ed templates were not exported, which was seemingly unintended.
On non-MSVC targets (MinGW, Cygwin, and PS), such move constructors should be exported consistently with copy constructors and with the behavior of modern MSVC.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Tomohiro Kashiwada (kikairoya)

Changes

Previously, even when MSVC compatibility was not requested, inline move constructors in dllexport-ed templates were not exported, which was seemingly unintended.
On non-MSVC targets (MinGW, Cygwin, and PS), such move constructors should be exported consistently with copy constructors and with the behavior of modern MSVC.

---

Full diff: https://github.com/llvm/llvm-project/pull/168170.diff

3 Files Affected:

• (modified) clang/lib/Sema/SemaDeclCXX.cpp (+1)
• (modified) clang/test/CodeGenCXX/dllexport.cpp (+1)
• (modified) clang/test/CodeGenCXX/mingw-template-dllexport.cpp (+2)

diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index aa36a79142e52..5f741166d0e29 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -6627,6 +6627,7 @@ void Sema::checkClassLevelDLLAttribute(CXXRecordDecl *Class) {
         auto *Ctor = dyn_cast<CXXConstructorDecl>(MD);
         if ((MD->isMoveAssignmentOperator() ||
              (Ctor && Ctor->isMoveConstructor())) &&
+            getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2010) &&
             !getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015))
           continue;
 
diff --git a/clang/test/CodeGenCXX/dllexport.cpp b/clang/test/CodeGenCXX/dllexport.cpp
index ef9d8131c511c..a34a87c5b9f21 100644
--- a/clang/test/CodeGenCXX/dllexport.cpp
+++ b/clang/test/CodeGenCXX/dllexport.cpp
@@ -1131,5 +1131,6 @@ class __declspec(dllexport) ACE_Shared_Object {
 class __declspec(dllexport) ACE_Service_Object : public ACE_Shared_Object {};
 // Implicit move constructor declaration.
 // MSVC2015-DAG: define weak_odr dso_local dllexport {{.+}}ACE_Service_Object@@Q{{.+}}@$$Q
+// PS-DAG: define weak_odr dllexport void @_ZN18ACE_Service_ObjectC1EOS_
 // The declarations should not be exported.
 // MSVC2013-NOT: define weak_odr dso_local dllexport {{.+}}ACE_Service_Object@@Q{{.+}}@$$Q
diff --git a/clang/test/CodeGenCXX/mingw-template-dllexport.cpp b/clang/test/CodeGenCXX/mingw-template-dllexport.cpp
index de112d6da53db..15969d2415cca 100644
--- a/clang/test/CodeGenCXX/mingw-template-dllexport.cpp
+++ b/clang/test/CodeGenCXX/mingw-template-dllexport.cpp
@@ -10,11 +10,13 @@
 
 template <class T>
 class c {
+  c(c &&) noexcept {}
   void f() {}
 };
 
 template class __declspec(dllexport) c<int>;
 
+// CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIiEC1EOS0_
 // CHECK: define {{.*}} dllexport {{.*}} @_ZN1cIiE1fEv
 
 extern template class __declspec(dllexport) c<char>;

[kikairoya]

cc: @mstorsjo @jmorse

[jmorse]

@bd1976bris @playstation-edd this feels harmless to me (exporting inline move-constructors and move-operators where we already do it for normal copy ones?) , however I'm no expert on dllexport and friends. Opinions please!

[bd1976bris]

Apologies for the slow reply here. I'll try to take a look tomorrow (GMT).

[bd1976bris]

This tripped me up when I first looked at it. I think it would be nice to introduce a no argument form bool isCompatibleWithMSVC() const; and then write the MSVC compatibility bit as: getLangOpts().isCompatibleWithMSVC() && !getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015)

[bd1976bris]

Thanks for looking at this. I agree the current behaviour looks unintended.

I left a suggestion for improving the readability of the change. It would also be good to see dllimport test cases added.

Functionally this change looks good to me and I don't think that it poses an ABI problem for PlayStation except perhaps for internal codebases. Any such cases should be discovered via our release testing and we can deal with those as they arise.

I can't see any good reason why this change wouldn't be acceptable for other affected targets e.g. MinGW.

I suggest we get this in as quickly as possible to give affected targets as long as possible to exercise this change before the next release.

[kikairoya]

Thank you for the reviewing.
I've added a new function isCompatibleWithMSVC and tests for dllimport.

[bd1976bris]

Thanks, LGTM. Maybe wait until Friday to merge this to give a bit more time for @mstorsjo to comment.

[mstorsjo]

Thanks, LGTM. Maybe wait until Friday to merge this to give a bit more time for @mstorsjo to comment.

No objection from me to this; I can't say I can confidently review this as such, but it sounds reasonable.

[kikairoya]

If it's OK to merge this, could you please merge this for me? I don't have the permission.

[jmorse]

With @bd1976bris having approved, without objection I'll click merge today or tomorrow.