[smart_holder] type_caster ODR guard
#4022
Conversation
|
This PR was Google-globally tested in a couple configurations: HEAD, HEAD + change that motivated this PR. I found 6 missing includes for HEAD, and no additional issues for HEAD + change. I did not find stl.h / stl_bind.h mixes in the wild, therefore I added a test here to be sure that that situation is actually covered for by the ODR guard (b98c9c1). Attempting to explain the observations:
|
…864, which has more changes on top.
…ion_unit_local) {`
…TER_SOURCE_FILE_LINE, baked into PYBIND11_TYPE_CASTER macro.
…esolves "unused" warning when compiling test_custom_type_casters.cpp
…ails to link. Trying GitHub Actions anyway to see if there are any platforms that support https://en.cppreference.com/w/cpp/language/tu_local before C++20. Note that Debian clang 13 C++17 works locally.
|
After commit 0c27340 we have almost all platforms back, except these two: I think I'll leave the Second-level check: |
…spective of the compiler)
|
Fresh quality check:
|
|
Looks good. |
Yes, without the change in descr.h and the new unit tests this would be a pretty small PR. In case someone looks here in the future, regarding the change in descr.h, I had a choice:
Speculating: Hopefully one day the ASAN
When that happens, maybe we can delete everything that was added with this PR, except Crossing into the realm of wishful thinking, even better of course:
It is very uncertain though how much work that will be, and how disruptive it will be to the API. |
…added with master PR pybind#4587
* use C++17 syntax to get rid of recursive template instantiations for concatenating type signatures (#4587) * Apply descr.h `src_loc` change (smart_holder PR #4022) to code added with master PR #4587 * Add test_class_sh_property_non_owning to CMakeLists.txt (fixes oversight in PR #4586) * Resolve clang-tidy errors. * clang-tidy auto fix --------- Co-authored-by: Konstantin Bespalov <[email protected]>
…_guard() constructor accessing translation_unit_local.value
… namespace detail). (pybind#4049) Very minor refactoring to ease development and debugging. Having to declare a local `std::string` has bugged me many times. Nice to get this little nuisance out of the way. Extracted from PR pybind#4022, where it is used like this: ``` std::fprintf(stdout, "\nTYPE_CASTER_ODR_GUARD_IMPL %s %s\n", clean_type_id(intrinsic_type_info.name()).c_str(), source_file_line_from_sloc.c_str()); ```
…ive in the pybind11k repo.) This rolls back pybind#4022 (including follow-on tweaks in other PRs).
Description
In large-scale environments (e.g. Google)
type_casterODR violations are probably the biggest pybind11 pitfall. Without this PR, it is very difficult to ensure that a large application consistently uses the sametype_caster<T>definition for a givenTin all translation units linked together with mutual linker-level visibility. Accidents are to be expected and are very difficult to prevent & debug at scale, for example a mix of translation units that usePYBIND11_MAKE_OPAQUE+ pybind11/stl_bind.h, and others that use pybind11/stl.h. Another type of accident is that customtype_casterincludes are simply forgotten in some translation units, but by chance the linker picks all the righttype_casterfragments (from other translation units), until one day it suddenly does not, because of some unrelated change in the environment (e.g. new compiler version, a system library update), resulting in seemingly inexplicable failures. Specific to the smart_holder branch, there is also the problem that thePYBIND11_SMART_HOLDER_TYPE_CASTERSmacros may accidentally be missing in some translation units. In many cases runtime errors may give useful clues, but in general, the behavior is undefined, which is not acceptable in large-scale production environments.Runtime detection of ODR violations is very elusive, especially when involving templates. Unfortunately, currently the ASAN
detect_odr_violationoption is known to not catchtype_casterODR violations. Thetype_casterODR guard added with this PR is currently the only tool available, although it employs ODR itself to shieldtype_caster::name.sloc(source location) information from the linker mechanisms that discard all but one of multipletype_casterdefinitions. See theWARNINGs in pybind11/detail/descr.h and pybind11/detail/type_caster_odr_guard.h for details. The current implementation is known to work on almost all platforms that meet the compiler requirements (C++17, C++20), except one (gcc 9.4.0 Debug build). See comments below for details.Note that the new test_type_caster_odr_guard_1, test_type_caster_odr_guard_2 pair deliberately violates the ODR (see C++ source code comments), but in a way that is evidently compatible with reliable unit testing, very similar to how the ODR in the guard itself is evidently of a benign kind.
In the pybind11 unit tests, the ODR guard is tested in as many environments as possible, but because of the ODR in the guard itself, it is NOT recommended to turn on the ODR guard in regular builds, production, or debug. The guard is meant to be used similar to a sanitizer, to check for
type_casterODR violations in binaries that are otherwise already fully tested and assumed to be healthy. To activate the ODR guard (outside pybind11/tests), definePYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLEorPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD. See the long comment & code near the top of pybind11/detail/descr.h for details.Suggested changelog entry: