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

git merge smart_holder #30157

Merged
merged 10 commits into from
Aug 29, 2024
3 changes: 2 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,8 @@ set(PYBIND11_HEADERS
include/pybind11/stl/filesystem.h
include/pybind11/trampoline_self_life_support.h
include/pybind11/type_caster_pyobject_ptr.h
include/pybind11/typing.h)
include/pybind11/typing.h
include/pybind11/warnings.h)

# Compare with grep and warn if mismatched
if(PYBIND11_MASTER_PROJECT)
Expand Down
44 changes: 44 additions & 0 deletions docs/faq.rst
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,50 @@ been received, you must either explicitly interrupt execution by throwing
});
}

What is a highly conclusive and simple way to find memory leaks (e.g. in pybind11 bindings)?
============================================================================================

Use ``while True`` & ``top`` (Linux, macOS).

For example, locally change tests/test_type_caster_pyobject_ptr.py like this:

.. code-block:: diff

def test_return_list_pyobject_ptr_reference():
+ while True:
vec_obj = m.return_list_pyobject_ptr_reference(ValueHolder)
assert [e.value for e in vec_obj] == [93, 186]
# Commenting out the next `assert` will leak the Python references.
# An easy way to see evidence of the leaks:
# Insert `while True:` as the first line of this function and monitor the
# process RES (Resident Memory Size) with the Unix top command.
- assert m.dec_ref_each_pyobject_ptr(vec_obj) == 2
+ # assert m.dec_ref_each_pyobject_ptr(vec_obj) == 2

Then run the test as you would normally do, which will go into the infinite loop.

**In another shell, but on the same machine** run:

.. code-block:: bash

top

This will show:

.. code-block::

PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
1266095 rwgk 20 0 5207496 611372 45696 R 100.0 0.3 0:08.01 test_type_caste

Look for the number under ``RES`` there. You'll see it going up very quickly.

**Don't forget to Ctrl-C the test command** before your machine becomes
unresponsive due to swapping.

This method only takes a couple minutes of effort and is very conclusive.
What you want to see is that the ``RES`` number is stable after a couple
seconds.

CMake doesn't detect the right Python version
=============================================

Expand Down
15 changes: 9 additions & 6 deletions include/pybind11/detail/struct_smart_holder.h
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ struct smart_holder {
// race conditions, but in the context of Python it is a bug (elsewhere)
// if the Global Interpreter Lock (GIL) is not being held when this code
// is reached.
// SMART_HOLDER_WIP: IMPROVABLE: assert(GIL is held).
// PYBIND11:REMINDER: This may need to be protected by a mutex in free-threaded Python.
if (vptr.use_count() != 1) {
throw std::invalid_argument(std::string("Cannot disown use_count != 1 (") + context
+ ").");
Expand Down Expand Up @@ -277,29 +277,32 @@ struct smart_holder {
return hld;
}

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

// Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
// 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);
is_disowned = false;
}

// Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
// Caller is responsible for ensuring the complex preconditions
// (see `smart_holder_type_caster_support::load_helper`).
void release_disowned() { vptr.reset(); }

// SMART_HOLDER_WIP: review this function.
void ensure_can_release_ownership(const char *context = "ensure_can_release_ownership") const {
ensure_is_not_disowned(context);
ensure_vptr_is_using_builtin_delete(context);
ensure_use_count_1(context);
}

// Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
// 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);
release_disowned();
Expand Down
9 changes: 4 additions & 5 deletions include/pybind11/detail/type_caster_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ inline PyObject *make_new_instance(PyTypeObject *type);

#ifdef PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT

// SMART_HOLDER_WIP: Needs refactoring of existing pybind11 code.
// PYBIND11:REMINDER: Needs refactoring of existing pybind11 code.
inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo);

PYBIND11_NAMESPACE_BEGIN(smart_holder_type_caster_support)
Expand Down Expand Up @@ -569,8 +569,7 @@ handle smart_holder_from_unique_ptr(std::unique_ptr<T, D> &&src,

if (static_cast<void *>(src.get()) == src_raw_void_ptr) {
// This is a multiple-inheritance situation that is incompatible with the current
// shared_from_this handling (see PR #3023).
// SMART_HOLDER_WIP: IMPROVABLE: Is there a better solution?
// shared_from_this handling (see PR #3023). Is there a better solution?
src_raw_void_ptr = nullptr;
}
auto smhldr = smart_holder::from_unique_ptr(std::move(src), src_raw_void_ptr);
Expand Down Expand Up @@ -626,8 +625,8 @@ handle smart_holder_from_shared_ptr(const std::shared_ptr<T> &src,
void *src_raw_void_ptr = static_cast<void *>(src_raw_ptr);
const detail::type_info *tinfo = st.second;
if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) {
// SMART_HOLDER_WIP: MISSING: Enforcement of consistency with existing smart_holder.
// SMART_HOLDER_WIP: MISSING: keep_alive.
// PYBIND11:REMINDER: MISSING: Enforcement of consistency with existing smart_holder.
// PYBIND11:REMINDER: MISSING: keep_alive.
return existing_inst;
}

Expand Down
5 changes: 2 additions & 3 deletions include/pybind11/trampoline_self_life_support.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,13 @@
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

PYBIND11_NAMESPACE_BEGIN(detail)
// SMART_HOLDER_WIP: Needs refactoring of existing pybind11 code.
// PYBIND11:REMINDER: Needs refactoring of existing pybind11 code.
inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo);
PYBIND11_NAMESPACE_END(detail)

