-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix MSVC MT/MD incompatibility in PYBIND11_BUILD_ABI #4953
base: master
Are you sure you want to change the base?
Changes from 10 commits
607812a
71b0e9e
d1695f1
4c6c344
543b08f
e7a559d
12a6478
10dd08a
72b6d2a
9ce2def
1d2f952
0131c55
c3415a9
a5b5c3b
a3944f3
b9b54a7
713b427
79a3770
ef6c3f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -311,12 +311,27 @@ struct type_info { | |
#endif | ||
|
||
/// On Linux/OSX, changes in __GXX_ABI_VERSION__ indicate ABI incompatibility. | ||
/// On MSVC, changes in _MSC_VER may indicate ABI incompatibility (#2898). | ||
/// On MSVC, mixing /MT and /MD will result in crashes. See (#4953) | ||
/// There is no macro for major version for MSVC, so we check for major version | ||
/// 19, 20, 21, 22 for now as major version 19 is MSVC 2015-2022 and we future | ||
/// proof for 3 major versions in the future | ||
#ifndef PYBIND11_BUILD_ABI | ||
# if defined(__GXX_ABI_VERSION) | ||
# define PYBIND11_BUILD_ABI "_cxxabi" PYBIND11_TOSTRING(__GXX_ABI_VERSION) | ||
# elif defined(_MSC_VER) | ||
# define PYBIND11_BUILD_ABI "_mscver" PYBIND11_TOSTRING(_MSC_VER) | ||
# elif defined(_MSC_VER) && defined(_DLL) && defined(_MT) | ||
# if ((_MSC_VER) / 100 == 19) | ||
# define PYBIND11_BUILD_ABI "_md_mscver19" | ||
# elif ((_MSC_VER) / 100 == 20) | ||
# define PYBIND11_BUILD_ABI "_md_mscver20" | ||
# elif ((_MSC_VER) / 100 == 21) | ||
# define PYBIND11_BUILD_ABI "_md_mscver21" | ||
# elif ((_MSC_VER) / 100 == 22) | ||
# define PYBIND11_BUILD_ABI "_md_mscver22" | ||
# else | ||
# error "Unknown major version for MSC_VER" | ||
# endif | ||
# elif defined(_MSC_VER) && defined(_MT) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if the assumption that all It would be ideal to reduce the redundancy and avoid the
I'm not sure about the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll ask around to find someone familiar with the MSVC ABI universe. It's important that we get this right this time around, because everytime we make a change here evidently we're creating a big problem for conda-forge. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to ask, did you consider the idea of preserving The idea is to minimize ABI breakages. I'm just not sure which branch is the best one to pick. |
||
# define PYBIND11_BUILD_ABI "_mt_mscver" PYBIND11_TOSTRING(_MSC_VER) | ||
# else | ||
# define PYBIND11_BUILD_ABI "" | ||
# endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better not to do that (future proof for 3 major versions), because the future is unpredictable. It's only important that we don't silently fall through to
""
like before. (You're doing that already.)(I want to preserve some of the changes under my experimental #5411. Might take me a day or two to get back to that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't characterize it as unpredictable. The version has a predictable MMmm pattern and since there's no way to divide an integer and assign to another macro in the preprocessor, this is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant: It is likely but not certain that future MSVC versions will be ABI compatible if
(_MSC_VER) / 100
are identical.I think it's better to adjust/review here when those versions are released and we start testing. (We can backport to older pybind11 versions as needed to make them work. That's safer than having them assume ABI compatibility that may not exist.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the sentiment though. On other platforms like clang on macos, we assume that things work fine, but on certain platforms like msvc, we assume the worst.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's true.
We don't have anything like
version / 100
anywhere else. (Please correct me if I'm missing something.)Unless ABI compatibility for
version / 100
is a documented feature for a given platform, I believe it's prudent not to jump to conclusions. Safer is better here in my judgement.In the back of my mind:
Obviously the "it will be ok" fall-through has proven to be harmful. Let's do less of that in the future.
See feat: enable type-safe interoperability between different independent Python/C++ bindings systems. #5296 for where my interest in more strictly enforced correctness is coming from. The conduit feature could work between any pair of bindings systems, but it has to be safe to not get a bad name.