From 9143689d78dff32aae8547e86b1870733a643288 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 7 Jun 2024 14:16:56 -0700 Subject: [PATCH 1/9] Transfer diff from pybind11k fork as-is. New tests are still missing. --- include/pybind11/cast.h | 15 +++++++++++++-- include/pybind11/pybind11.h | 3 ++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 02d9488dae..6a1631b564 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1339,13 +1339,24 @@ enable_if_t::value, T> cast_ref(object &&, // static_assert, even though if it's in dead code, so we provide a "trampoline" to pybind11::cast // that only does anything in cases where pybind11::cast is valid. template -enable_if_t::value, T> cast_safe(object &&) { +enable_if_t::value + && !detail::is_same_ignoring_cvref::value, + T> +cast_safe(object &&) { pybind11_fail("Internal error: cast_safe fallback invoked"); } template enable_if_t::value, void> cast_safe(object &&) {} template -enable_if_t, std::is_void>::value, T> +enable_if_t::value, PyObject *> +cast_safe(object &&o) { + return o.release().ptr(); +} +template +enable_if_t, + std::is_void, + detail::is_same_ignoring_cvref>::value, + T> cast_safe(object &&o) { return pybind11::cast(std::move(o)); } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 92ed573a0b..d0ec12174e 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2868,7 +2868,8 @@ function get_override(const T *this_ptr, const char *name) { = pybind11::get_override(static_cast(this), name); \ if (override) { \ auto o = override(__VA_ARGS__); \ - if (pybind11::detail::cast_is_temporary_value_reference::value) { \ + if (!pybind11::detail::is_same_ignoring_cvref::value \ + && pybind11::detail::cast_is_temporary_value_reference::value) { \ static pybind11::detail::override_caster_t caster; \ return pybind11::detail::cast_ref(std::move(o), caster); \ } \ From 98a5e3b5949a52be53b8bfb972c1c5ec66b64f21 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 7 Jun 2024 15:31:10 -0700 Subject: [PATCH 2/9] Add `PYBIND11_WARNING_DISABLE_MSVC(4127)` into `PYBIND11_OVERRIDE_IMPL` macro. --- include/pybind11/pybind11.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index d0ec12174e..0f3ce8698e 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2868,8 +2868,11 @@ function get_override(const T *this_ptr, const char *name) { = pybind11::get_override(static_cast(this), name); \ if (override) { \ auto o = override(__VA_ARGS__); \ + PYBIND11_WARNING_PUSH \ + PYBIND11_WARNING_DISABLE_MSVC(4127) \ if (!pybind11::detail::is_same_ignoring_cvref::value \ && pybind11::detail::cast_is_temporary_value_reference::value) { \ + PYBIND11_WARNING_POP \ static pybind11::detail::override_caster_t caster; \ return pybind11::detail::cast_ref(std::move(o), caster); \ } \ From 3f1ad5a44b8a127d45c893e9d7193456bab3df44 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 9 Jun 2024 12:07:42 -0700 Subject: [PATCH 3/9] Add test_trampoline_with_pyobject_ptr_return() --- tests/test_type_caster_pyobject_ptr.cpp | 35 +++++++++++++++++++++++-- tests/test_type_caster_pyobject_ptr.py | 16 +++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/tests/test_type_caster_pyobject_ptr.cpp b/tests/test_type_caster_pyobject_ptr.cpp index 8069f7dcd9..87290b5889 100644 --- a/tests/test_type_caster_pyobject_ptr.cpp +++ b/tests/test_type_caster_pyobject_ptr.cpp @@ -5,9 +5,10 @@ #include "pybind11_tests.h" #include +#include #include -namespace { +namespace test_type_caster_pyobject_ptr { std::vector make_vector_pyobject_ptr(const py::object &ValueHolder) { std::vector vec_obj; @@ -18,9 +19,33 @@ std::vector make_vector_pyobject_ptr(const py::object &ValueHolder) return vec_obj; } -} // namespace +struct WithPyObjectPtrReturn { + virtual ~WithPyObjectPtrReturn() = default; + virtual PyObject *return_pyobject_ptr() const = 0; +}; + +struct WithPyObjectPtrReturnTrampoline : WithPyObjectPtrReturn { + PyObject *return_pyobject_ptr() const { + PYBIND11_OVERRIDE_PURE(PyObject *, WithPyObjectPtrReturn, return_pyobject_ptr, + /* no arguments */); + } +}; + +std::string call_return_pyobject_ptr(const WithPyObjectPtrReturn *base_class_ptr) { + PyObject *returned_obj = base_class_ptr->return_pyobject_ptr(); + if (Py_REFCNT(returned_obj) != 1) { + py::pybind11_fail(__FILE__ ":" PYBIND11_TOSTRING(__LINE__)); + } + std::string ret_val = py::repr(returned_obj).cast(); + Py_DECREF(returned_obj); + return ret_val; +} + +} // namespace test_type_caster_pyobject_ptr TEST_SUBMODULE(type_caster_pyobject_ptr, m) { + using namespace test_type_caster_pyobject_ptr; + m.def("cast_from_pyobject_ptr", []() { PyObject *ptr = PyLong_FromLongLong(6758L); return py::cast(ptr, py::return_value_policy::take_ownership); @@ -127,4 +152,10 @@ TEST_SUBMODULE(type_caster_pyobject_ptr, m) { (void) py::cast(*ptr); } #endif + + py::class_(m, "WithPyObjectPtrReturn") + .def(py::init<>()) + .def("return_pyobject_ptr", &WithPyObjectPtrReturn::return_pyobject_ptr); + + m.def("call_return_pyobject_ptr", call_return_pyobject_ptr); } diff --git a/tests/test_type_caster_pyobject_ptr.py b/tests/test_type_caster_pyobject_ptr.py index 1f1ece2baf..f6358d0118 100644 --- a/tests/test_type_caster_pyobject_ptr.py +++ b/tests/test_type_caster_pyobject_ptr.py @@ -102,3 +102,19 @@ def test_return_list_pyobject_ptr_reference(): def test_type_caster_name_via_incompatible_function_arguments_type_error(): with pytest.raises(TypeError, match=r"1\. \(arg0: object, arg1: int\) -> None"): m.pass_pyobject_ptr_and_int(ValueHolder(101), ValueHolder(202)) + + +def test_trampoline_with_pyobject_ptr_return(): + class Drvd(m.WithPyObjectPtrReturn): + def return_pyobject_ptr(self): + return ["11", "22", "33"] + + # Basic health check: First make sure this works as expected. + d = Drvd() + assert d.return_pyobject_ptr() == ["11", "22", "33"] + + while True: + # This failed before PR #5156: AddressSanitizer: heap-use-after-free ... in Py_DECREF + d_repr = m.call_return_pyobject_ptr(d) + assert d_repr == repr(["11", "22", "33"]) + break # Comment out for manual leak checking. From d4de515341985b450fd631f0295b33f2a69ea4da Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 9 Jun 2024 15:22:36 -0700 Subject: [PATCH 4/9] Resolve clang-tidy error: use auto when initializing with a template cast to avoid duplicating the type name [modernize-use-auto,-warnings-as-errors] --- tests/test_type_caster_pyobject_ptr.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_type_caster_pyobject_ptr.cpp b/tests/test_type_caster_pyobject_ptr.cpp index 87290b5889..8e9668c734 100644 --- a/tests/test_type_caster_pyobject_ptr.cpp +++ b/tests/test_type_caster_pyobject_ptr.cpp @@ -36,7 +36,7 @@ std::string call_return_pyobject_ptr(const WithPyObjectPtrReturn *base_class_ptr if (Py_REFCNT(returned_obj) != 1) { py::pybind11_fail(__FILE__ ":" PYBIND11_TOSTRING(__LINE__)); } - std::string ret_val = py::repr(returned_obj).cast(); + auto ret_val = py::repr(returned_obj).cast(); Py_DECREF(returned_obj); return ret_val; } From ff69adc886fde27b897117faf5f5231634cc5718 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 9 Jun 2024 15:31:20 -0700 Subject: [PATCH 5/9] Disabled checking refcount when building with PyPy. --- tests/test_type_caster_pyobject_ptr.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_type_caster_pyobject_ptr.cpp b/tests/test_type_caster_pyobject_ptr.cpp index 8e9668c734..8db4cb38d8 100644 --- a/tests/test_type_caster_pyobject_ptr.cpp +++ b/tests/test_type_caster_pyobject_ptr.cpp @@ -33,9 +33,11 @@ struct WithPyObjectPtrReturnTrampoline : WithPyObjectPtrReturn { std::string call_return_pyobject_ptr(const WithPyObjectPtrReturn *base_class_ptr) { PyObject *returned_obj = base_class_ptr->return_pyobject_ptr(); +#if !defined(PYPY_VERSION) // It is not worth the trouble doing something special for PyPy. if (Py_REFCNT(returned_obj) != 1) { py::pybind11_fail(__FILE__ ":" PYBIND11_TOSTRING(__LINE__)); } +#endif auto ret_val = py::repr(returned_obj).cast(); Py_DECREF(returned_obj); return ret_val; From 6237e44f438ff4835bd6442371512bbc8ec49481 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 9 Jun 2024 15:36:08 -0700 Subject: [PATCH 6/9] Clang 3.6, 3.7, 3.9 compatibility. ``` /__w/pybind11/pybind11/tests/test_type_caster_pyobject_ptr.cpp:23:13: error: definition of implicit copy constructor for 'WithPyObjectPtrReturn' is deprecated because it has a user-declared destructor [-Werror,-Wdeprecated] virtual ~WithPyObjectPtrReturn() = default; ^ ``` --- tests/test_type_caster_pyobject_ptr.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test_type_caster_pyobject_ptr.cpp b/tests/test_type_caster_pyobject_ptr.cpp index 8db4cb38d8..a2fe8f02e3 100644 --- a/tests/test_type_caster_pyobject_ptr.cpp +++ b/tests/test_type_caster_pyobject_ptr.cpp @@ -20,6 +20,10 @@ std::vector make_vector_pyobject_ptr(const py::object &ValueHolder) } struct WithPyObjectPtrReturn { +#if defined(__clang_major__) && __clang_major__ < 4 + WithPyObjectPtrReturn() = default; + WithPyObjectPtrReturn(const WithPyObjectPtrReturn &) = default; +#endif virtual ~WithPyObjectPtrReturn() = default; virtual PyObject *return_pyobject_ptr() const = 0; }; From 42cc6b039f27b7fcb6f90b39dd544295c8f8b580 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 9 Jun 2024 15:43:29 -0700 Subject: [PATCH 7/9] Minor clean-up of production code changes. --- include/pybind11/cast.h | 4 ++-- include/pybind11/pybind11.h | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 6a1631b564..624b8ebaca 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1354,8 +1354,8 @@ cast_safe(object &&o) { } template enable_if_t, - std::is_void, - detail::is_same_ignoring_cvref>::value, + detail::is_same_ignoring_cvref, + std::is_void>::value, T> cast_safe(object &&o) { return pybind11::cast(std::move(o)); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 0f3ce8698e..0d9ce5bba1 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2870,10 +2870,9 @@ function get_override(const T *this_ptr, const char *name) { auto o = override(__VA_ARGS__); \ PYBIND11_WARNING_PUSH \ PYBIND11_WARNING_DISABLE_MSVC(4127) \ - if (!pybind11::detail::is_same_ignoring_cvref::value \ - && pybind11::detail::cast_is_temporary_value_reference::value) { \ - PYBIND11_WARNING_POP \ - static pybind11::detail::override_caster_t caster; \ + if (pybind11::detail::cast_is_temporary_value_reference::value \ + && !pybind11::detail::is_same_ignoring_cvref::value) { \ + PYBIND11_WARNING_POP static pybind11::detail::override_caster_t caster; \ return pybind11::detail::cast_ref(std::move(o), caster); \ } \ return pybind11::detail::cast_safe(std::move(o)); \ From b3154732a6b4159fd7b4ffe7f09a57fc8f957b6e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 9 Jun 2024 15:56:05 -0700 Subject: [PATCH 8/9] Add missing `override` (to resolve clang-tidy error). --- tests/test_type_caster_pyobject_ptr.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_type_caster_pyobject_ptr.cpp b/tests/test_type_caster_pyobject_ptr.cpp index a2fe8f02e3..a45c08b648 100644 --- a/tests/test_type_caster_pyobject_ptr.cpp +++ b/tests/test_type_caster_pyobject_ptr.cpp @@ -29,7 +29,7 @@ struct WithPyObjectPtrReturn { }; struct WithPyObjectPtrReturnTrampoline : WithPyObjectPtrReturn { - PyObject *return_pyobject_ptr() const { + PyObject *return_pyobject_ptr() const override { PYBIND11_OVERRIDE_PURE(PyObject *, WithPyObjectPtrReturn, return_pyobject_ptr, /* no arguments */); } From bd19ca143a073fb78532c37f9ccaee813a4518ff Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 9 Jun 2024 23:42:28 -0700 Subject: [PATCH 9/9] Move PYBIND11_WARNING_POP for a better clang-format outcome. --- include/pybind11/pybind11.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 0d9ce5bba1..b3fdb7fce3 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2872,9 +2872,10 @@ function get_override(const T *this_ptr, const char *name) { PYBIND11_WARNING_DISABLE_MSVC(4127) \ if (pybind11::detail::cast_is_temporary_value_reference::value \ && !pybind11::detail::is_same_ignoring_cvref::value) { \ - PYBIND11_WARNING_POP static pybind11::detail::override_caster_t caster; \ + static pybind11::detail::override_caster_t caster; \ return pybind11::detail::cast_ref(std::move(o), caster); \ } \ + PYBIND11_WARNING_POP \ return pybind11::detail::cast_safe(std::move(o)); \ } \ } while (false)