Skip to content

Commit

Permalink
[smart_holder] type_caster ODR guard (#4022)
Browse files Browse the repository at this point in the history
* 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<T>::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<T>`, to be able to undo the 2 changes from `load_type` to `load_type<T>`. 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<mrc_ns::type_mrc>: 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 31e8ac5.

* 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.
  • Loading branch information
Ralf W. Grosse-Kunstleve authored Jul 21, 2022
1 parent 0ec9e31 commit c557f9a
Show file tree
Hide file tree
Showing 12 changed files with 708 additions and 29 deletions.
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 16 additions & 3 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -44,8 +45,20 @@ class type_caster_for_class_ : public type_caster_base<T> {};
template <typename type, typename SFINAE = void>
class type_caster : public type_caster_for_class_<type> {};

#if defined(PYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD)

template <typename type>
using make_caster_for_intrinsic = type_caster_odr_guard<type, type_caster<type>>;

#else

template <typename type>
using make_caster = type_caster<intrinsic_t<type>>;
using make_caster_for_intrinsic = type_caster<type>;

#endif

template <typename type>
using make_caster = make_caster_for_intrinsic<intrinsic_t<type>>;

template <typename T>
struct type_uses_smart_holder_type_caster {
Expand Down Expand Up @@ -1035,8 +1048,8 @@ struct return_value_policy_override<
};

// Basic python -> C++ casting; throws if casting fails
template <typename T, typename SFINAE>
type_caster<T, SFINAE> &load_type(type_caster<T, SFINAE> &conv, const handle &handle) {
template <typename T>
make_caster_for_intrinsic<T> &load_type(make_caster_for_intrinsic<T> &conv, const handle &handle) {
static_assert(!detail::is_pyobject<T>::value,
"Internal error: type_caster should only be used for C++ types");
if (!conv.load(handle, true)) {
Expand Down
153 changes: 127 additions & 26 deletions include/pybind11/detail/descr.h
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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 <size_t N, typename... Ts>
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<N>()) {}
constexpr descr(char const (&s)[N + 1], src_loc sloc = src_loc::here())
: descr(s, make_index_sequence<N>(), sloc) {}

template <size_t... Is>
constexpr descr(char const (&s)[N + 1], index_sequence<Is...>) : text{s[Is]..., '\0'} {}
constexpr descr(char const (&s)[N + 1], index_sequence<Is...>, src_loc sloc = src_loc::here())
: text{s[Is]..., '\0'}, sloc(sloc) {}

template <typename... Chars>
// NOLINTNEXTLINE(google-explicit-constructor)
constexpr descr(char c, Chars... cs) : text{c, static_cast<char>(cs)..., '\0'} {}
constexpr descr(src_loc sloc, char c, Chars... cs)
: text{c, static_cast<char>(cs)..., '\0'}, sloc(sloc) {}

static constexpr std::array<const std::type_info *, sizeof...(Ts) + 1> types() {
return {{&typeid(Ts)..., nullptr}};
Expand All @@ -47,7 +131,8 @@ constexpr descr<N1 + N2, Ts1..., Ts2...> plus_impl(const descr<N1, Ts1...> &a,
index_sequence<Is1...>,
index_sequence<Is2...>) {
PYBIND11_WORKAROUND_INCORRECT_MSVC_C4100(b);
return {a.text[Is1]..., b.text[Is2]...};
return descr<N1 + N2, Ts1..., Ts2...>{
a.sloc.if_known_or(b.sloc), a.text[Is1]..., b.text[Is2]...};
}

template <size_t N1, size_t N2, typename... Ts1, typename... Ts2>
Expand All @@ -57,27 +142,33 @@ constexpr descr<N1 + N2, Ts1..., Ts2...> operator+(const descr<N1, Ts1...> &a,
}

template <size_t N>
constexpr descr<N - 1> const_name(char const (&text)[N]) {
return descr<N - 1>(text);
constexpr descr<N - 1> const_name(char const (&text)[N], src_loc sloc = src_loc::here()) {
return descr<N - 1>(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 <size_t Rem, size_t... Digits>
struct int_to_str : int_to_str<Rem / 10, Rem % 10, Digits...> {};
template <size_t... Digits>
struct int_to_str<0, Digits...> {
// WARNING: This only works with C++17 or higher.
static constexpr auto digits = descr<sizeof...(Digits)>(('0' + Digits)...);
// src_loc not tracked (not needed in this situation, at least at the moment).
static constexpr auto digits
= descr<sizeof...(Digits)>(src_loc{nullptr, 0}, ('0' + Digits)...);
};

// Ternary description (like std::conditional)
template <bool B, size_t N1, size_t N2>
constexpr enable_if_t<B, descr<N1 - 1>> const_name(char const (&text1)[N1], char const (&)[N2]) {
return const_name(text1);
constexpr enable_if_t<B, descr<N1 - 1>>
const_name(char const (&text1)[N1], char const (&)[N2], src_loc sloc = src_loc::here()) {
return const_name(text1, sloc);
}
template <bool B, size_t N1, size_t N2>
constexpr enable_if_t<!B, descr<N2 - 1>> const_name(char const (&)[N1], char const (&text2)[N2]) {
return const_name(text2);
constexpr enable_if_t<!B, descr<N2 - 1>>
const_name(char const (&)[N1], char const (&text2)[N2], src_loc sloc = src_loc::here()) {
return const_name(text2, sloc);
}

template <bool B, typename T1, typename T2>
Expand All @@ -91,12 +182,13 @@ constexpr enable_if_t<!B, T2> const_name(const T1 &, const T2 &d) {

template <size_t Size>
auto constexpr const_name() -> remove_cv_t<decltype(int_to_str<Size / 10, Size % 10>::digits)> {
// src_loc not tracked (not needed in this situation, at least at the moment).
return int_to_str<Size / 10, Size % 10>::digits;
}

template <typename Type>
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.
Expand All @@ -106,16 +198,18 @@ constexpr descr<1, Type> const_name() {
#ifndef _
# define PYBIND11_DETAIL_UNDERSCORE_BACKWARD_COMPATIBILITY
template <size_t N>
constexpr descr<N - 1> _(char const (&text)[N]) {
return const_name<N>(text);
constexpr descr<N - 1> _(char const (&text)[N], src_loc sloc = src_loc::here()) {
return const_name<N>(text, sloc);
}
template <bool B, size_t N1, size_t N2>
constexpr enable_if_t<B, descr<N1 - 1>> _(char const (&text1)[N1], char const (&text2)[N2]) {
return const_name<B, N1, N2>(text1, text2);
constexpr enable_if_t<B, descr<N1 - 1>>
_(char const (&text1)[N1], char const (&text2)[N2], src_loc sloc = src_loc::here()) {
return const_name<B, N1, N2>(text1, text2, sloc);
}
template <bool B, size_t N1, size_t N2>
constexpr enable_if_t<!B, descr<N2 - 1>> _(char const (&text1)[N1], char const (&text2)[N2]) {
return const_name<B, N1, N2>(text1, text2);
constexpr enable_if_t<!B, descr<N2 - 1>>
_(char const (&text1)[N1], char const (&text2)[N2], src_loc sloc = src_loc::here()) {
return const_name<B, N1, N2>(text1, text2, sloc);
}
template <bool B, typename T1, typename T2>
constexpr enable_if_t<B, T1> _(const T1 &d1, const T2 &d2) {
Expand All @@ -128,15 +222,16 @@ constexpr enable_if_t<!B, T2> _(const T1 &d1, const T2 &d2) {

template <size_t Size>
auto constexpr _() -> remove_cv_t<decltype(int_to_str<Size / 10, Size % 10>::digits)> {
// src_loc not tracked (not needed in this situation, at least at the moment).
return const_name<Size>();
}
template <typename Type>
constexpr descr<1, Type> _() {
return const_name<Type>();
constexpr descr<1, Type> _(src_loc sloc = src_loc::here()) {
return const_name<Type>(sloc);
}
#endif // #ifndef _

constexpr descr<0> concat() { return {}; }
constexpr descr<0> concat(src_loc sloc = src_loc::here()) { return descr<0>{sloc}; }

template <size_t N, typename... Ts>
constexpr descr<N, Ts...> concat(const descr<N, Ts...> &descr) {
Expand All @@ -146,13 +241,19 @@ constexpr descr<N, Ts...> concat(const descr<N, Ts...> &descr) {
template <size_t N, typename... Ts, typename... Args>
constexpr auto concat(const descr<N, Ts...> &d, const Args &...args)
-> decltype(std::declval<descr<N + 2, Ts...>>() + 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 <size_t N, typename... Ts>
constexpr descr<N + 2, Ts...> type_descr(const descr<N, Ts...> &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)
Loading

0 comments on commit c557f9a

Please sign in to comment.