Skip to content

WIP: Call std::get_deleter<guarded_delete>() through internals #5728

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
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
23 changes: 20 additions & 3 deletions include/pybind11/detail/internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#pragma once

#include <pybind11/conduit/pybind11_platform_abi_id.h>
#include <pybind11/detail/struct_smart_holder.h>
#include <pybind11/gil_simple.h>
#include <pybind11/pytypes.h>

Expand All @@ -35,11 +36,11 @@
/// further ABI-incompatible changes may be made before the ABI is officially
/// changed to the new version.
#ifndef PYBIND11_INTERNALS_VERSION
# define PYBIND11_INTERNALS_VERSION 10
# define PYBIND11_INTERNALS_VERSION 11
#endif

#if PYBIND11_INTERNALS_VERSION < 10
# error "PYBIND11_INTERNALS_VERSION 10 is the minimum for all platforms for pybind11v3."
#if PYBIND11_INTERNALS_VERSION < 11
# error "PYBIND11_INTERNALS_VERSION 11 is the minimum for all platforms for pybind11v3."
#endif

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
Expand Down Expand Up @@ -254,6 +255,8 @@ struct internals {

type_map<PyObject *> native_enum_type_map;

std::vector<memory::get_guarded_delete_fn> memory_guarded_delete_fns; // See PRs #5728, #5700.

internals()
: static_property_type(make_static_property_type()),
default_metaclass(make_default_metaclass()) {
Expand All @@ -262,6 +265,7 @@ struct internals {

istate = cur_tstate->interp;
registered_exception_translators.push_front(&translate_exception);
memory_guarded_delete_fns.push_back(memory::get_guarded_delete);
#ifdef Py_GIL_DISABLED
// Scale proportional to the number of cores. 2x is a heuristic to reduce contention.
auto num_shards
Expand Down Expand Up @@ -671,6 +675,19 @@ inline auto with_exception_translators(const F &cb)
local_internals.registered_exception_translators);
}

// Traverse all DSOs to find memory::guarded_delete with matching RTTI.
inline memory::guarded_delete *find_memory_guarded_delete(const std::shared_ptr<void> &ptr) {
return with_internals([&](detail::internals &internals) -> memory::guarded_delete * {
for (auto fn : internals.memory_guarded_delete_fns) {
memory::guarded_delete *gd = fn(ptr);
if (gd) {
return gd;
}
}
return nullptr;
});
}

inline std::uint64_t mix64(std::uint64_t z) {
// David Stafford's variant 13 of the MurmurHash3 finalizer popularized
// by the SplitMix PRNG.
Expand Down
49 changes: 23 additions & 26 deletions include/pybind11/detail/struct_smart_holder.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,19 +58,6 @@ High-level aspects:
#include <typeinfo>
#include <utility>

// IMPORTANT: This code block must stay BELOW the #include <stdexcept> above.
// This is only required on some builds with libc++ (one of three implementations
// in
// https://github.com/llvm/llvm-project/blob/a9b64bb3180dab6d28bf800a641f9a9ad54d2c0c/libcxx/include/typeinfo#L271-L276
// require it)
#if !defined(PYBIND11_EXPORT_GUARDED_DELETE)
# if defined(_LIBCPP_VERSION) && !defined(WIN32) && !defined(_WIN32)
# define PYBIND11_EXPORT_GUARDED_DELETE __attribute__((visibility("default")))
# else
# define PYBIND11_EXPORT_GUARDED_DELETE
# endif
#endif

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
PYBIND11_NAMESPACE_BEGIN(memory)

Expand All @@ -91,7 +78,8 @@ static constexpr bool type_has_shared_from_this(const void *) {
return false;
}

struct PYBIND11_EXPORT_GUARDED_DELETE guarded_delete {
struct guarded_delete {
// NOTE: PYBIND11_INTERNALS_VERSION needs to be bumped if changes are made to this struct.
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.
Expand All @@ -113,6 +101,12 @@ struct PYBIND11_EXPORT_GUARDED_DELETE guarded_delete {
}
};

inline guarded_delete *get_guarded_delete(const std::shared_ptr<void> &ptr) {
return std::get_deleter<guarded_delete>(ptr);
}

using get_guarded_delete_fn = guarded_delete *(*) (const std::shared_ptr<void> &);

template <typename T, typename std::enable_if<std::is_destructible<T>::value, int>::type = 0>
inline void builtin_delete_if_destructible(void *raw_ptr) {
std::default_delete<T>{}(static_cast<T *>(raw_ptr));
Expand All @@ -133,6 +127,7 @@ guarded_delete make_guarded_builtin_delete(bool armed_flag) {

template <typename T, typename D>
struct custom_deleter {
// NOTE: PYBIND11_INTERNALS_VERSION needs to be bumped if changes are made to this struct.
D deleter;
explicit custom_deleter(D &&deleter) : deleter{std::forward<D>(deleter)} {}
void operator()(void *raw_ptr) { deleter(static_cast<T *>(raw_ptr)); }
Expand All @@ -151,6 +146,7 @@ inline bool is_std_default_delete(const std::type_info &rtti_deleter) {
}

struct smart_holder {
// NOTE: PYBIND11_INTERNALS_VERSION needs to be bumped if changes are made to this struct.
const std::type_info *rtti_uqp_del = nullptr;
std::shared_ptr<void> vptr;
bool vptr_is_using_noop_deleter : 1;
Expand Down Expand Up @@ -244,19 +240,20 @@ struct smart_holder {
}
}

void reset_vptr_deleter_armed_flag(bool armed_flag) const {
auto *vptr_del_ptr = std::get_deleter<guarded_delete>(vptr);
if (vptr_del_ptr == nullptr) {
void reset_vptr_deleter_armed_flag(const get_guarded_delete_fn ggd_fn, bool armed_flag) const {
auto *gd = ggd_fn(vptr);
if (gd == nullptr) {
throw std::runtime_error(
"smart_holder::reset_vptr_deleter_armed_flag() called in an invalid context.");
}
vptr_del_ptr->armed_flag = armed_flag;
gd->armed_flag = armed_flag;
}

// Caller is responsible for precondition: ensure_compatible_rtti_uqp_del<T, D>() must succeed.
template <typename T, typename D>
std::unique_ptr<D> extract_deleter(const char *context) const {
const auto *gd = std::get_deleter<guarded_delete>(vptr);
std::unique_ptr<D> extract_deleter(const char *context,
const get_guarded_delete_fn ggd_fn) const {
auto *gd = ggd_fn(vptr);
if (gd && gd->use_del_fun) {
const auto &custom_deleter_ptr = gd->del_fun.template target<custom_deleter<T, D>>();
if (custom_deleter_ptr == nullptr) {
Expand Down Expand Up @@ -301,15 +298,15 @@ struct smart_holder {

// Caller is responsible for ensuring the complex preconditions
// (see `smart_holder_type_caster_support::load_helper`).
void disown() {
reset_vptr_deleter_armed_flag(false);
void disown(const get_guarded_delete_fn ggd_fn) {
reset_vptr_deleter_armed_flag(ggd_fn, false);
is_disowned = true;
}

// Caller is responsible for ensuring the complex preconditions
// (see `smart_holder_type_caster_support::load_helper`).
void reclaim_disowned() {
reset_vptr_deleter_armed_flag(true);
void reclaim_disowned(const get_guarded_delete_fn ggd_fn) {
reset_vptr_deleter_armed_flag(ggd_fn, true);
is_disowned = false;
}

Expand All @@ -325,8 +322,8 @@ struct smart_holder {

// Caller is responsible for ensuring the complex preconditions
// (see `smart_holder_type_caster_support::load_helper`).
void release_ownership() {
reset_vptr_deleter_armed_flag(false);
void release_ownership(const get_guarded_delete_fn ggd_fn) {
reset_vptr_deleter_armed_flag(ggd_fn, false);
release_disowned();
}

Expand Down
18 changes: 10 additions & 8 deletions include/pybind11/detail/type_caster_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ struct value_and_holder_helper {

// have_holder() must be true or this function will fail.
void throw_if_instance_is_currently_owned_by_shared_ptr() const {
auto *vptr_gd_ptr = std::get_deleter<memory::guarded_delete>(holder().vptr);
auto *vptr_gd_ptr = find_memory_guarded_delete(holder().vptr);
if (vptr_gd_ptr != nullptr && !vptr_gd_ptr->released_ptr.expired()) {
throw value_error("Python instance is currently owned by a std::shared_ptr.");
}
Expand Down Expand Up @@ -576,7 +576,7 @@ handle smart_holder_from_unique_ptr(std::unique_ptr<T, D> &&src,
}
// Critical transfer-of-ownership section. This must stay together.
self_life_support->deactivate_life_support();
holder.reclaim_disowned();
holder.reclaim_disowned(find_memory_guarded_delete);
(void) src.release();
// Critical section end.
return existing_inst;
Expand Down Expand Up @@ -763,7 +763,7 @@ struct load_helper : value_and_holder_helper {
}
auto *type_raw_ptr = static_cast<T *>(void_raw_ptr);
if (python_instance_is_alias && !force_potentially_slicing_shared_ptr) {
auto *vptr_gd_ptr = std::get_deleter<memory::guarded_delete>(hld.vptr);
auto *vptr_gd_ptr = find_memory_guarded_delete(holder().vptr);
if (vptr_gd_ptr != nullptr) {
std::shared_ptr<void> released_ptr = vptr_gd_ptr->released_ptr.lock();
if (released_ptr) {
Expand Down Expand Up @@ -818,13 +818,14 @@ struct load_helper : value_and_holder_helper {
// This is enforced indirectly by a static_assert in the class_ implementation:
assert(!python_instance_is_alias || self_life_support);

std::unique_ptr<D> extracted_deleter = holder().template extract_deleter<T, D>(context);
std::unique_ptr<D> extracted_deleter
= holder().template extract_deleter<T, D>(context, find_memory_guarded_delete);

// Critical transfer-of-ownership section. This must stay together.
if (self_life_support != nullptr) {
holder().disown();
holder().disown(find_memory_guarded_delete);
} else {
holder().release_ownership();
holder().release_ownership(find_memory_guarded_delete);
}
auto result = unique_with_deleter<T, D>(raw_type_ptr, std::move(extracted_deleter));
if (self_life_support != nullptr) {
Expand All @@ -848,8 +849,9 @@ struct load_helper : value_and_holder_helper {
return unique_with_deleter<T, D>(nullptr, std::unique_ptr<D>());
}
holder().template ensure_compatible_rtti_uqp_del<T, D>(context);
return unique_with_deleter<T, D>(
raw_type_ptr, std::move(holder().template extract_deleter<T, D>(context)));
return unique_with_deleter<T, D>(raw_type_ptr,
std::move(holder().template extract_deleter<T, D>(
context, find_memory_guarded_delete)));
}
};

Expand Down
4 changes: 2 additions & 2 deletions tests/pure_cpp/smart_holder_poc.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ T *as_raw_ptr_release_ownership(smart_holder &hld,
const char *context = "as_raw_ptr_release_ownership") {
hld.ensure_can_release_ownership(context);
T *raw_ptr = hld.as_raw_ptr_unowned<T>();
hld.release_ownership();
hld.release_ownership(get_guarded_delete);
return raw_ptr;
}

Expand All @@ -46,7 +46,7 @@ std::unique_ptr<T, D> as_unique_ptr(smart_holder &hld) {
hld.ensure_compatible_rtti_uqp_del<T, D>(context);
hld.ensure_use_count_1(context);
T *raw_ptr = hld.as_raw_ptr_unowned<T>();
hld.release_ownership();
hld.release_ownership(get_guarded_delete);
// KNOWN DEFECT (see PR #4850): Does not copy the deleter.
return std::unique_ptr<T, D>(raw_ptr);
}
Expand Down
12 changes: 7 additions & 5 deletions tests/pure_cpp/smart_holder_poc_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#define CATCH_CONFIG_MAIN
#include "catch.hpp"

using pybind11::memory::guarded_delete;
using pybind11::memory::smart_holder;
namespace poc = pybind11::memory::smart_holder_poc;

Expand Down Expand Up @@ -160,11 +161,12 @@ TEST_CASE("from_raw_ptr_take_ownership+as_shared_ptr", "[S]") {
TEST_CASE("from_raw_ptr_take_ownership+disown+reclaim_disowned", "[S]") {
auto hld = smart_holder::from_raw_ptr_take_ownership(new int(19));
std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
hld.disown();
hld.disown(pybind11::memory::get_guarded_delete);
REQUIRE(poc::as_lvalue_ref<int>(hld) == 19);
REQUIRE(*new_owner == 19);
hld.reclaim_disowned(); // Manually veriified: without this, clang++ -fsanitize=address reports
// "detected memory leaks".
// Manually verified: without this, clang++ -fsanitize=address reports
// "detected memory leaks".
hld.reclaim_disowned(pybind11::memory::get_guarded_delete);
// NOLINTNEXTLINE(bugprone-unused-return-value)
(void) new_owner.release(); // Manually verified: without this, clang++ -fsanitize=address
// reports "attempting double-free".
Expand All @@ -175,7 +177,7 @@ TEST_CASE("from_raw_ptr_take_ownership+disown+reclaim_disowned", "[S]") {
TEST_CASE("from_raw_ptr_take_ownership+disown+release_disowned", "[S]") {
auto hld = smart_holder::from_raw_ptr_take_ownership(new int(19));
std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
hld.disown();
hld.disown(pybind11::memory::get_guarded_delete);
REQUIRE(poc::as_lvalue_ref<int>(hld) == 19);
REQUIRE(*new_owner == 19);
hld.release_disowned();
Expand All @@ -187,7 +189,7 @@ TEST_CASE("from_raw_ptr_take_ownership+disown+ensure_is_not_disowned", "[E]") {
auto hld = smart_holder::from_raw_ptr_take_ownership(new int(19));
hld.ensure_is_not_disowned(context); // Does not throw.
std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
hld.disown();
hld.disown(pybind11::memory::get_guarded_delete);
REQUIRE_THROWS_WITH(hld.ensure_is_not_disowned(context),
"Holder was disowned already (test_case).");
}
Expand Down
Loading