Fix MSVC MT/MD incompatibility in PYBIND11_BUILD_ABI #4953

isuruf · 2023-11-28T16:08:37Z

Description

The ABI is compatible between different versions of MSVC 14.* (2015 - 14.0, 2017 - 14.1, 2019 - 14.2, 2022 - 14.3).
However, the internal structures are different between these versions. For eg: the internal details of how C++ structures like maps are implemented might be different. (I'm not saying they are, but how maps are implemented is an implementation detail)

With /MT you are statically linking in the C runtime and when allocating one structure in a DLL created with a different MSVC version, runtime linked statically, and trying to use it in another DLL linked to a different MSVC runtime (static or dynamic) results in crashes. That's why when you link with /MT and is crossing DLL boundaries you need them to have the same version.

However, if you are using /MD you are using only one C runtime and the internal structures are consistent. Therefore you don't need to check MSVC version.

See https://learn.microsoft.com/en-us/cpp/porting/binary-compat-2015-2017?view=msvc-170

For increased safety, this PR also eliminates the fall-through case leaving PYBIND11_BUILD_ABI blank.

Suggested changelog entry:

rwgk · 2023-11-28T18:22:45Z

Could you please explain the rationale in the PR description?

IIUC, currently this PR undoes #4779 for /MD, is that the right thing to do?

Because ultimately we're sharing C++ pointers between Python extension modules. The C++ ABI must be compatible, independently of everything else. Falling back to define PYBIND11_BUILD_ABI "" (IIUC) seems very suspicious.

I don't know a lot about Windows linking and the various options. I'd appreciate if you could take the time to explain carefully in the PR description so that people like myself can clearly see what is correct and why. (So that we don't have to come back here again.)

isuruf · 2023-11-28T18:28:12Z

IIUC, currently this PR undoes #4779 for /MD, is that the right thing to do?

Yes, the ABI is compatible between different versions of MSVC 14.* (2015 - 14.0, 2017 - 14.1, 2019 - 14.2, 2022 - 14.3).
However, the internal structures are different between these versions.

With /MT you are statically linking in the C runtime and when allocating one structure in a DLL created with a different MSVC version, runtime linked statically, and trying to use it in another DLL linked to a different MSVC runtime (static or dynamic) results in crashes. That's why when you link with /MT and is crossing DLL boundaries you need them to have the same version.

However, if you are using /MD you are using only one C runtime and the internal structures are consistent. Therefore you don't need to check MSVC version.

rwgk · 2023-11-28T18:43:10Z

Yes, the ABI is compatible between different versions of MSVC 14.* (2015 - 14.0, 2017 - 14.1, 2019 - 14.2, 2022 - 14.3).

Amazing.

However, the internal structures are different between these versions.

Could you please clarify what the "internal structures" are?

If you move your comment with that clarification to the PR description it'll look good to me.

Please also point to this PR from the comment in internals.h (See (#4779) -> See (#4953)).

@wjakob this looks like something we want to keep in sync between pybind11 and nanobind.

rwgk

Thanks, I'm glad you caught this before we made a release.

wjakob · 2023-11-28T21:24:09Z

I am not so sure about this one @rwgk @isuruf . Consider the following scenario:

User A builds an extension with an ancient version of MSVC.
User B builds an extension with a really recent one.
Once imported into the same Python interpreter, both the extension of user A and B will use the same runtime C library. This means that calls to functions like malloc() and free() are consistently handled by the same heap. So far so good.
Let's also say that this patch gets merged, and both of these users compile with /MD. This means that their extensions have the same pybind11 ABI key. So they are allowed to talk to each other via pybind11's internal data structures.

This is where we are leaving the world of C ABI compatibility world and entering the world of C++ ABI compatibility. The opener of the PR says:

However, the internal structures are different between these versions. For eg: the internal details of how C++ structures like maps are implemented might be different.

This is precisely the issue. And it is also the issue in the other related PR (#3793) and the CondaForge issue (conda-forge/pybind11-feedstock#79), where I am surprised about the nonchalance in introducing an (in my opinion) dangerous change.

After following the above scenario, each of the two pybind11 extensions will now want to read from and write to a shared internal data structure containing lots of nested STL types. It is a minefield -- even the slightest change in how those types are implemented will cause undefined behavior. Crashes in the simplest case, and silent data corruption on the worse side of the scale.

This is just the pybind11 internals -- perhaps one could go through through the entire MSVC C++ library and confirm that types used to implement pybind11 itself (std::type_index, std::string, std::pair, std::tuple, std::unordered_set, std::unordered_multimap, std::vector, std::forward_list, etc.) are indeed 100% ABI compatible between all relevant MSVC versions since they are in any case pretty old and hopefully stabilized.

But it does not stop there in my opinion: pybind11 has not just the job of protecting its own internals. It should also protect user data structures from ABI incompatibilities.The point of multiple extensions sharing a pybind11 "domain" is so that functions in these extensions can call each other with instances across libraries.

Perhaps both users A and B created an extension that includes the header

struct Foo {
   std::fancy_cpp20_type<...> foo;
};

Well, if user A's extension creates an instance of the type Foo and then passes it to a function in user B's extension, then this can still crash if std::fancy_cpp20_type isn't ABI compatible between the MSC versions that built these libraries.

(case in point, C++20 features were exposed in MSVC but were not API stable yet microsoft/STL#1814 (comment))

I will say what I said earlier in the two other commits: most pybind11 extensions don't need to need type-level access to the internals of other pybind11 extensions. They can be isolated and everything works fine.

It is just a small subset of extensions that have such an "intimate" relationship. I think it is reasonable to expect that such extensions are built with C++ libraries that declare themselves as being ABI-compatible.

I know that this can sometimes be inconvenient. But weakening the pybind11 protection mechanism is not the right solution to this problem.

isuruf · 2023-11-28T22:09:48Z

Are there any pybind developer calls where we can discuss these issues? I feel like it would be easier to discuss these on a call and summarize the findings.

rwgk · 2023-11-28T22:11:46Z

After following the above scenario, each of the two pybind11 extensions will now want to read from and write to a shared internal data structure containing lots of nested STL types.

I didn't think that through before. I'm trying to make that concrete for myself by looking at the sources. I think this is the door through which the potential for UB slips in:

pybind11/include/pybind11/detail/internals.h

Line 498 in 4bb6163

internals_pp = get_internals_pp_from_capsule(internals_obj);

Here extension module B is extracting a pointer to an object of this struct created in extension module A:

pybind11/include/pybind11/detail/internals.h

Lines 169 to 223 in 4bb6163

    
           struct internals { 
        
               // std::type_index -> pybind11's type information 
        
               type_map<type_info *> registered_types_cpp; 
        
               // PyTypeObject* -> base type_info(s) 
        
               std::unordered_map<PyTypeObject *, std::vector<type_info *>> registered_types_py; 
        
               std::unordered_multimap<const void *, instance *> registered_instances; // void * -> instance* 
        
               std::unordered_set<std::pair<const PyObject *, const char *>, override_hash> 
        
                   inactive_override_cache; 
        
               type_map<std::vector<bool (*)(PyObject *, void *&)>> direct_conversions; 
        
               std::unordered_map<const PyObject *, std::vector<PyObject *>> patients; 
        
               std::forward_list<ExceptionTranslator> registered_exception_translators; 
        
               std::unordered_map<std::string, void *> shared_data; // Custom data to be shared across 
        
                                                                    // extensions 
        
           #if PYBIND11_INTERNALS_VERSION == 4 
        
               std::vector<PyObject *> unused_loader_patient_stack_remove_at_v5; 
        
           #endif 
        
               std::forward_list<std::string> static_strings; // Stores the std::strings backing 
        
                                                              // detail::c_str() 
        
               PyTypeObject *static_property_type; 
        
               PyTypeObject *default_metaclass; 
        
               PyObject *instance_base; 
        
           #if defined(WITH_THREAD) 
        
               // Unused if PYBIND11_SIMPLE_GIL_MANAGEMENT is defined: 
        
               PYBIND11_TLS_KEY_INIT(tstate) 
        
           #    if PYBIND11_INTERNALS_VERSION > 4 
        
               PYBIND11_TLS_KEY_INIT(loader_life_support_tls_key) 
        
           #    endif // PYBIND11_INTERNALS_VERSION > 4 
        
               // Unused if PYBIND11_SIMPLE_GIL_MANAGEMENT is defined: 
        
               PyInterpreterState *istate = nullptr; 
        
           #    if PYBIND11_INTERNALS_VERSION > 4 
        
               // Note that we have to use a std::string to allocate memory to ensure a unique address 
        
               // We want unique addresses since we use pointer equality to compare function records 
        
               std::string function_record_capsule_name = internals_function_record_capsule_name; 
        
           #    endif 
        
               internals() = default; 
        
               internals(const internals &other) = delete; 
        
               internals &operator=(const internals &other) = delete; 
        
               ~internals() { 
        
           #    if PYBIND11_INTERNALS_VERSION > 4 
        
                   PYBIND11_TLS_FREE(loader_life_support_tls_key); 
        
           #    endif // PYBIND11_INTERNALS_VERSION > 4 
        
                   // This destructor is called *after* Py_Finalize() in finalize_interpreter(). 
        
                   // That *SHOULD BE* fine. The following details what happens when PyThread_tss_free is 
        
                   // called. PYBIND11_TLS_FREE is PyThread_tss_free on python 3.7+. On older python, it does 
        
                   // nothing. PyThread_tss_free calls PyThread_tss_delete and PyMem_RawFree. 
        
                   // PyThread_tss_delete just calls TlsFree (on Windows) or pthread_key_delete (on *NIX). 
        
                   // Neither of those have anything to do with CPython internals. PyMem_RawFree *requires* 
        
                   // that the `tstate` be allocated with the CPython allocator. 
        
                   PYBIND11_TLS_FREE(tstate); 
        
               } 
        
           #endif 
        
           };

If the <vector> etc. headers seen by B are different from those seen by A, we have UB if B is operating on the internals object.

Sounds convincing to me.

rwgk · 2023-11-28T22:15:20Z

Are there any pybind developer calls where we can discuss these issues?

We're in vastly different timezones. I think sorting this out via comments here will work best. (We didn't have a meeting in a long time.)

isuruf · 2023-11-28T22:16:21Z

@rwgk, that wouldn't happen as the allocation of internal structures happen in the same C runtime. If the external view of the object changes, then the stdlib developers would indicate this so that things wouldn't link at all. For eg: the std::string ABI changed from g++ 4 to g++ 5. That's the external view of the object. The internal view of the object does not matter unless you are statically linking in the C runtime.

isuruf · 2023-11-28T22:26:44Z

(case in point, C++20 features were exposed in MSVC but were not API stable yet microsoft/STL#1814 (comment))

This is a valid point. API stability in the case of experimental features is an issue in all compilers.

rwgk · 2023-11-28T22:43:50Z

then the stdlib developers would indicate this so that things wouldn't link at all.

That's a cool feature (I didn't know about it), but I don't think we have that protection:

Here the internals** is cast to a void* (stored in a Python capsule):

pybind11/include/pybind11/detail/internals.h

Line 535 in 4bb6163

state_dict[PYBIND11_INTERNALS_ID] = capsule(internals_pp);

And here the void* is cast back to internals**:

pybind11/include/pybind11/detail/internals.h

Line 469 in 4bb6163

return static_cast<internals **>(raw_ptr);

That by-passes any link checks. The extension modules are "connected" only at runtime.

Is it possible to have a runtime check for e.g. std::vector compatibility between MSVC versions? (To substitute for the link-time check?) (And is is simple enough, so that gain / cost is reasonable?)

isuruf · 2023-11-28T23:00:05Z

You can use typeid(func).name() before the type erasure to get the mangled name.

rwgk · 2023-11-28T23:46:19Z

You can use typeid(func).name() before the type erasure to get the mangled name.

Is that documented to be unique to a given "external view"?

(The mangled names I happened to see over the years didn't appear to encode any versioning information.)

rwgk · 2023-11-29T19:16:49Z

After turning this over in the back of my mind a little bit:

I think it's clear that falling back to define PYBIND11_BUILD_ABI "" is unsafe.

But it also sounds like using define PYBIND11_BUILD_ABI "_mscver" PYBIND11_TOSTRING(_MSC_VER) is more restrictive than it needs to be.

Could we find a middle ground? Maybe, @isuruf do you think you could add preprocessor-level functionality to map _MSC_VER to something like a PYBIND11_MSVC_ABI_COMPATIBILITY_GROUP, which we then use for PYBIND11_BUILD_ABI?

rwgk · 2023-12-01T01:05:37Z

include/pybind11/detail/internals.h

 #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(_MT)


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?

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.

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.

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.

wjakob · 2023-12-01T08:58:28Z

Hi @isuruf and @rwgk,

I had a bit of time look a bit into C++ ABI stability in the last few days. I think that I am too paranoid after having been repeatedly bitten by this some years ago. It seems that basically all the three main compiler tools (GCC, Clang, MSVC) have started taking ABI stability seriously at some point in the last years and there haven't been major breaks since then.

So I'm open to making a change here that allows interoperability between more extensions built with different compilers. But let's get it right and make something that we hopefully won't have to tweak retroactively because the criterion turns out to be too loose and, e.g., and ancient MSVC version turns out to not be ABI-compatible after all.

One important think to keep in mind (@rwgk) is that a change here is itself an ABI break, of pybind11. Modifying the PYBIND11_BUILD_ABI string means that the extension is now isolated from all other extensions previously built with a different PYBIND11_BUILD_ABI string. So we should put this change into a larger release that bundles accumulated ABI-breaking commits.

-Wenzel

rwgk · 2024-07-03T17:03:09Z

@isuruf: this PR was mentioned under conda-forge/pybind11-feedstock#95

This PR slipped my attention. We missed the pybind11 v2.13.0 release. @isuruf do you have an opinion when this is best merged? Would it make sense to tweak this PR, to avoid further ABI breaks? E.g. currently the diff under this PR is:

-#        define PYBIND11_BUILD_ABI "_mscver" PYBIND11_TOSTRING(_MSC_VER)
+#    elif defined(_MSC_VER) && defined(_MT)
+#        define PYBIND11_BUILD_ABI "_mt_mscver" PYBIND11_TOSTRING(_MSC_VER)
+#    elif defined(_MSC_VER) && defined(_MD) && (_MSC_VER >= 1900) && (_MSC_VER < 2000)
+#        define PYBIND11_BUILD_ABI "_md_mscver14"
+#    elif defined(_MSC_VER) && defined(_MD)
+#        define PYBIND11_BUILD_ABI "_md_mscver" PYBIND11_TOSTRING(_MSC_VER)

could this (note the <<<< LOOK HERE)

-#        define PYBIND11_BUILD_ABI "_mscver" PYBIND11_TOSTRING(_MSC_VER)
+#    elif defined(_MSC_VER) && defined(_MT)
+#        define PYBIND11_BUILD_ABI "_mscver" PYBIND11_TOSTRING(_MSC_VER)  <<<< LOOK HERE
+#    elif defined(_MSC_VER) && defined(_MD) && (_MSC_VER >= 1900) && (_MSC_VER < 2000)
+#        define PYBIND11_BUILD_ABI "_md_mscver14"
+#    elif defined(_MSC_VER) && defined(_MD)
+#        define PYBIND11_BUILD_ABI "_md_mscver" PYBIND11_TOSTRING(_MSC_VER)

or this

-#        define PYBIND11_BUILD_ABI "_mscver" PYBIND11_TOSTRING(_MSC_VER)
+#    elif defined(_MSC_VER) && defined(_MT)
+#        define PYBIND11_BUILD_ABI "_mt_mscver" PYBIND11_TOSTRING(_MSC_VER)
+#    elif defined(_MSC_VER) && defined(_MD) && (_MSC_VER >= 1900) && (_MSC_VER < 2000)
+#        define PYBIND11_BUILD_ABI "_md_mscver14"
+#    elif defined(_MSC_VER) && defined(_MD)
+#        define PYBIND11_BUILD_ABI "_mscver" PYBIND11_TOSTRING(_MSC_VER)  <<<< LOOK HERE

be better?

rwgk · 2024-07-03T17:04:10Z

@henryiii @Skylion007 for visitility (see my other comment posted a minute ago)

rwgk · 2024-10-14T18:21:27Z

Here's the new output after fixing a bug and expanding tests:

     13 __pybind11_internals_vX_msvc_md_mscver((int) (1929) / 100)_debug__ 
      3 __pybind11_internals_vX_msvc_md_mscver((int) (1941) / 100)__ 
     21 __pybind11_internals_vX_msvc_md_mscver((int) (1941) / 100)_debug__

Do we want __pybind11_internals_vX_msvc_md_mscver19__ here? Are you looking into this already?

isuruf · 2024-10-14T19:31:20Z

Fixed now. New values

     26 __pybind11_internals_vX_clang_libcpp_cxxabi1002__ 
     10 __pybind11_internals_vX_clang_libstdcpp_cxxabi1002__ 
     27 __pybind11_internals_vX_gcc_libstdcpp_cxxabi1013__ 
      3 __pybind11_internals_vX_gcc_libstdcpp_cxxabi1014__ 
      3 __pybind11_internals_vX_gcc_libstdcpp_cxxabi1016__ 
      2 __pybind11_internals_vX_gcc_libstdcpp_cxxabi1017__ 
      5 __pybind11_internals_vX_gcc_libstdcpp_cxxabi1018__ 
      2 __pybind11_internals_vX_icc_libstdcpp_cxxabi1010__ 
      6 __pybind11_internals_vX_mingw_libstdcpp_cxxabi1019__ 
      3 __pybind11_internals_vX_msvc_md_mscver19__ 
     34 __pybind11_internals_vX_msvc_md_mscver19_debug__ 
      3 __pybind11_internals_vX_msvc_mt_mscver1941__ 
      1 __pybind11_internals_vX_pgi_libstdcpp__

rwgk · 2024-10-14T21:01:07Z

include/pybind11/detail/internals.h

+/// 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


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

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.

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

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.

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.

rwgk · 2024-10-23T23:11:13Z

I think something went wrong between the two commits below. Some mixup between testing for defined(_MT) and then defining "_md_mscver19". There is also && defined(_DLL) in the mix.

My commit c3415a9 is almost certainly also flawed. I removed the && defined(_DLL) condition in my latest commit a5b5c3b. Do we need to put it back somewhere else?

commit 4c6c344
Author: Isuru Fernando [email protected]
Date: Mon Sep 30 19:53:53 2024 -0500

#ifndef PYBIND11_BUILD_ABI
#    if defined(__GXX_ABI_VERSION)
#        define PYBIND11_BUILD_ABI "_cxxabi" PYBIND11_TOSTRING(__GXX_ABI_VERSION)
#    elif defined(_MSC_VER) && defined(_MT)
#        define PYBIND11_BUILD_ABI "_mt_mscver" PYBIND11_TOSTRING(_MSC_VER)
#    elif defined(_MSC_VER) && defined(_MD)
#        define PYBIND11_BUILD_ABI "_md_mscver" PYBIND11_TOSTRING(((int) (_MSC_VER) / 100))
#    else
#        define PYBIND11_BUILD_ABI ""
#    endif
#endif

commit 1d2f952
Author: Isuru Fernando [email protected]
Date: Tue Oct 15 14:45:57 2024 -0400

#ifndef PYBIND11_BUILD_ABI
#    if defined(__GXX_ABI_VERSION)
#        define PYBIND11_BUILD_ABI "_cxxabi" PYBIND11_TOSTRING(__GXX_ABI_VERSION)
#    elif defined(_MSC_VER) && defined(_DLL) && defined(_MT)
#        if ((_MSC_VER) / 100 == 19)
#            define PYBIND11_BUILD_ABI "_md_mscver19"
#        else
#            error "Unknown major version for MSC_VER"
#        endif
#    elif defined(_MSC_VER) && defined(_MT)
#        define PYBIND11_BUILD_ABI "_mt_mscver" PYBIND11_TOSTRING(_MSC_VER)
#    else
#        define PYBIND11_BUILD_ABI ""
#    endif
#endif

isuruf · 2024-10-24T12:20:01Z

@rwgk this is ready now. See https://learn.microsoft.com/en-us/cpp/build/reference/md-mt-ld-use-run-time-library?view=msvc-170#remarks for more details. Basically

Option	macros
/MD	`_MT`, `_DLL`
/MT	`_MT`
/MDd	`_MT`, `_DLL`, `_DEBUG`
/MTd	`_MT`, `_DEBUG`

rwgk · 2024-10-24T22:17:54Z

Thanks @isuruf for the explanation!

For quality assurance: I found this in the latest GHA logs (LGTM):

     26 __pybind11_internals_vX_clang_libcpp_cxxabi1002__
     10 __pybind11_internals_vX_clang_libstdcpp_cxxabi1002__
     26 __pybind11_internals_vX_gcc_libstdcpp_cxxabi1013__
      3 __pybind11_internals_vX_gcc_libstdcpp_cxxabi1014__
      6 __pybind11_internals_vX_gcc_libstdcpp_cxxabi1016__
      2 __pybind11_internals_vX_gcc_libstdcpp_cxxabi1017__
      2 __pybind11_internals_vX_gcc_libstdcpp_cxxabi1018__
      2 __pybind11_internals_vX_icc_libstdcpp_cxxabi1010__
      6 __pybind11_internals_vX_mingw_libstdcpp_cxxabi1019__
      3 __pybind11_internals_vX_msvc_md_mscver19__
     34 __pybind11_internals_vX_msvc_md_mscver19_debug__
      3 __pybind11_internals_vX_msvc_mt_mscver1941__
      1 __pybind11_internals_vX_pgi_libstdcpp__

).

rwgk · 2024-10-24T22:39:20Z

Hi @robertmaynard, is there a chance that you could help review the changes in include/pybind11/detail/internals.h? — It's <20 changed/added lines. To have a clear track record, it would be great if you could formally approve this PR if/when it looks good to you.

Main question: Is that a reasonable compromise for MSVC, safety vs practicality?

Beyond fixing up PYBIND11_BUILD_ABI for MSVC:

For safety, I want to eliminate silently falling back to #define PYBIND11_BUILD_ABI "".
Except for __NVCOMPILER (NVHPC), because I'm assuming that it is outdated and not worth creating a stir. Does that make sense? — Specifically, does my comment there look OK? (Does NVHPC have something like an official end of life?)

robertmaynard · 2024-10-28T17:29:27Z

For safety, I want to eliminate silently falling back to #define PYBIND11_BUILD_ABI "".

Except for __NVCOMPILER (NVHPC), because I'm assuming that it is outdated and not worth creating a stir. Does that make sense? — Specifically, does my comment there look OK? (Does NVHPC have something like an official end of life?)

NVHPC is actively maintained and new version come out every couple of months.

rwgk · 2024-10-28T17:39:51Z

NVHPC is actively maintained and new version come out every couple of months.

Thanks! I just revised the comment (713b427). — Coincidentally I was working on revising the misleading comment before I saw your comment here, after asking my teammates about it.

include/pybind11/detail/internals.h

robertmaynard · 2024-10-28T18:00:31Z

include/pybind11/detail/internals.h

 #ifndef PYBIND11_BUILD_ABI
-#    if defined(__GXX_ABI_VERSION)
+#    if defined(__GXX_ABI_VERSION) // Linux/OSX.


I know this PR is for msvc but this isn't the correct way to check for linux.

Background:
This macro value is the same across all version of clang ( always 1002 ) but changes anytime the gcc internal compiler ABI changes, which generally has no impact. You can find more details on the gcc abi changes here: https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html

Generally you can safely pass types across anything 1002 or newer as long as the _GLIBCXX_USE_CXX11_ABI ( https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html ) is the same.

So if you build two libraries with differing values of _GLIBCXX_USE_CXX11_ABI they can't have the same abi:

~ $ clang++-16 -D_GLIBCXX_USE_CXX11_ABI=0 ./test.cpp -shared && nm ./a.out | grep foo | c++filt 00000000000011a0 T foo(std::basic_string<char, std::char_traits<char>, std::allocator<char> >) ~ $ clang++-16 -D_GLIBCXX_USE_CXX11_ABI=1 ./test.cpp -shared && nm ./a.out | grep foo | c++filt 00000000000011a0 T foo(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)

But that isn't captured in your ABI checks at all and is significantly more important compared to __GXX_ABI_VERSION

robertmaynard · 2024-10-28T18:40:10Z

include/pybind11/detail/internals.h

+#        else
+#            error "Unknown combination of MSVC preprocessor macros: PLEASE REVISE THIS CODE."
+#        endif
+#    elif defined(__NVCOMPILER)       // NVHPC (PGI-based).


For nvhpc you would check _GLIBCXX_USE_CXX11_ABI like you would for gcc or clang. You can combo that with __GNUC__ and __GNUC_MINOR__ to get the gcc compatible version that nvhpc is targetting. IIRC the behavior of nvhpc is always -fabi-version=0 where the latest is defined by the gcc it is targetting ( e.g. GNUC , GNUC_MINOR defines ).

Co-authored-by: Robert Maynard <[email protected]>

This commit incorporates some of the suggestions from pybind11 PR pybind/pybind11#4953 (and linked discussion) to relax ABI tagging for libstdc++ and MSVC. At the same time, it adds an extra ABI tag for libc++ (https://libcxx.llvm.org/DesignDocs/ABIVersioning.html) that was previously missing.

wjakob · 2024-11-05T00:51:45Z

Hi all -- the changes for MSVC look good to me. However, as @robertmaynard has mentioned, this is also an opportunity to relax the ABI tagging for libstdc++, and that should be done in the same ABI break.

While looking into ABIs for different compilers, I also found this for libc++: https://libcxx.llvm.org/DesignDocs/ABIVersioning.html. Setting a higher version causes some incompatibilities with standard C++ containers. IMO, pybind11 should also check for this.

I started working on an analogous set of changes for nanobind here: wjakob/nanobind#778

This commit incorporates some of the suggestions from pybind11 PR pybind/pybind11#4953 (and linked discussion) to relax ABI tagging for libstdc++ and MSVC. At the same time, it adds an extra ABI tag for libc++ (https://libcxx.llvm.org/DesignDocs/ABIVersioning.html) that was previously missing.

This commit incorporates some of the suggestions from pybind11 PR pybind/pybind11#4953 (and linked discussion) to relax ABI tagging for libstdc++ and MSVC. The goal is to ensure that ABI-incompatible extensions are separated from each other. These checks were previously too fine-grained and caused isolation in cases where an actual ABI incompatibility was not present. This is a long-standing problem in both pybind11 and nanobind causing inconvenience for users. While looking into this topic, I realized that libc++ has an opt-in unstable ABI feature (https://libcxx.llvm.org/DesignDocs/ABIVersioning.html). The PR also adds checks for this. Merging this PR will imply an ABI version bump for nanobind due to the differnt naming convention of the associated ABI tag.

isuruf mentioned this pull request Nov 28, 2023

[BUG] - crash on c++ exception when importing 2 pybind submodules built from different MSVC versions #2898

Open

rwgk mentioned this pull request Nov 28, 2023

fix: Different MSVC versions may be ABI incompatible, guard with _MSC_VER (fix #2898) #4779

Merged

rwgk approved these changes Nov 28, 2023

View reviewed changes

rwgk reviewed Dec 1, 2023

View reviewed changes

rwgk mentioned this pull request Dec 1, 2023

[BUG]: finer grained ABI compatibility #3793

Open

3 tasks

isuruf mentioned this pull request Oct 1, 2024

pybind11-abi test script is silently broken for pybind11 2.12, and may be missing ABI version 5 conda-forge/pybind11-feedstock#96

Open

1 task

isuruf added 4 commits September 30, 2024 19:50

Fix MSVC MT/MD incompatibility in PYBIND11_BUILD_ABI

607812a

Update comment about which PR

71b0e9e

Use msvc major version

d1695f1

Use _MSC_VER/100

4c6c344

isuruf force-pushed the incompat branch from 4ff53cf to 4c6c344 Compare October 1, 2024 00:54

isuruf added 3 commits October 14, 2024 14:47

Fix preprocessor

10dd08a

simplify code

72b6d2a

fix if

9ce2def

rwgk reviewed Oct 14, 2024

View reviewed changes

isuruf and others added 4 commits October 15, 2024 14:45

support only msvc 19

1d2f952

Merge branch 'master' into incompat

0131c55

Fold in changes from experimental PR pybind#5411. Polish error messages.

c3415a9

Remove && defined(_DLL) (TBD: is it needed? but what is correct?)

a5b5c3b

Fix MT vs MD

a3944f3

Add a couple comments, based on pybind#4953 (comment) (posted by @isuruf

b9b54a7

).

rwgk added 2 commits October 28, 2024 10:33

Replace misleading comment: NVHPC is NOT outdated.

713b427

Merge branch 'master' into incompat

79a3770

robertmaynard approved these changes Oct 28, 2024

View reviewed changes

Update include/pybind11/detail/internals.h

ef6c3f0

Co-authored-by: Robert Maynard <[email protected]>

wjakob mentioned this pull request Nov 5, 2024

Relaxed ABI tagging wjakob/nanobind#778

Open

rwgk added 2 commits November 5, 2024 10:16

Merge branch 'master' into incompat

fa75ca0

Merge branch 'master' into incompat

823a77b

Fix MSVC MT/MD incompatibility in PYBIND11_BUILD_ABI #4953

Are you sure you want to change the base?

Fix MSVC MT/MD incompatibility in PYBIND11_BUILD_ABI #4953

Conversation

isuruf commented Nov 28, 2023 • edited by rwgk Loading

Description

Suggested changelog entry:

rwgk commented Nov 28, 2023

isuruf commented Nov 28, 2023 • edited Loading

rwgk commented Nov 28, 2023

rwgk left a comment

Choose a reason for hiding this comment

wjakob commented Nov 28, 2023 • edited Loading

isuruf commented Nov 28, 2023

rwgk commented Nov 28, 2023

rwgk commented Nov 28, 2023

isuruf commented Nov 28, 2023 • edited Loading

isuruf commented Nov 28, 2023

rwgk commented Nov 28, 2023

isuruf commented Nov 28, 2023 • edited Loading

rwgk commented Nov 28, 2023

rwgk commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjakob commented Dec 1, 2023

rwgk commented Jul 3, 2024

rwgk commented Jul 3, 2024

rwgk commented Oct 14, 2024

isuruf commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Oct 23, 2024

isuruf commented Oct 24, 2024

rwgk commented Oct 24, 2024

rwgk commented Oct 24, 2024

robertmaynard commented Oct 28, 2024

rwgk commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjakob commented Nov 5, 2024 • edited Loading

isuruf commented Nov 28, 2023 •

edited by rwgk

Loading

isuruf commented Nov 28, 2023 •

edited

Loading

wjakob commented Nov 28, 2023 •

edited

Loading

isuruf commented Nov 28, 2023 •

edited

Loading

isuruf commented Nov 28, 2023 •

edited

Loading

wjakob commented Nov 5, 2024 •

edited

Loading