Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix MSVC MT/MD incompatibility in PYBIND11_BUILD_ABI #4953

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Fix MSVC MT/MD incompatibility in PYBIND11_BUILD_ABI #4953

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
607812a
Fix MSVC MT/MD incompatibility in PYBIND11_BUILD_ABI
isuruf Nov 28, 2023
71b0e9e
Update comment about which PR
isuruf Nov 28, 2023
d1695f1
Use msvc major version
isuruf Nov 30, 2023
4c6c344
Use _MSC_VER/100
isuruf Oct 1, 2024
543b08f
Fix figuring out MD vs MT
isuruf Oct 14, 2024
e7a559d
Add some test runs
isuruf Oct 14, 2024
12a6478
Skip one test
isuruf Oct 14, 2024
10dd08a
Fix preprocessor
isuruf Oct 14, 2024
72b6d2a
simplify code
isuruf Oct 14, 2024
9ce2def
fix if
isuruf Oct 14, 2024
1d2f952
support only msvc 19
isuruf Oct 15, 2024
0131c55
Merge branch 'master' into incompat
rwgk Oct 23, 2024
c3415a9
Fold in changes from experimental PR #5411. Polish error messages.
rwgk Oct 23, 2024
a5b5c3b
Remove `&& defined(_DLL)` (TBD: is it needed? but what is correct?)
rwgk Oct 23, 2024
a3944f3
Fix MT vs MD
isuruf Oct 24, 2024
b9b54a7
Add a couple comments, based on https://github.com/pybind/pybind11/pu…
rwgk Oct 24, 2024
713b427
Replace misleading comment: NVHPC is NOT outdated.
rwgk Oct 28, 2024
79a3770
Merge branch 'master' into incompat
rwgk Oct 28, 2024
ef6c3f0
Update include/pybind11/detail/internals.h
rwgk Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,24 @@ jobs:
# Inject a couple Windows 2019 runs
- runs-on: windows-2019
python: '3.9'
# Inject a few runs with different runtime libraries
- runs-on: windows-2022
python: '3.9'
args: >
-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded
- runs-on: windows-2022
python: '3.10'
args: >
-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDLL
# This needs a python built with MTd
# - runs-on: windows-2022
# python: '3.11'
# args: >
# -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug
- runs-on: windows-2022
python: '3.12'
args: >
-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebugDLL
# Extra ubuntu latest job
- runs-on: ubuntu-latest
python: '3.11'
Expand Down
21 changes: 18 additions & 3 deletions include/pybind11/detail/internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

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.)

Copy link
Contributor Author

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.

Copy link
Collaborator

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.)

Copy link
Contributor Author

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.

Copy link
Collaborator

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:

#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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if the assumption that all _MSC_VER >= 1900 && _MSC_VER < 2000 are fully binary compatible is valid, but I'm willing to take your word for it. I'll defer to @wjakob for make the final call on this one.

It would be ideal to reduce the redundancy and avoid the define PYBIND11_BUILD_ABI "" fallback. (IMO it would be good to get rid of that fallback entirely, but not in this PR.) How about this?

#    elif defined(_MSC_VER)
#        if defined(_MT)
#            define PYBIND11_BUILD_ABI "_mt_mscver" PYBIND11_TOSTRING(_MSC_VER)
#        elif defined(_MD)
#            if _MSC_VER >= 1900 && _MSC_VER < 2000
#                define PYBIND11_BUILD_ABI "_md_mscver14"
#            else
#                define PYBIND11_BUILD_ABI "_md_mscver" PYBIND11_TOSTRING(_MSC_VER)
#            else
#                define PYBIND11_BUILD_ABI "_unkown_mscver" PYBIND11_TOSTRING(_MSC_VER)
#            endif
#        endif

I'm not sure about the _unkown_mscver fallback. Maybe #error Unkown combination of MSVC macros (or similar) would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using _MSC_VER/100 now to get the major version. See https://learn.microsoft.com/en-us/cpp/overview/compiler-versions?view=msvc-170 on how _MSC_VER is formatted.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to ask, did you consider the idea of preserving "_mscver" for one branch of the #if defined logic?

#4953 (comment)

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
Expand Down