Skip to content

Commit

Permalink
[smart_holder] Bug fix: std::unique_ptr deleter needs to be copied. (
Browse files Browse the repository at this point in the history
…#4850)

* Store `std::function<void (void *)>` 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<void(void *)>(custom_deleter<T, D>(std::move(uqp_del))),
                                                                           ^~~~~~~~~
                                                                           std::forward<D>
```

* 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 #4850 (comment)

Does the `PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP` define have anything to do with it?

* Revert "Very simple experiment related to #4850 (comment)"

This reverts commit fe59369.
  • Loading branch information
Ralf W. Grosse-Kunstleve authored Nov 2, 2023
1 parent e955753 commit 55105fb
Show file tree
Hide file tree
Showing 5 changed files with 114 additions and 10 deletions.
53 changes: 43 additions & 10 deletions include/pybind11/detail/smart_holder_poc.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 <functional>
#include <memory>
#include <stdexcept>
#include <string>
#include <type_traits>
#include <typeinfo>
#include <utility>

// pybindit = Python Bindings Innovation Track.
// Currently not in pybind11 namespace to signal that this POC does not depend
Expand All @@ -68,14 +80,23 @@ static constexpr bool type_has_shared_from_this(const std::enable_shared_from_th
}

struct guarded_delete {
std::weak_ptr<void> released_ptr; // Trick to keep the smart_holder memory footprint small.
void (*del_ptr)(void *);
std::weak_ptr<void> released_ptr; // Trick to keep the smart_holder memory footprint small.
std::function<void(void *)> del_fun; // Rare case.
void (*del_ptr)(void *); // Common case.
bool use_del_fun;
bool armed_flag;
guarded_delete(std::function<void(void *)> &&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);
}
}
}
};
Expand All @@ -99,13 +120,16 @@ guarded_delete make_guarded_builtin_delete(bool armed_flag) {
}

template <typename T, typename D>
inline void custom_delete(void *raw_ptr) {
D()(static_cast<T *>(raw_ptr));
}
struct custom_deleter {
D deleter;
explicit custom_deleter(D &&deleter) : deleter{std::forward<D>(deleter)} {}
void operator()(void *raw_ptr) { deleter(static_cast<T *>(raw_ptr)); }
};

template <typename T, typename D>
guarded_delete make_guarded_custom_deleter(bool armed_flag) {
return guarded_delete(custom_delete<T, D>, armed_flag);
guarded_delete make_guarded_custom_deleter(D &&uqp_del, bool armed_flag) {
return guarded_delete(
std::function<void(void *)>(custom_deleter<T, D>(std::forward<D>(uqp_del))), armed_flag);
}

template <typename T>
Expand Down Expand Up @@ -232,21 +256,25 @@ struct smart_holder {
return static_cast<T *>(vptr.get());
}

#ifdef PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP // See comment near top.
template <typename T>
T &as_lvalue_ref() const {
static const char *context = "as_lvalue_ref";
ensure_is_populated(context);
ensure_has_pointee(context);
return *as_raw_ptr_unowned<T>();
}
#endif

#ifdef PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP // See comment near top.
template <typename T>
T &&as_rvalue_ref() const {
static const char *context = "as_rvalue_ref";
ensure_is_populated(context);
ensure_has_pointee(context);
return std::move(*as_raw_ptr_unowned<T>());
}
#endif

template <typename T>
static smart_holder from_raw_ptr_take_ownership(T *raw_ptr, bool void_cast_raw_ptr = false) {
Expand Down Expand Up @@ -291,13 +319,15 @@ struct smart_holder {
release_disowned();
}

#ifdef PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP // See comment near top.
template <typename T>
T *as_raw_ptr_release_ownership(const char *context = "as_raw_ptr_release_ownership") {
ensure_can_release_ownership(context);
T *raw_ptr = as_raw_ptr_unowned<T>();
release_ownership();
return raw_ptr;
}
#endif

template <typename T, typename D>
static smart_holder from_unique_ptr(std::unique_ptr<T, D> &&unq_ptr,
Expand All @@ -309,7 +339,7 @@ struct smart_holder {
if (hld.vptr_is_using_builtin_delete) {
gd = make_guarded_builtin_delete<T>(true);
} else {
gd = make_guarded_custom_deleter<T, D>(true);
gd = make_guarded_custom_deleter<T, D>(std::move(unq_ptr.get_deleter()), true);
}
if (void_ptr != nullptr) {
hld.vptr.reset(void_ptr, std::move(gd));
Expand All @@ -321,15 +351,18 @@ struct smart_holder {
return hld;
}

#ifdef PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP // See comment near top.
template <typename T, typename D = std::default_delete<T>>
std::unique_ptr<T, D> as_unique_ptr() {
static const char *context = "as_unique_ptr";
ensure_compatible_rtti_uqp_del<T, D>(context);
ensure_use_count_1(context);
T *raw_ptr = as_raw_ptr_unowned<T>();
release_ownership();
// KNOWN DEFECT (see PR #4850): Does not copy the deleter.
return std::unique_ptr<T, D>(raw_ptr);
}
#endif

template <typename T>
static smart_holder from_shared_ptr(std::shared_ptr<T> shd_ptr) {
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 24 additions & 0 deletions tests/pure_cpp/smart_holder_poc_test.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
#define PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP

#include "pybind11/detail/smart_holder_poc.h"

#include <functional>
#include <memory>
#include <type_traits>
#include <utility>

// Catch uses _ internally, which breaks gettext style defines
#ifdef _
# undef _
Expand All @@ -21,6 +28,15 @@ struct movable_int {
template <typename T>
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<int>'
// 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 <typename T>
Expand Down Expand Up @@ -259,6 +275,14 @@ TEST_CASE("from_unique_ptr_with_deleter+as_lvalue_ref", "[S]") {
REQUIRE(hld.as_lvalue_ref<int>() == 19);
}

TEST_CASE("from_unique_ptr_with_std_function_deleter+as_lvalue_ref", "[S]") {
std::unique_ptr<int, std::function<void(const int *)>> 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<int>() == 19);
}

TEST_CASE("from_unique_ptr_with_deleter+as_raw_ptr_release_ownership", "[E]") {
std::unique_ptr<int, helpers::functor_builtin_delete<int>> orig_owner(new int(19));
auto hld = smart_holder::from_unique_ptr(std::move(orig_owner));
Expand Down
40 changes: 40 additions & 0 deletions tests/test_class_sh_unique_ptr_custom_deleter.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
#include <pybind11/smart_holder.h>

#include "pybind11_tests.h"

#include <memory>

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<Pet, std::function<void(Pet *)>>;

std::string name;

static Ptr New(const std::string &name) {
return Ptr(new Pet(name), std::default_delete<Pet>());
}

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<Pet>(m, "Pet").def_readwrite("name", &Pet::name);

m.def("create", &Pet::New);
}

} // namespace class_sh_unique_ptr_custom_deleter
} // namespace pybind11_tests
6 changes: 6 additions & 0 deletions tests/test_class_sh_unique_ptr_custom_deleter.py
Original file line number Diff line number Diff line change
@@ -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"

0 comments on commit 55105fb

Please sign in to comment.