From c557f9a3ad49988ec77e8a2be1dd6f6bc5013a9d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 21 Jul 2022 06:38:21 -0700 Subject: [PATCH] [smart_holder] `type_caster` ODR guard (#4022) * Insert type_caster_odr_guard<> (an empty struct to start with). * Add odr_guard_registry() used in type_caster_odr_guard() default constructor. * Add minimal_real_caster (from PR #3862) to test_async, test_buffers * VERY MESSY SNAPSHOT of WIP, this was the starting point for cl/454658864, which has more changes on top. * Restore original test_async, test_buffers from current smart_holder HEAD * Copy from cl/454991845 snapshot Jun 14, 5:08 PM * Cleanup of tests. Systematically insert `if (make_caster::translation_unit_local) {` * Small simplification of odr_guard_impl() * WIP * Add PYBIND11_SOURCE_FILE_LINE macro. * Replace PYBIND11_TYPE_CASTER_UNIQUE_IDENTIFIER with PYBIND11_TYPE_CASTER_SOURCE_FILE_LINE, baked into PYBIND11_TYPE_CASTER macro. * Add more PYBIND11_DETAIL_TYPE_CASTER_ACCESS_TRANSLATION_UNIT_LOCAL; resolves "unused" warning when compiling test_custom_type_casters.cpp * load_type fixes & follow-on cleanup * Strip ./ from source_file_line * Add new tests to CMakeLists.txt, disable PYBIND11_WERROR * Replace C++17 syntax. Compiles with Debian clang 13 C++11 mode, but fails 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. * Show C++ version along with ODR VIOLATION DETECTED message. * Add source_file_line_basename() * Introduce PYBIND11_TYPE_CASTER_ODR_GUARD_ON (but not set automatically). * Minor cleanup. * Set PYBIND11_TYPE_CASTER_ODR_GUARD_ON automatically. * Resolve clang-tidy error. * Compatibility with old compilers. * Fix off-by-one in source_file_line_basename() * Report PYBIND11_INTERNALS_ID & C++ Version from pytest_configure() * Restore use of PYBIND11_WERROR * Move cpp_version_in_use() from cast.h to pybind11_tests.cpp * define PYBIND11_DETAIL_ODR_GUARD_IMPL_THROW_DISABLED true in test_odr_guard_1,2.cpp * IWYU cleanup of detail/type_caster_odr_guard.h * Replace `throw err;` to resolve clang-tidy error. * Add new header filename to CMakeLists.txt, test_files.py * Experiment: Try any C++17 compiler. * Fix ifdef for pragma GCC diagnostic. * type_caster_odr_guard_impl() cleanup * Move type_caster_odr_guard to type_caster_odr_guard.h * Rename test_odr_guard* to test_type_caster_odr_guard* * Remove comments that are (now) more distracting than helpful. * Mark tu_local_no_data_always_false operator bool as explicit (clang-tidy). See also: https://stackoverflow.com/questions/39995573/when-can-i-use-explicit-operator-bool-without-a-cast * New PYBIND11_TYPE_CASTER_ODR_GUARD_STRICT option (current on by default). * Add test_type_caster_odr_registry_values(), test_type_caster_odr_violation_detected_counter() * Report UNEXPECTED: test_type_caster_odr_guard_2.cpp prevailed (but do not fail). * Apply clang-tidy suggestion. * Attempt to handle valgrind behavior. * Another attempt to handle valgrind behavior. * Yet another attempt to handle valgrind behavior. * Trying a new direction: show compiler info & std for UNEXPECTED: type_caster_odr_violation_detected_count() == 0 * compiler_info MSVC fix. num_violations == 0 condition. * assert pybind11_tests.compiler_info is not None * Introduce `make_caster_intrinsic`, to be able to undo the 2 changes from `load_type` to `load_type`. This is to avoid breaking 2 `pybind11::detail::load_type()` calls found in the wild (Google global testing). One of the breakages in the wild was: https://github.com/google/tensorstore/blob/0f0f6007670a3588093acd9df77cce423e0de805/python/tensorstore/subscript_method.h#L61 * Add test for stl.h / stl_bind.h mix. Manually verified that the ODR guard detects the ODR violation: ``` C++ Info: Debian Clang 13.0.1 C++17 __pybind11_internals_v4_clang_libstdcpp_cxxabi1002_sh_def__ =========================================================== test session starts ============================================================ platform linux -- Python 3.9.12, pytest-6.2.3, py-1.10.0, pluggy-0.13.1 -- /usr/bin/python3 ... ================================================================= FAILURES ================================================================= _____________________________________________ test_type_caster_odr_violation_detected_counter ______________________________________________ def test_type_caster_odr_violation_detected_counter(): ... else: > assert num_violations == 1 E assert 2 == 1 E +2 E -1 num_violations = 2 test_type_caster_odr_guard_1.py:51: AssertionError ========================================================= short test summary info ========================================================== FAILED test_type_caster_odr_guard_1.py::test_type_caster_odr_violation_detected_counter - assert 2 == 1 ======================================================= 1 failed, 5 passed in 0.08s ======================================================== ``` * Eliminate need for `PYBIND11_DETAIL_TYPE_CASTER_ACCESS_TRANSLATION_UNIT_LOCAL` macro. Copying code first developed by @amauryfa. I tried this at an earlier stage, but by itself this was insufficient. In the meantime I added in the TU-local mechanisms: trying again. Passes local testing: ``` DISABLED std::system_error: ODR VIOLATION DETECTED: pybind11::detail::type_caster: SourceLocation1="/usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_odr_guard_1.cpp:18", SourceLocation2="/usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_odr_guard_2.cpp:19" C++ Info: Debian Clang 13.0.1 C++17 __pybind11_internals_v4_clang_libstdcpp_cxxabi1002_sh_def__ =========================================================== test session starts ============================================================ platform linux -- Python 3.9.12, pytest-6.2.3, py-1.10.0, pluggy-0.13.1 -- /usr/bin/python3 cachedir: .pytest_cache rootdir: /usr/local/google/home/rwgk/forked/pybind11/tests, configfile: pytest.ini collected 6 items test_type_caster_odr_guard_1.py::test_type_mrc_to_python PASSED test_type_caster_odr_guard_1.py::test_type_mrc_from_python PASSED test_type_caster_odr_guard_1.py::test_type_caster_odr_registry_values PASSED test_type_caster_odr_guard_1.py::test_type_caster_odr_violation_detected_counter PASSED test_type_caster_odr_guard_2.py::test_type_mrc_to_python PASSED test_type_caster_odr_guard_2.py::test_type_mrc_from_python PASSED ============================================================ 6 passed in 0.01s ============================================================= ``` * tu_local_descr with src_loc experiment * clang-tidy suggested fixes * Use source_file_line_from_sloc in type_caster_odr_guard_registry * Disable type_caster ODR guard for __INTEL_COMPILER (see comment). Also turn off printf. * Add missing include (discovered via google-internal testing). * Work `scr_loc` into `descr` * Use `TypeCasterType::name.sloc` instead of `source_file_line.sloc` Manual re-verification: ``` +++ b/tests/test_type_caster_odr_guard_2.cpp - // m.def("pass_vector_type_mrc", mrc_ns::pass_vector_type_mrc); + m.def("pass_vector_type_mrc", mrc_ns::pass_vector_type_mrc); ``` ``` > assert num_violations == 1 E assert 2 == 1 num_violations = 2 test_type_caster_odr_guard_1.py:51: AssertionError ``` * Fix small oversight (src_loc::here() -> src_loc{nullptr, 0}). * Remove PYBIND11_DETAIL_TYPE_CASTER_ACCESS_TRANSLATION_UNIT_LOCAL macro completely. * Remove PYBIND11_TYPE_CASTER_SOURCE_FILE_LINE macro completely. Some small extra cleanup. * Minor tweaks looking at the PR with a fresh eye. * src_loc comments * Add new test_descr_src_loc & and fix descr.h `concat()` `src_loc` bug discovered while working on the test. * Some more work on source code comments. * Fully document the ODR violations in the ODR guard itself and introduce `PYBIND11_TYPE_CASTER_ODR_GUARD_ON_IF_AVAILABLE` * Update comment (incl. mention of deadsnakes known to not work as intended). * Use no-destructor idiom for type_caster_odr_guard_registry, as suggested by @laramiel * Fix clang-tidy error: 'auto reg' can be declared as 'auto *reg' [readability-qualified-auto,-warnings-as-errors] * WIP * Revert "WIP" (tu_local_no_data_always_false_base experiment). This reverts commit 31e8ac562f91a2e3d480a82feaa76bf2142c9ce7. * Change `PYBIND11_TYPE_CASTER_ODR_GUARD_ON` to `PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD`, based on a suggestion by @rainwoodman * Improved `#if` determining `PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD`, based on suggestion by @laramiel * Make `descr::sloc` `const`, as suggested by @rainwoodman * Rename macro to `PYBIND11_DETAIL_TYPE_CASTER_ODR_GUARD_IMPL_DEBUG`, as suggested by @laramiel * Tweak comments some more (add "white hat hacker" analogy). * Bring back `PYBIND11_CPP17` in determining `PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD`, to hopefully resolve most if not all of the many CI failures (89 failing, 32 successful: https://github.com/pybind/pybind11/runs/7430295771). * Try another workaround for `__has_builtin`-related breakages (https://github.com/pybind/pybind11/runs/7430720321). * Remove `defined(__has_builtin)` and subconditions. * Update "known to not work" expectation in test and comment. * `pytest.skip` `num_violations == 0` only `#ifdef __NO_INLINE__` (irrespective of the compiler) * Systematically change all new `#ifdef` to `#if defined` (review suggestion). * Bring back MSVC comment that got lost while experimenting. --- CMakeLists.txt | 1 + include/pybind11/cast.h | 19 ++- include/pybind11/detail/descr.h | 153 +++++++++++++++--- .../pybind11/detail/type_caster_odr_guard.h | 142 ++++++++++++++++ tests/CMakeLists.txt | 5 + tests/extra_python_package/test_files.py | 1 + tests/test_descr_src_loc.cpp | 141 ++++++++++++++++ tests/test_descr_src_loc.py | 56 +++++++ tests/test_type_caster_odr_guard_1.cpp | 85 ++++++++++ tests/test_type_caster_odr_guard_1.py | 45 ++++++ tests/test_type_caster_odr_guard_2.cpp | 66 ++++++++ tests/test_type_caster_odr_guard_2.py | 23 +++ 12 files changed, 708 insertions(+), 29 deletions(-) create mode 100644 include/pybind11/detail/type_caster_odr_guard.h create mode 100644 tests/test_descr_src_loc.cpp create mode 100644 tests/test_descr_src_loc.py create mode 100644 tests/test_type_caster_odr_guard_1.cpp create mode 100644 tests/test_type_caster_odr_guard_1.py create mode 100644 tests/test_type_caster_odr_guard_2.cpp create mode 100644 tests/test_type_caster_odr_guard_2.py diff --git a/CMakeLists.txt b/CMakeLists.txt index a423ae442..38443da66 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -115,6 +115,7 @@ set(PYBIND11_HEADERS include/pybind11/detail/smart_holder_sfinae_hooks_only.h include/pybind11/detail/smart_holder_type_casters.h include/pybind11/detail/type_caster_base.h + include/pybind11/detail/type_caster_odr_guard.h include/pybind11/detail/typeid.h include/pybind11/attr.h include/pybind11/buffer_info.h diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index c7470aafa..4f694970f 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -14,6 +14,7 @@ #include "detail/descr.h" #include "detail/smart_holder_sfinae_hooks_only.h" #include "detail/type_caster_base.h" +#include "detail/type_caster_odr_guard.h" #include "detail/typeid.h" #include "pytypes.h" @@ -44,8 +45,20 @@ class type_caster_for_class_ : public type_caster_base {}; template class type_caster : public type_caster_for_class_ {}; +#if defined(PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD) + template -using make_caster = type_caster>; +using make_caster_for_intrinsic = type_caster_odr_guard>; + +#else + +template +using make_caster_for_intrinsic = type_caster; + +#endif + +template +using make_caster = make_caster_for_intrinsic>; template struct type_uses_smart_holder_type_caster { @@ -1035,8 +1048,8 @@ struct return_value_policy_override< }; // Basic python -> C++ casting; throws if casting fails -template -type_caster &load_type(type_caster &conv, const handle &handle) { +template +make_caster_for_intrinsic &load_type(make_caster_for_intrinsic &conv, const handle &handle) { static_assert(!detail::is_pyobject::value, "Internal error: type_caster should only be used for C++ types"); if (!conv.load(handle, true)) { diff --git a/include/pybind11/detail/descr.h b/include/pybind11/detail/descr.h index e7a5e2c14..357e45be3 100644 --- a/include/pybind11/detail/descr.h +++ b/include/pybind11/detail/descr.h @@ -1,3 +1,6 @@ +// Copyright (c) 2022 The Pybind Development Team. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. /* pybind11/detail/descr.h: Helper type for concatenating type signatures at compile time @@ -20,21 +23,102 @@ PYBIND11_NAMESPACE_BEGIN(detail) # define PYBIND11_DESCR_CONSTEXPR const #endif +// struct src_loc below is to support type_caster_odr_guard.h +// (see https://github.com/pybind/pybind11/pull/4022). +// The ODR guard creates ODR violations itself (see WARNING below & in type_caster_odr_guard.h), +// but is currently the only tool available. +// The ODR is useful to know *for sure* what is safe and what is not, but that is only a +// subset of what actually works in practice, in a specific environment. The implementation +// here exploits the gray area (similar to a white hat hacker). +// The dedicated test_type_caster_odr_guard_1, test_type_caster_odr_guard_2 pair of unit tests +// passes reliably on almost all platforms that meet the compiler requirements (C++17, C++20), +// except one (gcc 9.4.0 debug build). +// In the pybind11 unit tests we want to test the ODR guard in as many environments as possible, +// but it is NOT recommended to enable 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. +// +// * 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_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE) \ + && !defined(PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD) && defined(PYBIND11_CPP17) \ + && !defined(__INTEL_COMPILER) \ + && (!defined(_MSC_VER) || _MSC_VER >= 1920) // MSVC 2019 or newer. +# define PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD +#endif + +#if defined(PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD) + +// Not using std::source_location because: +// 1. "It is unspecified whether the copy/move constructors and the copy/move +// assignment operators of source_location are trivial and/or constexpr." +// (https://en.cppreference.com/w/cpp/utility/source_location). +// 2. A matching no-op stub is needed (below) to avoid code duplication. +struct src_loc { + const char *file; + unsigned line; + + constexpr src_loc(const char *file, unsigned line) : file(file), line(line) {} + + static constexpr src_loc here(const char *file = __builtin_FILE(), + unsigned line = __builtin_LINE()) { + return src_loc(file, line); + } + + constexpr src_loc if_known_or(const src_loc &other) const { + if (file != nullptr) { + return *this; + } + return other; + } +}; + +#else + +// No-op stub, to avoid code duplication, expected to be optimized out completely. +struct src_loc { + constexpr src_loc(const char *, unsigned) {} + + static constexpr src_loc here(const char * = nullptr, unsigned = 0) { + return src_loc(nullptr, 0); + } + + constexpr src_loc if_known_or(const src_loc &) const { return *this; } +}; + +#endif + +#if defined(PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD) +namespace { // WARNING: This creates an ODR violation in the ODR guard itself, + // but we do not have any alternative 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 */ template struct descr { char text[N + 1]{'\0'}; + const src_loc sloc; - constexpr descr() = default; + explicit constexpr descr(src_loc sloc) : sloc(sloc) {} // NOLINTNEXTLINE(google-explicit-constructor) - constexpr descr(char const (&s)[N + 1]) : descr(s, make_index_sequence()) {} + constexpr descr(char const (&s)[N + 1], src_loc sloc = src_loc::here()) + : descr(s, make_index_sequence(), sloc) {} template - constexpr descr(char const (&s)[N + 1], index_sequence) : text{s[Is]..., '\0'} {} + constexpr descr(char const (&s)[N + 1], index_sequence, src_loc sloc = src_loc::here()) + : text{s[Is]..., '\0'}, sloc(sloc) {} template // NOLINTNEXTLINE(google-explicit-constructor) - constexpr descr(char c, Chars... cs) : text{c, static_cast(cs)..., '\0'} {} + constexpr descr(src_loc sloc, char c, Chars... cs) + : text{c, static_cast(cs)..., '\0'}, sloc(sloc) {} static constexpr std::array types() { return {{&typeid(Ts)..., nullptr}}; @@ -47,7 +131,8 @@ constexpr descr plus_impl(const descr &a, index_sequence, index_sequence) { PYBIND11_WORKAROUND_INCORRECT_MSVC_C4100(b); - return {a.text[Is1]..., b.text[Is2]...}; + return descr{ + a.sloc.if_known_or(b.sloc), a.text[Is1]..., b.text[Is2]...}; } template @@ -57,27 +142,33 @@ constexpr descr operator+(const descr &a, } template -constexpr descr const_name(char const (&text)[N]) { - return descr(text); +constexpr descr const_name(char const (&text)[N], src_loc sloc = src_loc::here()) { + return descr(text, sloc); +} +constexpr descr<0> const_name(char const (&)[1], src_loc sloc = src_loc::here()) { + return descr<0>(sloc); } -constexpr descr<0> const_name(char const (&)[1]) { return {}; } template struct int_to_str : int_to_str {}; template struct int_to_str<0, Digits...> { // WARNING: This only works with C++17 or higher. - static constexpr auto digits = descr(('0' + Digits)...); + // src_loc not tracked (not needed in this situation, at least at the moment). + static constexpr auto digits + = descr(src_loc{nullptr, 0}, ('0' + Digits)...); }; // Ternary description (like std::conditional) template -constexpr enable_if_t> const_name(char const (&text1)[N1], char const (&)[N2]) { - return const_name(text1); +constexpr enable_if_t> +const_name(char const (&text1)[N1], char const (&)[N2], src_loc sloc = src_loc::here()) { + return const_name(text1, sloc); } template -constexpr enable_if_t> const_name(char const (&)[N1], char const (&text2)[N2]) { - return const_name(text2); +constexpr enable_if_t> +const_name(char const (&)[N1], char const (&text2)[N2], src_loc sloc = src_loc::here()) { + return const_name(text2, sloc); } template @@ -91,12 +182,13 @@ constexpr enable_if_t const_name(const T1 &, const T2 &d) { template auto constexpr const_name() -> remove_cv_t::digits)> { + // src_loc not tracked (not needed in this situation, at least at the moment). return int_to_str::digits; } template -constexpr descr<1, Type> const_name() { - return {'%'}; +constexpr descr<1, Type> const_name(src_loc sloc = src_loc::here()) { + return {sloc, '%'}; } // If "_" is defined as a macro, py::detail::_ cannot be provided. @@ -106,16 +198,18 @@ constexpr descr<1, Type> const_name() { #ifndef _ # define PYBIND11_DETAIL_UNDERSCORE_BACKWARD_COMPATIBILITY template -constexpr descr _(char const (&text)[N]) { - return const_name(text); +constexpr descr _(char const (&text)[N], src_loc sloc = src_loc::here()) { + return const_name(text, sloc); } template -constexpr enable_if_t> _(char const (&text1)[N1], char const (&text2)[N2]) { - return const_name(text1, text2); +constexpr enable_if_t> +_(char const (&text1)[N1], char const (&text2)[N2], src_loc sloc = src_loc::here()) { + return const_name(text1, text2, sloc); } template -constexpr enable_if_t> _(char const (&text1)[N1], char const (&text2)[N2]) { - return const_name(text1, text2); +constexpr enable_if_t> +_(char const (&text1)[N1], char const (&text2)[N2], src_loc sloc = src_loc::here()) { + return const_name(text1, text2, sloc); } template constexpr enable_if_t _(const T1 &d1, const T2 &d2) { @@ -128,15 +222,16 @@ constexpr enable_if_t _(const T1 &d1, const T2 &d2) { template auto constexpr _() -> remove_cv_t::digits)> { + // src_loc not tracked (not needed in this situation, at least at the moment). return const_name(); } template -constexpr descr<1, Type> _() { - return const_name(); +constexpr descr<1, Type> _(src_loc sloc = src_loc::here()) { + return const_name(sloc); } #endif // #ifndef _ -constexpr descr<0> concat() { return {}; } +constexpr descr<0> concat(src_loc sloc = src_loc::here()) { return descr<0>{sloc}; } template constexpr descr concat(const descr &descr) { @@ -146,13 +241,19 @@ constexpr descr concat(const descr &descr) { template constexpr auto concat(const descr &d, const Args &...args) -> decltype(std::declval>() + concat(args...)) { - return d + const_name(", ") + concat(args...); + // Ensure that src_loc of existing descr is used. + return d + const_name(", ", src_loc{nullptr, 0}) + concat(args...); } template constexpr descr type_descr(const descr &descr) { - return const_name("{") + descr + const_name("}"); + // Ensure that src_loc of existing descr is used. + return const_name("{", src_loc{nullptr, 0}) + descr + const_name("}"); } +#if defined(PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD) +} // namespace +#endif + PYBIND11_NAMESPACE_END(detail) PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/detail/type_caster_odr_guard.h b/include/pybind11/detail/type_caster_odr_guard.h new file mode 100644 index 000000000..6dda2583a --- /dev/null +++ b/include/pybind11/detail/type_caster_odr_guard.h @@ -0,0 +1,142 @@ +// Copyright (c) 2022 The Pybind Development Team. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#pragma once + +#include "descr.h" + +#if defined(PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD) + +# if !defined(PYBIND11_CPP20) && defined(__GNUC__) && !defined(__clang__) +# pragma GCC diagnostic ignored "-Wsubobject-linkage" +# endif + +# include "../pytypes.h" +# include "common.h" +# include "typeid.h" + +# include +# include +# include +# include +# include +# include +# include +# include + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +PYBIND11_NAMESPACE_BEGIN(detail) + +using type_caster_odr_guard_registry_type = std::unordered_map; + +inline type_caster_odr_guard_registry_type &type_caster_odr_guard_registry() { + // Using the no-destructor idiom (maximizes safety). + static auto *reg = new type_caster_odr_guard_registry_type(); + return *reg; +} + +inline unsigned &type_caster_odr_violation_detected_counter() { + static unsigned counter = 0; + return counter; +} + +inline std::string source_file_line_basename(const char *sfl) { + unsigned i_base = 0; + for (unsigned i = 0; sfl[i] != '\0'; i++) { + if (sfl[i] == '/' || sfl[i] == '\\') { + i_base = i + 1; + } + } + return std::string(sfl + i_base); +} + +// This macro is for cooperation with test_type_caster_odr_guard_?.cpp +# ifndef PYBIND11_DETAIL_TYPE_CASTER_ODR_GUARD_THROW_DISABLED +# define PYBIND11_DETAIL_TYPE_CASTER_ODR_GUARD_THROW_DISABLED false +# endif + +inline void type_caster_odr_guard_impl(const std::type_info &intrinsic_type_info, + const src_loc &sloc, + bool throw_disabled) { + std::string source_file_line_from_sloc + = std::string(sloc.file) + ':' + std::to_string(sloc.line); +// This macro is purely for debugging. +# if defined(PYBIND11_DETAIL_TYPE_CASTER_ODR_GUARD_IMPL_DEBUG) + // std::cout cannot be used here: static initialization could be incomplete. + 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()); + std::fflush(stdout); +# endif + auto ins = type_caster_odr_guard_registry().insert( + {std::type_index(intrinsic_type_info), source_file_line_from_sloc}); + auto reg_iter = ins.first; + auto added = ins.second; + if (!added + && source_file_line_basename(reg_iter->second.c_str()) + != source_file_line_basename(source_file_line_from_sloc.c_str())) { + std::string msg("ODR VIOLATION DETECTED: pybind11::detail::type_caster<" + + clean_type_id(intrinsic_type_info.name()) + ">: SourceLocation1=\"" + + reg_iter->second + "\", SourceLocation2=\"" + source_file_line_from_sloc + + "\""); + if (throw_disabled) { + std::fprintf(stderr, "\nDISABLED std::system_error: %s\n", msg.c_str()); + std::fflush(stderr); + type_caster_odr_violation_detected_counter()++; + } else { + throw std::system_error(std::make_error_code(std::errc::state_not_recoverable), msg); + } + } +} + +namespace { // WARNING: This creates an ODR violation in the ODR guard itself, + // but we do not have any alternative 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; } +}; + +} // namespace + +template +struct type_caster_odr_guard : TypeCasterType { + static tu_local_no_data_always_false translation_unit_local; + + type_caster_odr_guard() { + // Possibly, good optimizers will elide this `if` (and below) completely. + // It is needed only to trigger the TU-local mechanisms. + if (translation_unit_local) { + } + } + + // The original author of this function is @amauryfa + template + static handle cast(CType &&src, return_value_policy policy, handle parent, Arg &&...arg) { + if (translation_unit_local) { + } + return TypeCasterType::cast( + std::forward(src), policy, parent, std::forward(arg)...); + } +}; + +template +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); + return tu_local_no_data_always_false(); + }(); + +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) + +#endif // PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index e7aec339f..2dbbaa420 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -171,6 +171,8 @@ set(PYBIND11_TEST_FILES test_stl_binders test_tagbased_polymorphic test_thread + test_type_caster_odr_guard_1 + test_type_caster_odr_guard_2 test_union test_virtual_functions) @@ -465,6 +467,9 @@ foreach(target ${test_targets}) target_compile_options(${target} PRIVATE /utf-8) endif() + target_compile_definitions(${target} + PRIVATE -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE) + if(EIGEN3_FOUND) target_link_libraries(${target} PRIVATE Eigen3::Eigen) target_compile_definitions(${target} PRIVATE -DPYBIND11_TEST_EIGEN) diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index 8206000c9..7bcb5e59d 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -48,6 +48,7 @@ detail_headers = { "include/pybind11/detail/smart_holder_sfinae_hooks_only.h", "include/pybind11/detail/smart_holder_type_casters.h", "include/pybind11/detail/type_caster_base.h", + "include/pybind11/detail/type_caster_odr_guard.h", "include/pybind11/detail/typeid.h", } diff --git a/tests/test_descr_src_loc.cpp b/tests/test_descr_src_loc.cpp new file mode 100644 index 000000000..34bbb195d --- /dev/null +++ b/tests/test_descr_src_loc.cpp @@ -0,0 +1,141 @@ +// Copyright (c) 2022 The Pybind Development Team. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#include "pybind11_tests.h" + +// 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. + +#ifndef PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD + +TEST_SUBMODULE(descr_src_loc, m) { m.attr("block_descr_offset") = py::none(); } + +#else + +namespace pybind11_tests { +namespace descr_src_loc { + +using py::detail::const_name; +using py::detail::src_loc; + +struct block_descr { + static constexpr unsigned offset = __LINE__; + static constexpr auto c0 = py::detail::descr<0>(src_loc::here()); + static constexpr auto c1 = py::detail::descr<3>("Abc"); + static constexpr auto c2 = py::detail::descr<1>(src_loc::here(), 'D'); + static constexpr auto c3 = py::detail::descr<2>(src_loc::here(), 'E', 'f'); +}; + +struct block_const_name { + static constexpr unsigned offset = __LINE__; + static constexpr auto c0 = const_name("G"); + static constexpr auto c1 = const_name("Hi"); + static constexpr auto c2 = const_name<0>(); + static constexpr auto c3 = const_name<1>(); + static constexpr auto c4 = const_name<23>(); + static constexpr auto c5 = const_name(); + static constexpr auto c6 = const_name("J", "K"); + static constexpr auto c7 = const_name("L", "M"); +}; + +# if defined(PYBIND11_DETAIL_UNDERSCORE_BACKWARD_COMPATIBILITY) +struct block_underscore { + static constexpr unsigned offset = __LINE__; + // Using a macro to avoid copying the block_const_name code garbles the src_loc.line numbers. + static constexpr auto c0 = const_name("G"); + static constexpr auto c1 = const_name("Hi"); + static constexpr auto c2 = const_name<0>(); + static constexpr auto c3 = const_name<1>(); + static constexpr auto c4 = const_name<23>(); + static constexpr auto c5 = const_name(); + static constexpr auto c6 = const_name("J", "K"); + static constexpr auto c7 = const_name("L", "M"); +}; +# endif + +struct block_plus { + static constexpr unsigned offset = __LINE__; + static constexpr auto c0 = const_name("N") + // critical line break + const_name("O"); + static constexpr auto c1 = const_name("P", src_loc(nullptr, 0)) + // critical line break + const_name("Q"); +}; + +struct block_concat { + static constexpr unsigned offset = __LINE__; + static constexpr auto c0 = py::detail::concat(const_name("R")); + static constexpr auto c1 = py::detail::concat(const_name("S"), // critical line break + const_name("T")); + static constexpr auto c2 + = py::detail::concat(const_name("U", src_loc(nullptr, 0)), // critical line break + const_name("V")); +}; + +struct block_type_descr { + static constexpr unsigned offset = __LINE__; + static constexpr auto c0 = py::detail::type_descr(const_name("W")); +}; + +struct block_int_to_str { + static constexpr unsigned offset = __LINE__; + static constexpr auto c0 = py::detail::int_to_str<0>::digits; + static constexpr auto c1 = py::detail::int_to_str<4>::digits; + static constexpr auto c2 = py::detail::int_to_str<56>::digits; +}; + +} // namespace descr_src_loc +} // namespace pybind11_tests + +TEST_SUBMODULE(descr_src_loc, m) { + using namespace pybind11_tests::descr_src_loc; + +# define ATTR_OFFS(B) m.attr(# B "_offset") = B::offset; +# define ATTR_BLKC(B, C) \ + m.attr(#B "_" #C) = py::make_tuple(B::C.text, B::C.sloc.file, B::C.sloc.line); + + ATTR_OFFS(block_descr) + ATTR_BLKC(block_descr, c0) + ATTR_BLKC(block_descr, c1) + ATTR_BLKC(block_descr, c2) + ATTR_BLKC(block_descr, c3) + + ATTR_OFFS(block_const_name) + ATTR_BLKC(block_const_name, c0) + ATTR_BLKC(block_const_name, c1) + ATTR_BLKC(block_const_name, c2) + ATTR_BLKC(block_const_name, c3) + ATTR_BLKC(block_const_name, c4) + ATTR_BLKC(block_const_name, c5) + ATTR_BLKC(block_const_name, c6) + ATTR_BLKC(block_const_name, c7) + + ATTR_OFFS(block_underscore) + ATTR_BLKC(block_underscore, c0) + ATTR_BLKC(block_underscore, c1) + ATTR_BLKC(block_underscore, c2) + ATTR_BLKC(block_underscore, c3) + ATTR_BLKC(block_underscore, c4) + ATTR_BLKC(block_underscore, c5) + ATTR_BLKC(block_underscore, c6) + ATTR_BLKC(block_underscore, c7) + + ATTR_OFFS(block_plus) + ATTR_BLKC(block_plus, c0) + ATTR_BLKC(block_plus, c1) + + ATTR_OFFS(block_concat) + ATTR_BLKC(block_concat, c0) + ATTR_BLKC(block_concat, c1) + ATTR_BLKC(block_concat, c2) + + ATTR_OFFS(block_type_descr) + ATTR_BLKC(block_type_descr, c0) + + ATTR_OFFS(block_int_to_str) + ATTR_BLKC(block_int_to_str, c0) + ATTR_BLKC(block_int_to_str, c1) + ATTR_BLKC(block_int_to_str, c2) +} + +#endif // PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD diff --git a/tests/test_descr_src_loc.py b/tests/test_descr_src_loc.py new file mode 100644 index 000000000..a31dae745 --- /dev/null +++ b/tests/test_descr_src_loc.py @@ -0,0 +1,56 @@ +import pytest + +from pybind11_tests import descr_src_loc as m + +if m.block_descr_offset is None: + block_parametrize = (("all_blocks", None),) +else: + block_parametrize = ( + ("block_descr", (("", 1), ("Abc", 2), ("D", 3), ("Ef", 4))), + ( + "block_const_name", + ( + ("G", 1), + ("Hi", 2), + ("0", 0), + ("1", 0), + ("23", 0), + ("%", 6), + ("J", 7), + ("M", 8), + ), + ), + ( + "block_underscore", + ( + ("G", 2), + ("Hi", 3), + ("0", 0), + ("1", 0), + ("23", 0), + ("%", 7), + ("J", 8), + ("M", 9), + ), + ), + ("block_plus", (("NO", 1), ("PQ", 4))), + ("block_concat", (("R", 1), ("S, T", 2), ("U, V", 6))), + ("block_type_descr", (("{W}", 1),)), + ("block_int_to_str", (("", 0), ("4", 0), ("56", 0))), + ) + + +@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") + for ix, (expected_text, expected_line) in enumerate(expected_text_line): + text, file, line = getattr(m, f"{block_name}_c{ix}") + assert text == expected_text + if expected_line: + assert file is not None, expected_text_line + assert file.endswith("test_descr_src_loc.cpp") + assert line == offset + expected_line + else: + assert file is None + assert line == 0 diff --git a/tests/test_type_caster_odr_guard_1.cpp b/tests/test_type_caster_odr_guard_1.cpp new file mode 100644 index 000000000..6fcb11528 --- /dev/null +++ b/tests/test_type_caster_odr_guard_1.cpp @@ -0,0 +1,85 @@ +#define PYBIND11_DETAIL_TYPE_CASTER_ODR_GUARD_THROW_DISABLED true +#include "pybind11_tests.h" + +// For test of real-world issue. +#include "pybind11/stl.h" + +#include + +namespace mrc_ns { // minimal real caster + +struct type_mrc { + explicit type_mrc(int v = -9999) : value(v) {} + int value; +}; + +struct minimal_real_caster { + static constexpr auto name = py::detail::const_name(); + + static py::handle + cast(type_mrc const &src, py::return_value_policy /*policy*/, py::handle /*parent*/) { + return py::int_(src.value + 1010).release(); // ODR violation. + } + + // Maximizing simplicity. This will go terribly wrong for other arg types. + template + using cast_op_type = const type_mrc &; + + // NOLINTNEXTLINE(google-explicit-constructor) + operator type_mrc const &() { + static type_mrc obj; + obj.value = 11; // ODR violation. + return obj; + } + + bool load(py::handle src, bool /*convert*/) { + // Only accepts str, but the value is ignored. + return py::isinstance(src); + } +}; + +// Intentionally not called from Python: this test is to exercise the ODR guard, +// not stl.h or stl_bind.h. +inline void pass_vector_type_mrc(const std::vector &) {} + +} // namespace mrc_ns + +namespace pybind11 { +namespace detail { +template <> +struct type_caster : mrc_ns::minimal_real_caster {}; +} // namespace detail +} // namespace pybind11 + +TEST_SUBMODULE(type_caster_odr_guard_1, m) { + m.def("type_mrc_to_python", []() { return mrc_ns::type_mrc(101); }); + m.def("type_mrc_from_python", [](const mrc_ns::type_mrc &obj) { return obj.value + 100; }); + m.def("type_caster_odr_guard_registry_values", []() { +#if defined(PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD) + py::list values; + for (const auto ®_iter : py::detail::type_caster_odr_guard_registry()) { + values.append(py::str(reg_iter.second)); + } + return values; +#else + return py::none(); +#endif + }); + m.def("type_caster_odr_violation_detected_count", []() { +#if defined(PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD) + return py::detail::type_caster_odr_violation_detected_counter(); +#else + return py::none(); +#endif + }); + + // See comment near the bottom of test_type_caster_odr_guard_2.cpp. + m.def("pass_vector_type_mrc", mrc_ns::pass_vector_type_mrc); + + m.attr("if_defined__NO_INLINE__") = +#if defined(__NO_INLINE__) + true; +#else + false; +#endif +} diff --git a/tests/test_type_caster_odr_guard_1.py b/tests/test_type_caster_odr_guard_1.py new file mode 100644 index 000000000..2b2d8e348 --- /dev/null +++ b/tests/test_type_caster_odr_guard_1.py @@ -0,0 +1,45 @@ +import pytest + +import pybind11_tests +import pybind11_tests.type_caster_odr_guard_1 as m + + +def test_type_mrc_to_python(): + val = m.type_mrc_to_python() + if val == 101 + 2020: + pytest.skip( + "UNEXPECTED: test_type_caster_odr_guard_2.cpp prevailed (to_python)." + ) + else: + assert val == 101 + 1010 + + +def test_type_mrc_from_python(): + val = m.type_mrc_from_python("ignored") + if val == 100 + 22: + pytest.skip( + "UNEXPECTED: test_type_caster_odr_guard_2.cpp prevailed (from_python)." + ) + else: + assert val == 100 + 11 + + +def test_type_caster_odr_registry_values(): + reg_values = m.type_caster_odr_guard_registry_values() + if reg_values is None: + pytest.skip("type_caster_odr_guard_registry_values() is None") + else: + assert "test_type_caster_odr_guard_" in "\n".join(reg_values) + + +def test_type_caster_odr_violation_detected_counter(): + num_violations = m.type_caster_odr_violation_detected_count() + if num_violations is None: + pytest.skip("type_caster_odr_violation_detected_count() is None") + elif num_violations == 0 and m.if_defined__NO_INLINE__: + pytest.skip( + "type_caster_odr_violation_detected_count() == 0: %s, %s, __NO_INLINE__" + % (pybind11_tests.compiler_info, pybind11_tests.cpp_std) + ) + else: + assert num_violations == 1 diff --git a/tests/test_type_caster_odr_guard_2.cpp b/tests/test_type_caster_odr_guard_2.cpp new file mode 100644 index 000000000..e0d5054df --- /dev/null +++ b/tests/test_type_caster_odr_guard_2.cpp @@ -0,0 +1,66 @@ +#define PYBIND11_DETAIL_TYPE_CASTER_ODR_GUARD_THROW_DISABLED true +#include "pybind11_tests.h" + +// For test of real-world issue. +#include "pybind11/stl_bind.h" + +#include + +namespace mrc_ns { // minimal real caster + +struct type_mrc { + explicit type_mrc(int v = -9999) : value(v) {} + int value; +}; + +struct minimal_real_caster { + static constexpr auto name = py::detail::const_name(); + + static py::handle + cast(type_mrc const &src, py::return_value_policy /*policy*/, py::handle /*parent*/) { + return py::int_(src.value + 2020).release(); // ODR violation. + } + + // Maximizing simplicity. This will go terribly wrong for other arg types. + template + using cast_op_type = const type_mrc &; + + // NOLINTNEXTLINE(google-explicit-constructor) + operator type_mrc const &() { + static type_mrc obj; + obj.value = 22; // ODR violation. + return obj; + } + + bool load(py::handle src, bool /*convert*/) { + // Only accepts str, but the value is ignored. + return py::isinstance(src); + } +}; + +// Intentionally not called from Python: this test is to exercise the ODR guard, +// not stl.h or stl_bind.h. +inline void pass_vector_type_mrc(const std::vector &) {} + +} // namespace mrc_ns + +PYBIND11_MAKE_OPAQUE(std::vector); + +namespace pybind11 { +namespace detail { +template <> +struct type_caster : mrc_ns::minimal_real_caster {}; +} // namespace detail +} // namespace pybind11 + +TEST_SUBMODULE(type_caster_odr_guard_2, m) { + m.def("type_mrc_to_python", []() { return mrc_ns::type_mrc(202); }); + m.def("type_mrc_from_python", [](const mrc_ns::type_mrc &obj) { return obj.value + 200; }); + + // Uncomment and run test_type_caster_odr_guard_1.py to verify that the + // test_type_caster_odr_violation_detected_counter subtest fails + // (num_violations 2 instead of 1). + // Unlike the "controlled ODR violation" for the minimal_real_caster, this ODR violation is + // completely unsafe, therefore it cannot portably be exercised with predictable results. + // m.def("pass_vector_type_mrc", mrc_ns::pass_vector_type_mrc); +} diff --git a/tests/test_type_caster_odr_guard_2.py b/tests/test_type_caster_odr_guard_2.py new file mode 100644 index 000000000..b4f5ef6c4 --- /dev/null +++ b/tests/test_type_caster_odr_guard_2.py @@ -0,0 +1,23 @@ +import pytest + +import pybind11_tests.type_caster_odr_guard_2 as m + + +def test_type_mrc_to_python(): + val = m.type_mrc_to_python() + if val == 202 + 2020: + pytest.skip( + "UNEXPECTED: test_type_caster_odr_guard_2.cpp prevailed (to_python)." + ) + else: + assert val == 202 + 1010 + + +def test_type_mrc_from_python(): + val = m.type_mrc_from_python("ignored") + if val == 200 + 22: + pytest.skip( + "UNEXPECTED: test_type_caster_odr_guard_2.cpp prevailed (from_python)." + ) + else: + assert val == 200 + 11