// The original core idea for this struct goes back to PyCLIF:
// https://github.com/google/clif/blob/07f95d7e69dca2fcf7022978a55ef3acff506c19/clif/python/runtime.cc#L37
// URL provided here mainly to give proper credit. To fully explain the `HoldPyObj` feature, more
// context is needed (SMART_HOLDER_WIP).
// URL provided here mainly to give proper credit.
struct trampoline_self_life_support {
detail::value_and_holder v_h;

Expand Down
4 changes: 1 addition & 3 deletions include/pybind11/typing.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,7 @@ class Never : public none {
using none::none;
};

#if defined(__cpp_nontype_template_parameter_class) \
&& (/* See #5201 */ !defined(__GNUC__) \
|| (__GNUC__ > 10 || (__GNUC__ == 10 && __GNUC_MINOR__ >= 3)))
#if defined(__cpp_nontype_template_args) && __cpp_nontype_template_args >= 201911L
# define PYBIND11_TYPING_H_HAS_STRING_LITERAL
template <size_t N>
struct StringLiteral {
Expand Down
75 changes: 75 additions & 0 deletions include/pybind11/warnings.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
pybind11/warnings.h: Python warnings wrappers.

Copyright (c) 2024 Jan Iwaszkiewicz <[email protected]>

All rights reserved. Use of this source code is governed by a
BSD-style license that can be found in the LICENSE file.
*/

#pragma once

#include "pybind11.h"
#include "detail/common.h"

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

PYBIND11_NAMESPACE_BEGIN(detail)

inline bool PyWarning_Check(PyObject *obj) {
int result = PyObject_IsSubclass(obj, PyExc_Warning);
if (result == 1) {
return true;
}
if (result == -1) {
raise_from(PyExc_SystemError,
"pybind11::detail::PyWarning_Check(): PyObject_IsSubclass() call failed.");
throw error_already_set();
}
return false;
}

PYBIND11_NAMESPACE_END(detail)

PYBIND11_NAMESPACE_BEGIN(warnings)

inline object
new_warning_type(handle scope, const char *name, handle base = PyExc_RuntimeWarning) {
if (!detail::PyWarning_Check(base.ptr())) {
pybind11_fail("pybind11::warnings::new_warning_type(): cannot create custom warning, base "
"must be a subclass of "
"PyExc_Warning!");
}
if (hasattr(scope, name)) {
pybind11_fail("pybind11::warnings::new_warning_type(): an attribute with name \""
+ std::string(name) + "\" exists already.");
}
std::string full_name = scope.attr("__name__").cast<std::string>() + std::string(".") + name;
handle h(PyErr_NewException(full_name.c_str(), base.ptr(), nullptr));
if (!h) {
raise_from(PyExc_SystemError,
"pybind11::warnings::new_warning_type(): PyErr_NewException() call failed.");
throw error_already_set();
}
auto obj = reinterpret_steal<object>(h);
scope.attr(name) = obj;
return obj;
}

// Similar to Python `warnings.warn()`
inline void
warn(const char *message, handle category = PyExc_RuntimeWarning, int stack_level = 2) {
if (!detail::PyWarning_Check(category.ptr())) {
pybind11_fail(
"pybind11::warnings::warn(): cannot raise warning, category must be a subclass of "
"PyExc_Warning!");
}

if (PyErr_WarnEx(category.ptr(), message, stack_level) == -1) {
throw error_already_set();
}
}

PYBIND11_NAMESPACE_END(warnings)

PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
4 changes: 2 additions & 2 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ set(PYBIND11_TEST_FILES
test_class_sh_unique_ptr_member
test_class_sh_virtual_py_cpp_mix
test_class_sh_void_ptr_capsule
test_classh_mock
test_const_name
test_constants_and_functions
test_copy_move
Expand Down Expand Up @@ -182,7 +181,8 @@ set(PYBIND11_TEST_FILES
test_unnamed_namespace_a
test_unnamed_namespace_b
test_vector_unique_ptr_member
test_virtual_functions)
test_virtual_functions
test_warnings)

# Invoking cmake with something like:
# cmake -DPYBIND11_TEST_OVERRIDE="test_callbacks.cpp;test_pickling.cpp" ..
Expand Down
1 change: 1 addition & 0 deletions tests/extra_python_package/test_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
"include/pybind11/trampoline_self_life_support.h",
"include/pybind11/type_caster_pyobject_ptr.h",
"include/pybind11/typing.h",
"include/pybind11/warnings.h",
}

detail_headers = {
Expand Down
2 changes: 1 addition & 1 deletion tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def test_shared_ptr_arg_identity():
del obj
pytest.gc_collect()

# SMART_HOLDER_WIP: the behavior below is DIFFERENT from PR #2839
# NOTE: the behavior below is DIFFERENT from PR #2839
# python reference is gone because it is not an Alias instance
assert objref() is None
assert tester.has_python_instance() is False
Expand Down
73 changes: 0 additions & 73 deletions tests/test_classh_mock.cpp

This file was deleted.

13 changes: 0 additions & 13 deletions tests/test_classh_mock.py

This file was deleted.

4 changes: 2 additions & 2 deletions tests/test_pytypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -1026,7 +1026,7 @@ def test_optional_object_annotations(doc):

@pytest.mark.skipif(
not m.defined_PYBIND11_TYPING_H_HAS_STRING_LITERAL,
reason="C++20 feature not available.",
reason="C++20 non-type template args feature not available.",
)
def test_literal(doc):
assert (
Expand All @@ -1037,7 +1037,7 @@ def test_literal(doc):

@pytest.mark.skipif(
not m.defined_PYBIND11_TYPING_H_HAS_STRING_LITERAL,
reason="C++20 feature not available.",
reason="C++20 non-type template args feature not available.",
)
def test_typevar(doc):
assert (
Expand Down
Loading