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

Allow prealloc and registration before calling C++ ctor #4116

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
97 changes: 97 additions & 0 deletions docs/advanced/classes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1333,3 +1333,100 @@ You can do that using ``py::custom_type_setup``:
cls.def_readwrite("value", &OwnsPythonObjects::value);
.. versionadded:: 2.8

Accessing Python object from C++
================================

In advanced cases, it's really handy to access the Python sibling of
a C++ object to get/set attributes, do some heavy computations, just hide
the implementation details, or even dynamically create a new attribute!

One just need to rely on the `casting capabilities`_ of ``pybind11``:

.. code-block:: cpp
// main.cpp
#include "pybind11/pybind11.h"
namespace py = pybind11;
class Base {
public:
Base() = default;
std::string get_foo() { return py::cast(this).attr("foo").cast<std::string>(); };
void set_bar() { py::cast(this).attr("bar") = 10.; };
};
PYBIND11_MODULE(test, m) {
py::class_<Base>(m, "Base")
.def(py::init<>())
.def("get_foo", &Base::get_foo)
.def("set_bar", &Base::set_bar);
}
.. code-block:: python
# test.py
from test import Base
class Derived(Base):
def __init__(self):
Base.__init__(self)
self.foo = "hello"
b = Derived()
assert b.get_foo() == "hello"
assert not hasattr(b, "bar")
b.set_bar()
assert b.bar == 10.0
However, there is a special case where such a behavior needs a hint to work as expected:
when the C++ constructor is called, the C++ object is not yet allocated and registered,
making impossible the casting operation.

It's thus impossible to access the Python object from the C++ constructor one *as is*.

Adding the ``py::preallocate()`` extra option to a constructor binding definition informs
``pybind11`` to allocate the memory and register the object location just before calling the C++
constructor, enabling the use of ``py::cast(this)``:


.. code-block:: cpp
// main.cpp
#include "pybind11/pybind11.h"
namespace py = pybind11;
class Base {
public:
Base() { py::cast(this).attr("bar") = 10.; };
};
PYBIND11_MODULE(test, m) {
py::class_<Base>(m, "Base")
.def(py::init<>(), py::preallocate());
}
.. code-block:: python
# test.py
from test import Base
class Derived(Base):
...
b = Derived()
assert hasattr(b, "bar")
assert b.bar == 10.0
.. _casting capabilities: https://pybind11.readthedocs.io/en/stable/advanced/pycpp/object.html?highlight=cast#casting-back-and-forth
8 changes: 8 additions & 0 deletions include/pybind11/attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ struct keep_alive {};
/// Annotation indicating that a class is involved in a multiple inheritance relationship
struct multiple_inheritance {};

/// Annotation indicating that a class should be preallocated and registered before construction
struct preallocate {};

/// Annotation which enables dynamic attributes, i.e. adds `__dict__` to a class
struct dynamic_attr {};

Expand Down Expand Up @@ -438,6 +441,11 @@ struct process_attribute<is_operator> : process_attribute_default<is_operator> {
static void init(const is_operator &, function_record *r) { r->is_operator = true; }
};

template <>
struct process_attribute<preallocate> : process_attribute_default<preallocate> {
static void init(const preallocate &, function_record *) {}
};

