From 05e6e2d33bf45d9db9fc41af26e86660e33b749c Mon Sep 17 00:00:00 2001 From: Trigve Siver Date: Sat, 13 Nov 2021 11:17:57 +0100 Subject: [PATCH] override: Fix wrong caching of the overrides There was a problem when the python type, which was stored in override cache for C++ functions, was destroyed and the record wasn't removed from the override cache. Therefor, dangling pointer was stored there. Then when the memory was reused and new type was allocated at the given address and the method with the same name (as previously stored in the cache) was actually overridden in python, it would wrongly find it in the override cache for C++ functions and therefor override from python wouldn't be called. The fix is to erase the type from the override cache when the type is destroyed. --- include/pybind11/pybind11.h | 10 ++++++ tests/test_class_sh_inheritance.cpp | 26 ++++++++++++++ tests/test_class_sh_inheritance.py | 19 +++++++++++ tests/test_embed/CMakeLists.txt | 2 +- tests/test_embed/test_derived.py | 18 ++++++++++ tests/test_embed/test_interpreter.cpp | 49 +++++++++++++++++++++++++++ 6 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 tests/test_embed/test_derived.py diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 5738f7a9ce..ad622a7f32 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2093,6 +2093,16 @@ inline std::pair all_t // gets destroyed: weakref((PyObject *) type, cpp_function([type](handle wr) { get_internals().registered_types_py.erase(type); + + // Actually just `std::erase_if`, but that's only available in C++20 + auto &cache = get_internals().inactive_override_cache; + for (auto it = cache.begin(), last = cache.end(); it != last; ) { + if (it->first == reinterpret_cast(type)) + it = cache.erase(it); + else + ++it; + } + wr.dec_ref(); })).release(); } diff --git a/tests/test_class_sh_inheritance.cpp b/tests/test_class_sh_inheritance.cpp index d5b05cd8c4..785abce20a 100644 --- a/tests/test_class_sh_inheritance.cpp +++ b/tests/test_class_sh_inheritance.cpp @@ -49,6 +49,24 @@ struct drvd2 : base1, base2 { int id() const override { return 3 * base1::base_id + 4 * base2::base_id; } }; +class test_derived { + +public: + virtual int func() { return 0; } + + test_derived() = default; + ~test_derived() = default; + // Non-copyable + test_derived &operator=(test_derived const &Right) = delete; + test_derived(test_derived const &Copy) = delete; +}; + +class py_test_derived : public test_derived { + virtual int func() override { PYBIND11_OVERRIDE(int, test_derived, func); } +}; + +inline int test_override_cache(std::shared_ptr < test_derived> instance) { return instance->func(); } + // clang-format off inline drvd2 *rtrn_mptr_drvd2() { return new drvd2; } inline base1 *rtrn_mptr_drvd2_up_cast1() { return new drvd2; } @@ -69,6 +87,8 @@ PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_inheritance::base1) PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_inheritance::base2) PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_inheritance::drvd2) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_inheritance::test_derived) + namespace pybind11_tests { namespace class_sh_inheritance { @@ -99,6 +119,12 @@ TEST_SUBMODULE(class_sh_inheritance, m) { m.def("pass_cptr_base1", pass_cptr_base1); m.def("pass_cptr_base2", pass_cptr_base2); m.def("pass_cptr_drvd2", pass_cptr_drvd2); + + py::classh(m, "test_derived") + .def(py::init_alias<>()) + .def("func", &test_derived::func); + + m.def("test_override_cache", test_override_cache); } } // namespace class_sh_inheritance diff --git a/tests/test_class_sh_inheritance.py b/tests/test_class_sh_inheritance.py index 69f9131d70..0956219780 100644 --- a/tests/test_class_sh_inheritance.py +++ b/tests/test_class_sh_inheritance.py @@ -61,3 +61,22 @@ def __init__(self): assert i1 == 110 + 21 i2 = m.pass_cptr_base2(d) assert i2 == 120 + 22 + + +def test_python_override(): + def func(): + class Test(m.test_derived): + def func(self): + return 42 + + return Test() + + def func2(): + class Test(m.test_derived): + pass + + return Test() + + for _ in range(1500): + assert m.test_override_cache(func()) == 42 + assert m.test_override_cache(func2()) == 0 diff --git a/tests/test_embed/CMakeLists.txt b/tests/test_embed/CMakeLists.txt index 3b89d6e584..d1ace33f21 100644 --- a/tests/test_embed/CMakeLists.txt +++ b/tests/test_embed/CMakeLists.txt @@ -25,7 +25,7 @@ pybind11_enable_warnings(test_embed) target_link_libraries(test_embed PRIVATE pybind11::embed Catch2::Catch2 Threads::Threads) if(NOT CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_CURRENT_BINARY_DIR) - file(COPY test_interpreter.py DESTINATION "${CMAKE_CURRENT_BINARY_DIR}") + file(COPY test_interpreter.py test_derived.py DESTINATION "${CMAKE_CURRENT_BINARY_DIR}") endif() add_custom_target( diff --git a/tests/test_embed/test_derived.py b/tests/test_embed/test_derived.py new file mode 100644 index 0000000000..ad94865b71 --- /dev/null +++ b/tests/test_embed/test_derived.py @@ -0,0 +1,18 @@ +# -*- coding: utf-8 -*- + +import derived_module + + +def func(): + class Test(derived_module.test_derived): + def func(self): + return 42 + + return Test() + + +def func2(): + class Test(derived_module.test_derived): + pass + + return Test() diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index 20bcade0ac..949ebfea0a 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -1,4 +1,5 @@ #include +#include #ifdef _MSC_VER // Silence MSVC C++17 deprecation warning from Catch regarding std::uncaught_exceptions (up to catch @@ -37,6 +38,24 @@ class PyWidget final : public Widget { std::string argv0() const override { PYBIND11_OVERRIDE_PURE(std::string, Widget, argv0); } }; +class test_derived { + +public: + virtual int func() { return 0; } + + test_derived() = default; + virtual ~test_derived() = default; + // Non-copyable + test_derived &operator=(test_derived const &Right) = delete; + test_derived(test_derived const &Copy) = delete; +}; + +class py_test_derived : public test_derived { + virtual int func() override { PYBIND11_OVERRIDE(int, test_derived, func); } +}; + +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_derived) + PYBIND11_EMBEDDED_MODULE(widget_module, m) { py::class_(m, "Widget") .def(py::init()) @@ -45,6 +64,12 @@ PYBIND11_EMBEDDED_MODULE(widget_module, m) { m.def("add", [](int i, int j) { return i + j; }); } +PYBIND11_EMBEDDED_MODULE(derived_module, m) { + py::classh(m, "test_derived") + .def(py::init_alias<>()) + .def("func", &test_derived::func); +} + PYBIND11_EMBEDDED_MODULE(throw_exception, ) { throw std::runtime_error("C++ Error"); } @@ -73,6 +98,30 @@ TEST_CASE("Pass classes and data between modules defined in C++ and Python") { REQUIRE(cpp_widget.the_answer() == 42); } +TEST_CASE("Override cache") { + auto module_ = py::module_::import("test_derived"); + REQUIRE(py::hasattr(module_, "func")); + REQUIRE(py::hasattr(module_, "func2")); + + auto locals = py::dict(**module_.attr("__dict__")); + + int i = 0; + for (; i < 1500; ++i) { + std::shared_ptr p_obj; + std::shared_ptr p_obj2; + + p_obj = pybind11::cast>(locals["func"]()); + + int ret = p_obj->func(); + + REQUIRE(ret == 42); + + p_obj2 = pybind11::cast>(locals["func2"]()); + + p_obj2->func(); + } +} + TEST_CASE("Import error handling") { REQUIRE_NOTHROW(py::module_::import("widget_module")); REQUIRE_THROWS_WITH(py::module_::import("throw_exception"),