Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[smart_holder] Bug fix: std::unique_ptr deleter needs to be copied. #4850

Merged
merged 16 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Feel free to ignore) This line missing a semicolon trips up the syntax highlighting on github, but it seems to be in line with the other tests. Could add a do {} while(0) at the end of the macro to enforce the ; at the end of the line. But I think that requires a lot of changes in other unit tests as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Too late", unfortunately: I just looked, I'd have to change 35 files around the Google codebase, outside the pybind11 source directory. It'll set me back a whole day or two worth of effort. And there are so many other far more substantial things wishing for those days.


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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unit test doesn't actually test the deleter is ran, unless ran inside valgrind / lsan, is that intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unit test doesn't actually test the deleter is ran, unless ran inside valgrind / lsan, is that intentional?

@wangxf123456 was completely focused on reducing a test from the wild, just enough to reproduce the error message we saw.

Copy-pasting what @iwanders wrote in another comment:

my main concern is the above statement that the deleter is only handled when going into the smart holder, not when being converted back to a unique pointer.

I agree. I was also completely focused on the issue in the wild. — Reporting a thought/uncertainty in the back of my mind: Do we actually have to solve this (is that feature actually needed in the wild)? Or would it be sufficient to clearly report an error if someone tried to use the non-existing feature? And then work on it as needed.

I'll add a note about it to the PR description, then merge this PR when I see that the GHA are happy.

Sure, I can't commit a lot of time, but happy to help over the next week or so.

I really appreciate any help! And I'm a big believer in small steps. I want to encourage you to pick out one thing at a time. Solve it. Merge. Next one. (My goal, in general, is to review PRs within hours; highest priority.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or would it be sufficient to clearly report an error if someone tried to use the non-existing feature? And then work on it as needed.

Yeah, tricky, it's a balance. I'd at least throw in case there's a non default deleter detected in as_unique_ptr, that way it explicitly fails, instead of implicitly losing the deleter and leaking memory, and that should still be doable while avoiding most of the extra code I had to add to account for it.

Speaking of as_unique_ptr, another random thought; that method could be marked &&, so std::unique_ptr<T, D> as_unique_ptr() &&, that ensures it can only be called on an r-value smart_holder, which makes it clear that the smart_holder object after the method call is in an indeterminate state. In that case it would be illegal to call as_unique_ptr on an lvalue though, so it makes it a bit more cumbersome at the callsite, but it does draw more attention to what it is doing. Not saying this should be done, but just stating it is an option to make it clear we are moving out of the smart holder.

I'll add a note about it to the PR description, then merge this PR when I see that the GHA are happy.

Sounds good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see that as_unique_ptr was also wrapped with PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP; 100% agree it's probably not worth worrying about the deleter there then 👍

Loading