template <>
struct process_attribute<is_new_style_constructor>
: process_attribute_default<is_new_style_constructor> {
Expand Down
12 changes: 12 additions & 0 deletions include/pybind11/detail/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,18 @@ using is_lambda = satisfies_none_of<remove_reference_t<T>,
std::is_pointer,
std::is_member_pointer>;

/// Check if T is part of a template parameter pack
template <typename T, typename... List>
struct contains : std::true_type {};

template <typename T, typename Head, typename... Rest>
struct contains<T, Head, Rest...>
: std::conditional<std::is_same<T, Head>::value, std::true_type, contains<T, Rest...>>::type {
};

template <typename T>
struct contains<T> : std::false_type {};

// [workaround(intel)] Internal error on fold expression
/// Apply a function over each element of a parameter pack
#if defined(__cpp_fold_expressions) && !defined(__INTEL_COMPILER)
Expand Down
72 changes: 51 additions & 21 deletions include/pybind11/detail/init.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,24 +61,53 @@ constexpr bool is_alias(void *) {
return false;
}

// Constructs and returns a new object; if the given arguments don't map to a constructor, we fall
// Constructs a new object inplace; if the given arguments don't map to a constructor, we fall
// back to brace aggregate initiailization so that for aggregate initialization can be used with
// py::init, e.g. `py::init<int, int>` to initialize a `struct T { int a; int b; }`. For
// non-aggregate types, we need to use an ordinary T(...) constructor (invoking as `T{...}` usually
// works, but will not do the expected thing when `T` has an `initializer_list<T>` constructor).
template <typename Class,
typename... Args,
detail::enable_if_t<std::is_constructible<Class, Args...>::value, int> = 0>
inline Class *construct_or_initialize(Args &&...args) {
return new Class(std::forward<Args>(args)...);
template <
typename Class,
bool Preallocate,
typename... Args,
detail::enable_if_t<std::is_constructible<Class, Args...>::value && !Preallocate, int> = 0>
inline void construct_or_initialize(value_and_holder &v_h, Args &&...args) {
v_h.value_ptr() = new Class(std::forward<Args>(args)...);
}
template <typename Class,
typename... Args,
detail::enable_if_t<!std::is_constructible<Class, Args...>::value, int> = 0>
inline Class *construct_or_initialize(Args &&...args) {
return new Class{std::forward<Args>(args)...};
template <
typename Class,
bool Preallocate,
typename... Args,
detail::enable_if_t<!std::is_constructible<Class, Args...>::value && !Preallocate, int> = 0>
inline void construct_or_initialize(value_and_holder &v_h, Args &&...args) {
v_h.value_ptr() = new Class{std::forward<Args>(args)...};
}
// The preallocated variants are performing memory allocation and registration before actually
// calling the constructor to allow casting the C++ pointer to its Python counterpart.
template <
typename Class,
bool Preallocate,
typename... Args,
detail::enable_if_t<std::is_constructible<Class, Args...>::value && Preallocate, int> = 0>
inline void construct_or_initialize(value_and_holder &v_h, Args &&...args) {
v_h.value_ptr() = malloc(sizeof(Class));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am pretty sure we only 'Pymem_alloc*' apis for portability reasons.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

I need to find time to look at this carefully, but a quick search for "free" didn't match anything in the diff.

@adriendelsalle did you look already how the memory is deallocated? We want to be sure it is compatible with the allocation mechanism.

Purely from memory and hand-waving: I believe for old-style __init__ the current code can wind up with a delete that has no matching new (some other allocation mechanism is used). If you happen to stumble over that, too, adding a comment would be great. (I wanted to do that but never got back to it.)

Copy link
Author

Choose a reason for hiding this comment

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

Awesome to have your comments!
I'm using the lib for years but only started to go in the details few days ago..so I'm not really comfortable with alloc/dealloc.
Is there some dev or design/architecture doc for that? I'll do my best to digest that tomorrow!

Copy link
Collaborator

@rwgk rwgk Aug 8, 2022

Choose a reason for hiding this comment

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

Is there some dev or design/architecture doc for that?

Not to my knowledge.
At least I found it necessary to learn the hard way, experimenting a lot with the code.
Feel free to ask questions here. Maybe we can learn together.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seeing the only now, the valgrind failure seems to be spot-on:

https://github.com/pybind/pybind11/runs/7732180285?check_suite_focus=true

==7330== Mismatched free() / delete / delete []

Copy link
Author

Choose a reason for hiding this comment

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

I saw that! Hopefully I added tests to cover the changes. I'll reproduce locally an debug that

Feel free to ask questions here. Maybe we can learn together.

👍

Copy link
Author

@adriendelsalle adriendelsalle Aug 9, 2022

Choose a reason for hiding this comment

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

I just pushed a change to replace malloc with new allocation to be fully consistent with delete used in dealloc. It should be then very similar to what's done in the allocation+construction overload of construct_or_initialize.

It's now all green using Valgrind on the tests locally. I used act to play the CI job.

I am pretty sure we only 'Pymem_alloc*' apis for portability reasons.

I don't really get the point, could you please elaborate a bit? I don't see why allocation and construction of a C++ object should use some Py* alloc.

Here is my understanding of how the changes integrate with the implem:

  • passing py::preallocate() as an extra argument of a new style constructor will preallocate and register just before actually calling the constructor of the class
  • if the call is fine, the holder is still not registered making this piece of code to be called (only to init the holder then):
    if (overloads->is_constructor && !self_value_and_holder.holder_constructed()) {
    auto *pi = reinterpret_cast<instance *>(parent.ptr());
    self_value_and_holder.type->init_instance(pi, nullptr);
    }
  • if the call is not successful (because of cast error AFAIU), the next overload try (if any) will deallocate the memory via operator delete if already allocated (no holder constructed yet):
    if (func.is_new_style_constructor) {
    // The `value` may have been preallocated by an old-style `__init__`
    // if it was a preceding candidate for overload resolution.
    if (self_value_and_holder) {
    self_value_and_holder.type->dealloc(self_value_and_holder);
    • maybe it would worth to update the comment because now it's not only in case of old-style __init__ that the memory is already allocated?
    • I don't really understand the comment:
      } else if (rec->is_new_style_constructor && arg_index == 0) {
      // A new-style `__init__` takes `self` as `value_and_holder`.
      // Rewrite it to the proper class type.

      since it looks like to me the opposite: a new-style __init__ takes value_and_holder as self, isn't it?
      call.init_self = PyTuple_GET_ITEM(args_in, 0);
      call.args.emplace_back(reinterpret_cast<PyObject *>(&self_value_and_holder));

register_instance(v_h.inst, v_h.value_ptr(), v_h.type);
v_h.set_instance_registered();

new (v_h.value_ptr<Class>()) Class(std::forward<Args>(args)...);
}
template <
typename Class,
bool Preallocate,
typename... Args,
detail::enable_if_t<!std::is_constructible<Class, Args...>::value && Preallocate, int> = 0>
inline void construct_or_initialize(value_and_holder &v_h, Args &&...args) {
v_h.value_ptr() = malloc(sizeof(Class));
register_instance(v_h.inst, v_h.value_ptr(), v_h.type);
v_h.set_instance_registered();

new (v_h.value_ptr<Class>()) Class{std::forward<Args>(args)...};
}

// Attempts to constructs an alias using a `Alias(Cpp &&)` constructor. This allows types with
// an alias to provide only a single Cpp factory function as long as the Alias can be
// constructed from an rvalue reference of the base Cpp type. This means that Alias classes
Expand Down Expand Up @@ -200,7 +229,8 @@ struct constructor {
cl.def(
"__init__",
[](value_and_holder &v_h, Args... args) {
v_h.value_ptr() = construct_or_initialize<Cpp<Class>>(std::forward<Args>(args)...);
construct_or_initialize<Cpp<Class>, contains<preallocate, Extra...>::value>(
v_h, std::forward<Args>(args)...);
},
is_new_style_constructor(),
extra...);
Expand All @@ -215,11 +245,11 @@ struct constructor {
"__init__",
[](value_and_holder &v_h, Args... args) {
if (Py_TYPE(v_h.inst) == v_h.type->type) {
v_h.value_ptr()
= construct_or_initialize<Cpp<Class>>(std::forward<Args>(args)...);
construct_or_initialize<Cpp<Class>, contains<preallocate, Extra...>::value>(
v_h, std::forward<Args>(args)...);
} else {
v_h.value_ptr()
= construct_or_initialize<Alias<Class>>(std::forward<Args>(args)...);
construct_or_initialize<Alias<Class>, contains<preallocate, Extra...>::value>(
v_h, std::forward<Args>(args)...);
}
},
is_new_style_constructor(),
Expand All @@ -234,8 +264,8 @@ struct constructor {
cl.def(
"__init__",
[](value_and_holder &v_h, Args... args) {
v_h.value_ptr()
= construct_or_initialize<Alias<Class>>(std::forward<Args>(args)...);
construct_or_initialize<Alias<Class>, contains<preallocate, Extra...>::value>(
v_h, std::forward<Args>(args)...);
},
is_new_style_constructor(),
extra...);
Expand All @@ -253,8 +283,8 @@ struct alias_constructor {
cl.def(
"__init__",
[](value_and_holder &v_h, Args... args) {
v_h.value_ptr()
= construct_or_initialize<Alias<Class>>(std::forward<Args>(args)...);
construct_or_initialize<Alias<Class>, contains<preallocate, Extra...>::value>(
v_h, std::forward<Args>(args)...);
},
is_new_style_constructor(),
extra...);
Expand Down
22 changes: 22 additions & 0 deletions tests/test_methods_and_attributes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,20 @@ struct RValueRefParam {
std::size_t func4(std::string &&s) const & { return s.size(); }
};

// Test Python init from C++ constructor
struct InitPyFromCpp1 {
InitPyFromCpp1() { py::cast(this).attr("bar") = 10.; };
};
struct InitPyFromCpp2 {
InitPyFromCpp2() { py::cast(this).attr("bar") = 10.; };
};
struct InitPyFromCppDynamic1 {
InitPyFromCppDynamic1() { py::cast(this).attr("bar") = 10.; };
};
struct InitPyFromCppDynamic2 {
InitPyFromCppDynamic2() { py::cast(this).attr("bar") = 10.; };
};

TEST_SUBMODULE(methods_and_attributes, m) {
// test_methods_and_attributes
py::class_<ExampleMandA> emna(m, "ExampleMandA");
Expand Down Expand Up @@ -456,4 +470,12 @@ TEST_SUBMODULE(methods_and_attributes, m) {
.def("func2", &RValueRefParam::func2)
.def("func3", &RValueRefParam::func3)
.def("func4", &RValueRefParam::func4);

// Test Python init from C++ constructor
py::class_<InitPyFromCpp1>(m, "InitPyFromCpp1").def(py::init<>());
py::class_<InitPyFromCpp2>(m, "InitPyFromCpp2").def(py::init<>(), py::preallocate());
py::class_<InitPyFromCppDynamic1>(m, "InitPyFromCppDynamic1", py::dynamic_attr())
.def(py::init<>());
py::class_<InitPyFromCppDynamic2>(m, "InitPyFromCppDynamic2", py::dynamic_attr())
.def(py::init<>(), py::preallocate());
}
48 changes: 48 additions & 0 deletions tests/test_methods_and_attributes.py
Original file line number Diff line number Diff line change
Expand Up @@ -525,3 +525,51 @@ def test_rvalue_ref_param():
assert r.func2("1234") == 4
assert r.func3("12345") == 5
assert r.func4("123456") == 6


@pytest.mark.xfail("env.PYPY")
def test_init_py_from_cpp():
# test dynamically added attr from C++ to Python counterpart

# 1. on class not supporting dynamic attributes
with pytest.raises(AttributeError):
m.InitPyFromCpp1()

with pytest.raises(AttributeError):
m.InitPyFromCpp2()

# 2. on derived class of base not supporting dynamic attributes
class Derived1(m.InitPyFromCpp1):
...

with pytest.raises(AttributeError):
Derived1()

class Derived2(m.InitPyFromCpp2):
...

assert Derived2().bar == 10.0

# 3. on class supporting dynamic attributes
# constructor will set the `bar` attribute to a temporary Python object
a = m.InitPyFromCppDynamic1()
with pytest.raises(AttributeError):
a.bar

# works fine
assert m.InitPyFromCppDynamic2().bar == 10.0

# 4. on derived class of base supporting dynamic attributes
class DynamicDerived1(m.InitPyFromCppDynamic1):
...

# still the same issue
d = DynamicDerived1()
with pytest.raises(AttributeError):
d.bar

# works fine
class DynamicDerived2(m.InitPyFromCppDynamic2):
...

assert DynamicDerived2().bar == 10.0