diff --git a/include/pybind11/detail/descr.h b/include/pybind11/detail/descr.h index 15c6ee902..89cd5ccc7 100644 --- a/include/pybind11/detail/descr.h +++ b/include/pybind11/detail/descr.h @@ -23,16 +23,26 @@ PYBIND11_NAMESPACE_BEGIN(detail) # define PYBIND11_DESCR_CONSTEXPR const #endif +// type_caster_odr_guard.h requires Translation-Unit-local features +// (https://en.cppreference.com/w/cpp/language/tu_local), standardized only with C++20. +// The ODR guard creates ODR violations itself (see WARNINGs below & in +// type_caster_odr_guard.h), but the dedicated test_type_caster_odr_guard_1, +// test_type_caster_odr_guard_2 pair of unit tests passes reliably with almost all +// tested C++17 & C++20 compilers, and even the exceptions are not due to ODR issues: // * MSVC 2017 does not support __builtin_FILE(), __builtin_LINE(). // * Intel 2021.6.0.20220226 (g++ 9.4 mode) __builtin_LINE() is unreliable // (line numbers vary between translation units). -#if !defined(PYBIND11_DETAIL_DESCR_SRC_LOC_ON) && !defined(PYBIND11_DETAIL_DESCR_SRC_LOC_OFF) \ - && !defined(__INTEL_COMPILER) \ +// Here we want to test the ODR guard in as many environments as possible, but +// it is NOT recommended to turn on the guard in regular builds, production, or +// debug. The guard is meant to be used similar to a sanitizer, to check for type_caster +// ODR violations in binaries that are otherwise already fully tested and assumed to be healthy. +#if defined(PYBIND11_TYPE_CASTER_ODR_GUARD_ON_IF_AVAILABLE) \ + && !defined(PYBIND11_TYPE_CASTER_ODR_GUARD_ON) && !defined(__INTEL_COMPILER) \ && ((defined(_MSC_VER) && _MSC_VER >= 1920) || defined(PYBIND11_CPP17)) -# define PYBIND11_DETAIL_DESCR_SRC_LOC_ON +# define PYBIND11_TYPE_CASTER_ODR_GUARD_ON #endif -#ifdef PYBIND11_DETAIL_DESCR_SRC_LOC_ON +#ifdef PYBIND11_TYPE_CASTER_ODR_GUARD_ON // struct src_loc supports type_caster_odr_guard.h @@ -62,8 +72,7 @@ struct src_loc { #else -// No-op stub, for compilers that do not support __builtin_FILE(), __builtin_LINE(), -// or for situations in which it is desirable to disable the src_loc feature. +// No-op stub, to avoid code duplication, expected to be optimized out completely. struct src_loc { constexpr src_loc(const char *, unsigned) {} @@ -76,8 +85,15 @@ struct src_loc { #endif -#ifdef PYBIND11_DETAIL_DESCR_SRC_LOC_ON -namespace { // Activates TU-local features (see type_caster_odr_guard.h). +#ifdef PYBIND11_TYPE_CASTER_ODR_GUARD_ON +namespace { // WARNING: This creates an ODR violation in the ODR guard itself, + // but we do not have anything better at the moment. +// The ODR violation here is a difference in constexpr between multiple TUs. +// All definitions have the same data layout, the only difference is the +// text const char* pointee (the pointees are identical in value), +// src_loc const char* file pointee (the pointees are different in value), +// src_loc unsigned line value. +// See also: Comment above; WARNING in type_caster_odr_guard.h #endif /* Concatenate type signatures at compile time */ @@ -231,7 +247,7 @@ constexpr descr type_descr(const descr &descr) { return const_name("{", src_loc{nullptr, 0}) + descr + const_name("}"); } -#ifdef PYBIND11_DETAIL_DESCR_SRC_LOC_ON +#ifdef PYBIND11_TYPE_CASTER_ODR_GUARD_ON } // namespace #endif diff --git a/include/pybind11/detail/type_caster_odr_guard.h b/include/pybind11/detail/type_caster_odr_guard.h index d756336ec..9454eba62 100644 --- a/include/pybind11/detail/type_caster_odr_guard.h +++ b/include/pybind11/detail/type_caster_odr_guard.h @@ -6,16 +6,6 @@ #include "descr.h" -// The type_caster ODR guard feature requires Translation-Unit-local entities -// (https://en.cppreference.com/w/cpp/language/tu_local), a C++20 feature, but -// almost all tested C++17 compilers support this feature already. -// The preconditions for PYBIND11_DETAIL_DESCR_SRC_LOC_ON (descr.h) happen to be -// a subset of the preconditions for PYBIND11_TYPE_CASTER_ODR_GUARD_ON. -#if !defined(PYBIND11_TYPE_CASTER_ODR_GUARD_ON) && !defined(PYBIND11_TYPE_CASTER_ODR_GUARD_OFF) \ - && defined(PYBIND11_DETAIL_DESCR_SRC_LOC_ON) -# define PYBIND11_TYPE_CASTER_ODR_GUARD_ON -#endif - #ifdef PYBIND11_TYPE_CASTER_ODR_GUARD_ON # if !defined(PYBIND11_CPP20) && defined(__GNUC__) && !defined(__clang__) @@ -99,7 +89,10 @@ inline void type_caster_odr_guard_impl(const std::type_info &intrinsic_type_info } } -namespace { +namespace { // WARNING: This creates an ODR violation in the ODR guard itself, + // but we do not have anything better at the moment. +// The ODR violation here does not involve any data at all. +// See also: Comment near top of descr.h & WARNING in descr.h struct tu_local_no_data_always_false { explicit operator bool() const noexcept { return false; } @@ -133,6 +126,8 @@ tu_local_no_data_always_false type_caster_odr_guard::translation_unit_local = []() { // Executed only once per process (e.g. when a PYBIND11_MODULE is initialized). + // Conclusively tested vi test_type_caster_odr_guard_1, test_type_caster_odr_guard_2: + // those tests will fail if the sloc here is not working as intended (TU-local). type_caster_odr_guard_impl(typeid(IntrinsicType), TypeCasterType::name.sloc, PYBIND11_DETAIL_TYPE_CASTER_ODR_GUARD_THROW_DISABLED); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ea1d9799f..0d35f4a35 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -467,6 +467,8 @@ foreach(target ${test_targets}) target_compile_options(${target} PRIVATE /utf-8) endif() + target_compile_definitions(${target} PRIVATE -DPYBIND11_TYPE_CASTER_ODR_GUARD_ON_IF_AVAILABLE) + if(EIGEN3_FOUND) target_link_libraries(${target} PRIVATE Eigen3::Eigen) target_compile_definitions(${target} PRIVATE -DPYBIND11_TEST_EIGEN) diff --git a/tests/test_descr_src_loc.cpp b/tests/test_descr_src_loc.cpp index 2ee065071..98cb7affe 100644 --- a/tests/test_descr_src_loc.cpp +++ b/tests/test_descr_src_loc.cpp @@ -4,10 +4,10 @@ #include "pybind11_tests.h" -#if !defined(PYBIND11_CPP17) +// This test actually works with almost all C++17 compilers, but is currently +// only needed (and tested) for type_caster_odr_guard.h, for simplicity. -// C++17 is required for the static constexpr inline definitions. Adapting -// this unit test to older compilers is more trouble than it is worth. +#if !defined(PYBIND11_TYPE_CASTER_ODR_GUARD_ON) TEST_SUBMODULE(descr_src_loc, m) { m.attr("block_descr_offset") = py::none(); } @@ -138,4 +138,4 @@ TEST_SUBMODULE(descr_src_loc, m) { ATTR_BLKC(block_int_to_str, c2) } -#endif // PYBIND11_CPP17 +#endif // PYBIND11_TYPE_CASTER_ODR_GUARD_ON diff --git a/tests/test_descr_src_loc.py b/tests/test_descr_src_loc.py index e477cb811..a31dae745 100644 --- a/tests/test_descr_src_loc.py +++ b/tests/test_descr_src_loc.py @@ -40,9 +40,7 @@ else: ) -@pytest.mark.skipif( - m.block_descr_offset is None, reason="C++17 is required for this unit test." -) +@pytest.mark.skipif(m.block_descr_offset is None, reason="Not enabled.") @pytest.mark.parametrize("block_name, expected_text_line", block_parametrize) def test_block(block_name, expected_text_line): offset = getattr(m, f"{block_name}_offset")