Page MenuHomePhabricator

[ms][dll] #27212: Generating of implicit special members should take into account MSVC compatibility version
ClosedPublic

Authored by avt77 on Apr 15 2016, 4:52 AM.

Details

Summary

Clang creates implicit move constructor/assign operator in all cases if there is std=c++11. But MSVC supports such generation starting from version 1900 only. As result we have some binary incompatibility.

Diff Detail

Event Timeline

avt77 updated this revision to Diff 53873.Apr 15 2016, 4:52 AM
avt77 retitled this revision from to [ms][dll] #27212: Generating of implicit special members should take into account MSVC compatibility version.
avt77 updated this object.
avt77 added a reviewer: rnk.
avt77 added a subscriber: cfe-commits.
rnk requested changes to this revision.Apr 20 2016, 10:04 AM
rnk edited edge metadata.

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.

This revision now requires changes to proceed.Apr 20 2016, 10:04 AM
avt77 added a comment.Apr 26 2016, 3:19 AM

It seems it will be even shorter if we do it via Sema::checkClassLevelDLLAttribute. Tnx.

avt77 updated this revision to Diff 55567.Apr 29 2016, 4:31 AM
avt77 edited edge metadata.

Now it's really a micro-patch: please review.

rnk added inline comments.May 11 2016, 8:30 AM
lib/Sema/SemaDeclCXX.cpp
4813

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.

rnk added inline comments.May 11 2016, 8:49 AM
lib/Sema/SemaDeclCXX.cpp
4816

The move constructor part of this is definitely a good fix though.

avt77 marked 2 inline comments as done.May 12 2016, 1:53 AM
avt77 added inline comments.
lib/Sema/SemaDeclCXX.cpp
4813

Yes, it's a question about binary compatibility only

4816

And what's the decision? Could I commit the patch?

rnk accepted this revision.May 12 2016, 10:46 AM
rnk edited edge metadata.

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.

This revision is now accepted and ready to land.May 12 2016, 10:46 AM