Re: [PATCH] D19156: [ms][dll] #27212: Generating of implicit special members should take into account MSVC compatibility version
rnk added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:4813 @@ +4812,3 @@ +// and move constructor, so don't attempt to import/export them if +// we have a definition. +auto *CXXC = dyn_cast(MD); Oh, so we were already doing this check. I don't see what's wrong with our current behavior, though. We export a few more symbols than MSVC 2013, but there's no ABI problem with that. http://reviews.llvm.org/D19156 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19156: [ms][dll] #27212: Generating of implicit special members should take into account MSVC compatibility version
rnk added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:4816 @@ -4815,1 +4815,3 @@ +if ((MD->isMoveAssignmentOperator() || + (CXXC && CXXC->isMoveConstructor())) && !getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015)) The move constructor part of this is definitely a good fix though. http://reviews.llvm.org/D19156 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19156: [ms][dll] #27212: Generating of implicit special members should take into account MSVC compatibility version
avt77 marked 2 inline comments as done. Comment at: lib/Sema/SemaDeclCXX.cpp:4813 @@ +4812,3 @@ +// and move constructor, so don't attempt to import/export them if +// we have a definition. +auto *CXXC = dyn_cast(MD); rnk wrote: > Oh, so we were already doing this check. I don't see what's wrong with our > current behavior, though. We export a few more symbols than MSVC 2013, but > there's no ABI problem with that. Yes, it's a question about binary compatibility only Comment at: lib/Sema/SemaDeclCXX.cpp:4816 @@ -4815,1 +4815,3 @@ +if ((MD->isMoveAssignmentOperator() || + (CXXC && CXXC->isMoveConstructor())) && !getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015)) rnk wrote: > The move constructor part of this is definitely a good fix though. And what's the decision? Could I commit the patch? http://reviews.llvm.org/D19156 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19156: [ms][dll] #27212: Generating of implicit special members should take into account MSVC compatibility version
rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm I've been trying to say that ABI compatibility is not the same thing as generating the same set of exported symbols, but it really doesn't matter. If MSVC 2013 and 2015 are not ABI compatible in this corner case, there is no need for clang to bend over backward to provide a marginally better experience. It sounds like it makes your life easier if we can always produce the exact same set of exports as the MSVC version we are targeting. http://reviews.llvm.org/D19156 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19156: [ms][dll] #27212: Generating of implicit special members should take into account MSVC compatibility version
DmitryPolukhin closed this revision. DmitryPolukhin added a comment. Committed as http://reviews.llvm.org/rL269400 http://reviews.llvm.org/D19156 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19156: [ms][dll] #27212: Generating of implicit special members should take into account MSVC compatibility version
rnk requested changes to this revision. rnk added a comment. This revision now requires changes to proceed. As mentioned twice in https://llvm.org/bugs/show_bug.cgi?id=27212, I don't think this is the right direction. To my knowledge, this only causes an ABI break when importing a class. I think we should change Sema::checkClassLevelDLLAttribute to not apply dllimport to implicit move special members instead. http://reviews.llvm.org/D19156 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19156: [ms][dll] #27212: Generating of implicit special members should take into account MSVC compatibility version
avt77 added a comment. It seems it will be even shorter if we do it via Sema::checkClassLevelDLLAttribute. Tnx. http://reviews.llvm.org/D19156 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19156: [ms][dll] #27212: Generating of implicit special members should take into account MSVC compatibility version
avt77 updated this revision to Diff 55567.
avt77 added a comment.
Now it's really a micro-patch: please review.
http://reviews.llvm.org/D19156
Files:
lib/Sema/SemaDeclCXX.cpp
test/CodeGenCXX/dllexport.cpp
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -4774,7 +4774,6 @@
// The class is either imported or exported.
const bool ClassExported = ClassAttr->getKind() == attr::DLLExport;
- const bool ClassImported = !ClassExported;
TemplateSpecializationKind TSK = Class->getTemplateSpecializationKind();
@@ -4809,9 +4808,12 @@
if (!Context.getTargetInfo().getCXXABI().isMicrosoft())
continue;
-// MSVC versions before 2015 don't export the move assignment
operators,
-// so don't attempt to import them if we have a definition.
-if (ClassImported && MD->isMoveAssignmentOperator() &&
+// MSVC versions before 2015 don't export the move assignment operators
+// and move constructor, so don't attempt to import/export them if
+// we have a definition.
+auto *CXXC = dyn_cast(MD);
+if ((MD->isMoveAssignmentOperator() ||
+ (CXXC && CXXC->isMoveConstructor())) &&
!getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015))
continue;
}
Index: test/CodeGenCXX/dllexport.cpp
===
--- test/CodeGenCXX/dllexport.cpp
+++ test/CodeGenCXX/dllexport.cpp
@@ -1,5 +1,9 @@
-// RUN: %clang_cc1 -triple i686-windows-msvc -emit-llvm -std=c++1y
-fno-threadsafe-statics -fms-extensions -O1 -mconstructor-aliases
-disable-llvm-optzns -o - %s -w | FileCheck --check-prefix=MSC
--check-prefix=M32 %s
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -std=c++1y
-fno-threadsafe-statics -fms-extensions -O0 -o - %s -w | FileCheck
--check-prefix=MSC --check-prefix=M64 %s
+// RUN: %clang_cc1 -triple i686-windows-msvc -emit-llvm -std=c++1y
-fno-threadsafe-statics -fms-extensions -O1 -mconstructor-aliases
-disable-llvm-optzns -o - %s -w -fms-compatibility-version=19.00 | FileCheck
--check-prefix=MSC --check-prefix=M32 -check-prefix=MSVC2015 %s
+// RUN: %clang_cc1 -triple i686-windows-msvc -emit-llvm -std=c++1y
-fno-threadsafe-statics -fms-extensions -O1 -mconstructor-aliases
-disable-llvm-optzns -o - %s -w -fms-compatibility-version=18.00 | FileCheck
--check-prefix=MSC --check-prefix=M32 -check-prefix=MSVC2013 %s
+
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -std=c++1y
-fno-threadsafe-statics -fms-extensions -O0 -o - %s -w
-fms-compatibility-version=19.00 | FileCheck --check-prefix=MSC
--check-prefix=M64 -check-prefix=MSVC2015 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -std=c++1y
-fno-threadsafe-statics -fms-extensions -O0 -o - %s -w
-fms-compatibility-version=18.00 | FileCheck --check-prefix=MSC
--check-prefix=M64 -check-prefix=MSVC2013 %s
+
// RUN: %clang_cc1 -triple i686-windows-gnu-emit-llvm -std=c++1y
-fno-threadsafe-statics -fms-extensions -O0 -o - %s -w | FileCheck
--check-prefix=GNU --check-prefix=G32 %s
// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -std=c++1y
-fno-threadsafe-statics -fms-extensions -O0 -o - %s -w | FileCheck
--check-prefix=GNU --check-prefix=G64 %s
@@ -528,6 +532,8 @@
SomeTemplate(T o = T()) : o(o) {}
T o;
};
+// MSVC2015-DAG: define weak_odr dllexport {{.+}}
@"\01??4?$SomeTemplate@H@@Q{{.+}}@$$Q{{.+}}@@Z"
+// MSVC2013-DAG: define weak_odr dllexport {{.+}}
@"\01??4?$SomeTemplate@H@@Q{{.+}}0@A{{.+}}0@@Z"
struct __declspec(dllexport) InheritFromTemplate : SomeTemplate {};
// M32-DAG: define weak_odr dllexport x86_thiscallcc void
@"\01??_F?$SomeTemplate@H@@QAEXXZ"({{.*}}) {{#[0-9]+}} comdat
@@ -616,7 +622,8 @@
struct __declspec(dllexport) Y {
// Move assignment operator:
- // M32-DAG: define weak_odr dllexport x86_thiscallcc
dereferenceable({{[0-9]+}}) %struct.Y* @"\01??4Y@@QAEAAU0@$$QAU0@@Z"
+ // MSVC2015-DAG: define weak_odr dllexport {{.+}}
@"\01??4Y@@Q{{.+}}@$$Q{{.+}}@@Z"
+ // MSVC2013-DAG: define weak_odr dllexport {{.+}}
@"\01??4Y@@Q{{.+}}0@A{{.+}}0@@Z"
int x;
};
@@ -933,3 +940,15 @@
USEMEMFUNC(ExplicitInstantiationDeclTemplateBase2, func)
// M32-DAG: define weak_odr dllexport x86_thiscallcc void
@"\01?func@?$ExplicitInstantiationDeclTemplateBase2@H@@QAEXXZ"
// G32-DAG: define weak_odr x86_thiscallcc void
@_ZN38ExplicitInstantiationDeclTemplateBase2IiE4funcEv
+
+class __declspec(dllexport) ACE_Shared_Object {
+ public:
+virtual ~ACE_Shared_Object ();
+};
+
+class __declspec(dllexport) ACE_Service_Object: public ACE_Shared_Object {};
+
+// Implicit move constructor declaration.
+// MSVC2015-DAG: define weak_odr dllexport
{{.+}}ACE_Service_Object@@Q{{.+}}@$$Q
+// The declarations should not be exported.
+// MSVC2013-NOT: define weak_odr dllexport
{{.+}}ACE_Service_Object@@Q{{.+}}
