From 55105fbeafe4f046414de92a6986f9a56ecfca71 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 2 Nov 2023 08:21:13 -0700 Subject: [PATCH] [smart_holder] Bug fix: `std::unique_ptr` deleter needs to be copied. (#4850) * Store `std::function` del_fun; in `guarded_delete` * Specialize the simple common case. Using a `union` is complicated: https://en.cppreference.com/w/cpp/language/union > If members of a union are classes with user-defined constructors and destructors, to switch the active member, explicit destructor and placement new are generally needed: Using `std::variant` increases compile-time overhead. It is currently unclear how much these effects matter in practice: optimization left for later. * Add one test case (more later). * Add `const` to resolve clang-tidy error. ``` -- The CXX compiler identification is Clang 15.0.7 /usr/bin/cmake -E __run_co_compile --tidy="/usr/bin/clang-tidy;--use-color;--warnings-as-errors=*;--extra-arg-before=--driver-mode=g++" --source=/__w/pybind11/pybind11/tests/test_class_sh_inheritance.cpp -- /usr/bin/c++ -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_EIGEN -Dpybind11_tests_EXPORTS -I/__w/pybind11/pybind11/include -isystem /usr/include/python3.9 -isystem /__w/pybind11/pybind11/build/_deps/eigen-src -Os -DNDEBUG -fPIC -fvisibility=hidden -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -flto=thin -std=c++17 -o CMakeFiles/pybind11_tests.dir/test_class_sh_inheritance.cpp.o -c /__w/pybind11/pybind11/tests/test_class_sh_inheritance.cpp /__w/pybind11/pybind11/tests/pure_cpp/smart_holder_poc_test.cpp:264:30: error: pointer parameter 'raw_ptr' can be pointer to const [readability-non-const-parameter,-warnings-as-errors] new int(19), [](int *raw_ptr) { delete raw_ptr; }); ^ const ``` * Introduce `struct custom_deleter` to ensure the deleter is moved as desired (the lambda function only captures a reference, which can become dangling). * Resolve helpful clang-tidy errors. ``` /usr/bin/cmake -E __run_co_compile --tidy="/usr/bin/clang-tidy;--use-color;--warnings-as-errors=*;--extra-arg-before=--driver-mode=g++" --source=/__w/pybind11/pybind11/tests/test_class.cpp -- /usr/bin/c++ -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_EIGEN -Dpybind11_tests_EXPORTS -I/__w/pybind11/pybind11/include -isystem /usr/include/python3.9 -isystem /__w/pybind11/pybind11/build/_deps/eigen-src -Os -DNDEBUG -fPIC -fvisibility=hidden -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -flto=thin -std=c++17 -o CMakeFiles/pybind11_tests.dir/test_class.cpp.o -c /__w/pybind11/pybind11/tests/test_class.cpp /__w/pybind11/pybind11/include/pybind11/detail/smart_holder_poc.h:114:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] custom_deleter(D &&deleter) : deleter{std::move(deleter)} {} ^ explicit /__w/pybind11/pybind11/include/pybind11/detail/smart_holder_poc.h:120:76: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors] return guarded_delete(std::function(custom_deleter(std::move(uqp_del))), ^~~~~~~~~ std::forward ``` * Workaround for gcc 4.8.5, clang 3.6 * Transfer reduced test here. Reduced from a PyCLIF use case in the wild by @wangxf123456 (internal change cl/565476030). * Add missing include (clangd Include Cleaner) * Change `std::move` to `std::forward` as suggested by @iwanders. * Add missing includes (clangd Include Cleaner) * Use new `PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP` to exclude `smart_holder::as_unique_ptr` method from production code. * Systematically add `PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP` to mark code that is not used from production code. Add comment to explain. * Very simple experiment related to https://github.com/pybind/pybind11/pull/4850#issuecomment-1789780676 Does the `PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP` define have anything to do with it? * Revert "Very simple experiment related to https://github.com/pybind/pybind11/pull/4850#issuecomment-1789780676" This reverts commit fe59369f408d354edef16e3d40e2f90d9c2a9ba8. --- include/pybind11/detail/smart_holder_poc.h | 53 +++++++++++++++---- tests/CMakeLists.txt | 1 + tests/pure_cpp/smart_holder_poc_test.cpp | 24 +++++++++ ...est_class_sh_unique_ptr_custom_deleter.cpp | 40 ++++++++++++++ ...test_class_sh_unique_ptr_custom_deleter.py | 6 +++ 5 files changed, 114 insertions(+), 10 deletions(-) create mode 100644 tests/test_class_sh_unique_ptr_custom_deleter.cpp create mode 100644 tests/test_class_sh_unique_ptr_custom_deleter.py diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index e169f2f26b..0f71586f7e 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -44,15 +44,27 @@ High-level aspects: * The `void_cast_raw_ptr` option is needed to make the `smart_holder` `vptr` member invisible to the `shared_from_this` mechanism, in case the lifetime of a `PyObject` is tied to the pointee. + +* Regarding `PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP` below: + This define serves as a marker for code that is NOT used + from smart_holder_type_casters.h, but is exercised only from + tests/pure_cpp/smart_holder_poc_test.cpp. The marked code was useful + mainly for bootstrapping the smart_holder work. At this stage, with + smart_holder_type_casters.h in production use (at Google) since around + February 2021, it could be moved from here to tests/pure_cpp/ (help welcome). + It will probably be best in most cases to add tests for new functionality + under test/test_class_sh_*. */ #pragma once +#include #include #include #include #include #include +#include // pybindit = Python Bindings Innovation Track. // Currently not in pybind11 namespace to signal that this POC does not depend @@ -68,14 +80,23 @@ static constexpr bool type_has_shared_from_this(const std::enable_shared_from_th } struct guarded_delete { - std::weak_ptr released_ptr; // Trick to keep the smart_holder memory footprint small. - void (*del_ptr)(void *); + std::weak_ptr released_ptr; // Trick to keep the smart_holder memory footprint small. + std::function del_fun; // Rare case. + void (*del_ptr)(void *); // Common case. + bool use_del_fun; bool armed_flag; + guarded_delete(std::function &&del_fun, bool armed_flag) + : del_fun{std::move(del_fun)}, del_ptr{nullptr}, use_del_fun{true}, + armed_flag{armed_flag} {} guarded_delete(void (*del_ptr)(void *), bool armed_flag) - : del_ptr{del_ptr}, armed_flag{armed_flag} {} + : del_ptr{del_ptr}, use_del_fun{false}, armed_flag{armed_flag} {} void operator()(void *raw_ptr) const { if (armed_flag) { - (*del_ptr)(raw_ptr); + if (use_del_fun) { + del_fun(raw_ptr); + } else { + del_ptr(raw_ptr); + } } } }; @@ -99,13 +120,16 @@ guarded_delete make_guarded_builtin_delete(bool armed_flag) { } template -inline void custom_delete(void *raw_ptr) { - D()(static_cast(raw_ptr)); -} +struct custom_deleter { + D deleter; + explicit custom_deleter(D &&deleter) : deleter{std::forward(deleter)} {} + void operator()(void *raw_ptr) { deleter(static_cast(raw_ptr)); } +}; template -guarded_delete make_guarded_custom_deleter(bool armed_flag) { - return guarded_delete(custom_delete, armed_flag); +guarded_delete make_guarded_custom_deleter(D &&uqp_del, bool armed_flag) { + return guarded_delete( + std::function(custom_deleter(std::forward(uqp_del))), armed_flag); } template @@ -232,6 +256,7 @@ struct smart_holder { return static_cast(vptr.get()); } +#ifdef PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP // See comment near top. template T &as_lvalue_ref() const { static const char *context = "as_lvalue_ref"; @@ -239,7 +264,9 @@ struct smart_holder { ensure_has_pointee(context); return *as_raw_ptr_unowned(); } +#endif +#ifdef PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP // See comment near top. template T &&as_rvalue_ref() const { static const char *context = "as_rvalue_ref"; @@ -247,6 +274,7 @@ struct smart_holder { ensure_has_pointee(context); return std::move(*as_raw_ptr_unowned()); } +#endif template static smart_holder from_raw_ptr_take_ownership(T *raw_ptr, bool void_cast_raw_ptr = false) { @@ -291,6 +319,7 @@ struct smart_holder { release_disowned(); } +#ifdef PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP // See comment near top. template T *as_raw_ptr_release_ownership(const char *context = "as_raw_ptr_release_ownership") { ensure_can_release_ownership(context); @@ -298,6 +327,7 @@ struct smart_holder { release_ownership(); return raw_ptr; } +#endif template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, @@ -309,7 +339,7 @@ struct smart_holder { if (hld.vptr_is_using_builtin_delete) { gd = make_guarded_builtin_delete(true); } else { - gd = make_guarded_custom_deleter(true); + gd = make_guarded_custom_deleter(std::move(unq_ptr.get_deleter()), true); } if (void_ptr != nullptr) { hld.vptr.reset(void_ptr, std::move(gd)); @@ -321,6 +351,7 @@ struct smart_holder { return hld; } +#ifdef PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP // See comment near top. template > std::unique_ptr as_unique_ptr() { static const char *context = "as_unique_ptr"; @@ -328,8 +359,10 @@ struct smart_holder { ensure_use_count_1(context); T *raw_ptr = as_raw_ptr_unowned(); release_ownership(); + // KNOWN DEFECT (see PR #4850): Does not copy the deleter. return std::unique_ptr(raw_ptr); } +#endif template static smart_holder from_shared_ptr(std::shared_ptr shd_ptr) { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 4bdd669ee1..e8cff2ead5 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -134,6 +134,7 @@ set(PYBIND11_TEST_FILES test_class_sh_trampoline_shared_from_this test_class_sh_trampoline_shared_ptr_cpp_arg test_class_sh_trampoline_unique_ptr + test_class_sh_unique_ptr_custom_deleter test_class_sh_unique_ptr_member test_class_sh_virtual_py_cpp_mix test_class_sh_void_ptr_capsule diff --git a/tests/pure_cpp/smart_holder_poc_test.cpp b/tests/pure_cpp/smart_holder_poc_test.cpp index b0823552f8..f820b9bf68 100644 --- a/tests/pure_cpp/smart_holder_poc_test.cpp +++ b/tests/pure_cpp/smart_holder_poc_test.cpp @@ -1,5 +1,12 @@ +#define PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP + #include "pybind11/detail/smart_holder_poc.h" +#include +#include +#include +#include + // Catch uses _ internally, which breaks gettext style defines #ifdef _ # undef _ @@ -21,6 +28,15 @@ struct movable_int { template struct functor_builtin_delete { void operator()(T *ptr) { delete ptr; } +#if (defined(__GNUC__) && __GNUC__ == 4 && __GNUC_MINOR__ == 8) \ + || (defined(__clang_major__) && __clang_major__ == 3 && __clang_minor__ == 6) + // Workaround for these errors: + // gcc 4.8.5: too many initializers for 'helpers::functor_builtin_delete' + // clang 3.6: excess elements in struct initializer + functor_builtin_delete() = default; + functor_builtin_delete(const functor_builtin_delete &) {} + functor_builtin_delete(functor_builtin_delete &&) {} +#endif }; template @@ -259,6 +275,14 @@ TEST_CASE("from_unique_ptr_with_deleter+as_lvalue_ref", "[S]") { REQUIRE(hld.as_lvalue_ref() == 19); } +TEST_CASE("from_unique_ptr_with_std_function_deleter+as_lvalue_ref", "[S]") { + std::unique_ptr> orig_owner( + new int(19), [](const int *raw_ptr) { delete raw_ptr; }); + auto hld = smart_holder::from_unique_ptr(std::move(orig_owner)); + REQUIRE(orig_owner.get() == nullptr); + REQUIRE(hld.as_lvalue_ref() == 19); +} + TEST_CASE("from_unique_ptr_with_deleter+as_raw_ptr_release_ownership", "[E]") { std::unique_ptr> orig_owner(new int(19)); auto hld = smart_holder::from_unique_ptr(std::move(orig_owner)); diff --git a/tests/test_class_sh_unique_ptr_custom_deleter.cpp b/tests/test_class_sh_unique_ptr_custom_deleter.cpp new file mode 100644 index 0000000000..070a10e0fb --- /dev/null +++ b/tests/test_class_sh_unique_ptr_custom_deleter.cpp @@ -0,0 +1,40 @@ +#include + +#include "pybind11_tests.h" + +#include + +namespace pybind11_tests { +namespace class_sh_unique_ptr_custom_deleter { + +// Reduced from a PyCLIF use case in the wild by @wangxf123456. +class Pet { +public: + using Ptr = std::unique_ptr>; + + std::string name; + + static Ptr New(const std::string &name) { + return Ptr(new Pet(name), std::default_delete()); + } + +private: + explicit Pet(const std::string &name) : name(name) {} +}; + +} // namespace class_sh_unique_ptr_custom_deleter +} // namespace pybind11_tests + +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_unique_ptr_custom_deleter::Pet) + +namespace pybind11_tests { +namespace class_sh_unique_ptr_custom_deleter { + +TEST_SUBMODULE(class_sh_unique_ptr_custom_deleter, m) { + py::classh(m, "Pet").def_readwrite("name", &Pet::name); + + m.def("create", &Pet::New); +} + +} // namespace class_sh_unique_ptr_custom_deleter +} // namespace pybind11_tests diff --git a/tests/test_class_sh_unique_ptr_custom_deleter.py b/tests/test_class_sh_unique_ptr_custom_deleter.py new file mode 100644 index 0000000000..d9cf1d4508 --- /dev/null +++ b/tests/test_class_sh_unique_ptr_custom_deleter.py @@ -0,0 +1,6 @@ +from pybind11_tests import class_sh_unique_ptr_custom_deleter as m + + +def test_create(): + pet = m.create("abc") + assert pet.name == "abc"