[smart_holder] .def_readonly, .def_readwrite adaptors #3581

rwgk · 2021-12-29T20:53:02Z

Description

NOTE: The continuation of this work is under PR #3844.

The original motivation for this PR was to fix the issue observed here: disowning creates a dangling pointer.

To ensure internal consistency down the road, the scope of this PR was expanded to cover .def_readonly and .def_readwrite behavior for

value
raw pointer
std::unique_ptr
std::shared_ptr

members (mutable & const for the pointer types).

For shared_ptr members, the existing behavior is as desired (no work needed).

For value and raw-pointer members, the PyCLIF approach here was adopted: aliasing shared_ptr tying the lifetime of the "outer" object to the lifetime of the Python object pointing to the member. In this way the existing mechanisms prevent the "outer" object from being disowned while pointers to members are still alive.

For unique_ptr members, the situation is complex. This PR only establishes a proof of concept. See the code and comments in tests/test_class_sh_property.py for details. Productizing the proof of concept is left for a future separate PR, as needed.

rwgk · 2022-01-21T22:43:51Z

To track progress (FYI @wangxf123456):

I've verified that the PyCLIF TODO here can be removed when using this PR (ASAN clean).

     a = nested_fields.AA()
     a.b.c.i = 123
     c = a.b.c
-    if 'pybind11' not in nested_fields.__doc__:
-      with self.assertRaises(ValueError):
-        # Cannot give up |a| as |c| still is alive
-        nested_fields.ConsumeAA(a)
-      del c
-      # Since |c| is now gone, we can give up |a|
+    with self.assertRaises(ValueError):
+      # Cannot give up |a| as |c| still is alive
       nested_fields.ConsumeAA(a)
-    else:
-      # TODO(rwgk): This disowns |a| even though |c| is still alive. Accessing
-      # |c| beyond this point generates an ASAN heap-use-after-free error.
-      nested_fields.ConsumeAA(a)
+    del c
+    # Since |c| is now gone, we can give up |a|
+    nested_fields.ConsumeAA(a)

     a = nested_fields.AA()
     a.b.c.i = 123

include/pybind11/pybind11.h

Skylion007 · 2022-01-23T20:06:51Z

include/pybind11/pybind11.h

+    detail::enable_if_t<detail::type_uses_smart_holder_type_caster<T>::value
+                        && detail::type_uses_smart_holder_type_caster<D>::value
+                        && !std::is_pointer<D>::value           //
+                        && !detail::is_std_unique_ptr<D>::value //
+                        && !detail::is_std_shared_ptr<D>::value>> {


Suggested change

detail::enable_if_t<detail::type_uses_smart_holder_type_caster<T>::value

&& detail::type_uses_smart_holder_type_caster<D>::value

&& !std::is_pointer<D>::value //

&& !detail::is_std_unique_ptr<D>::value //

&& !detail::is_std_shared_ptr<D>::value>> {

detail::enable_if_t<detail::all_of<

detail::type_uses_smart_holder_type_caster<T>,

detail::type_uses_smart_holder_type_caster<D>,

!std::is_pointer<D>, //

!detail::is_std_unique_ptr<D>, //

!detail::is_std_shared_ptr<D>>::value>> {

Thanks for pointing this out! It looks a lot nicer.

include/pybind11/pybind11.h

Skylion007 · 2022-01-23T20:09:36Z

include/pybind11/pybind11.h

+    using drp = typename std::remove_pointer<D>::type;
+
+    template <typename PM>
+    static cpp_function readonly(PM pm, const handle &hdl) {


Suggested change

static cpp_function readonly(PM pm, const handle &hdl) {

static cpp_function readonly(PM &pm, const handle &hdl) {

and capture by reference in the lambda. At the very least, the PM should be forwarded otherwise.

I think original pybind11 it's a pointer. being passed around.

This could invoke copy or moves depending on the type PMs.

I couldn't get this to work. I tried a few variations of sprinkling in &, &&, std::forward<PM>, but either it didn't build or I got segfaults. Then I decided it's better to just make it obvious that we're only dealing with pointers anyway, and the perfect forwarding doesn't buy us anything. See the long added comment for the new must_be_member_function_pointer SFINAE helper.

include/pybind11/pybind11.h

Skylion007

@rwgk Made some nits about potentially bugs that could arise from the typing of the templates and that all the helpers could benefit from using the detail::all_of helper defined in details/commons.h

rwgk · 2022-01-28T09:23:03Z

The CI failures are sure to be flakes or (hopefully transient) infrastructure failures. The previous CI run had no failures. I only added a couple comments (and ran pre-commit run --all-files before pushing).

…ronment.

Emulating PyCLIF approach: https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233

…he `const_cast` is not needed.

…builds & tests).

…on pairs struct xetter_cpp_function with read, write.

…just forwards to readonly (to get started).

rainwoodman · 2022-02-02T19:11:41Z

tests/test_class_sh_property.py

+    assert field.num == -99
+    with pytest.raises(ValueError) as excinfo:
+        m.DisownOuter(outer)
+    assert msg(excinfo.value) == "Cannot disown use_count != 1 (loaded_as_unique_ptr)."


The test case is clear and well defined behavior.

My concern is on the practical side -- it is hard to know where the reference to the object is. field could have been passed and kept in many remote part of the application.

A very canonical pattern I know of, for this type of situation is to mark the object as stale, and all uses of it errors. A very typical example is the 'NFS stale handle' error. Then the responsibility is distributed to the consumers of the inner objects, rather than the owner needing to reason a large code base 'who is reference my inner parts?'.

rainwoodman · 2022-02-02T22:13:38Z

tests/test_class_sh_property.py

+    assert msg(excinfo.value) == "Cannot disown use_count != 1 (loaded_as_unique_ptr)."
+    del field
+    m.DisownOuter(outer)
+    with pytest.raises(ValueError) as excinfo:


pytest.raises(ValueError, match=r".* 123 .*")?

charlesbeattie · 2022-02-03T09:31:54Z

tests/test_class_sh_property.cpp

+
+struct Outer {
+    // The compact 4-character naming matches that in test_class_sh_basic.cpp
+    // (c = const, m = mutable).


Maybe comment sh = shared_ptr and uq = unique_ptr as well.

charlesbeattie · 2022-02-03T09:34:22Z

tests/test_class_sh_property.cpp

+    int num = -88;
+};
+
+struct ClassicOuter {


Is the Classic prefix imply that this lass in not wrapped in PYBIND11_SMART_HOLDER_TYPE_CASTERS. A comment above each of these would help with readability.

charlesbeattie · 2022-02-03T09:36:15Z

tests/test_class_sh_property.cpp

@@ -0,0 +1,90 @@
+#include "pybind11_tests.h"
+
+#include "pybind11/smart_holder.h"


I've seen
#include <pybind11/smart_holder.h>
Elsewhere is there a consistent choice we should make with these includes?

charlesbeattie · 2022-02-03T09:37:35Z

Made some small style comments othwise it looks good to me.

@charlesbeattie

… as suggested by @charlesbeattie back in Jan/Feb under PR pybind#3581.

rwgk · 2022-04-02T08:49:35Z

I'm continuing this work under PR #3844. See the Note at top of the PR description there.

@charlesbeattie

… as suggested by @charlesbeattie back in Jan/Feb under PR pybind#3581.

@charlesbeattie

…f PR #3581). (#3844) * Transferred net diff from PR #3581, as-is. * Automatic `pre-commit run --all-files` fixes. NO manual changes. * Removing trailing `//` (originally added to manipulate clang-format), as suggested by @charlesbeattie back in Jan/Feb under PR #3581. * Renaming `xetter_cpp_function` to `property_cpp_function` as suggested by @rainwoodman * Fully explain the terse variable naming scheme in test_class_sh_property (as suggested by @rainwoodman) * Also use parametrize for readonly, readwrite (as suggested by @rainwoodman) * Apply change suggested by @Skylion007 (with clang-format).

rwgk force-pushed the sh_property branch 2 times, most recently from 2ca30c8 to 0b821ba Compare January 4, 2022 00:54

rwgk force-pushed the sh_property branch from 8793a31 to a4ea4ca Compare January 12, 2022 19:40

rwgk force-pushed the sh_property branch from a4ea4ca to c7b7799 Compare January 21, 2022 03:04

rwgk changed the title ~~[smart_holder] WIP property & disowning~~ [smart_holder] .def_readonly, .def_readwrite adaptors Jan 22, 2022

rwgk marked this pull request as ready for review January 22, 2022 02:18

rwgk requested a review from henryiii as a code owner January 22, 2022 02:18

Skylion007 reviewed Jan 23, 2022

View reviewed changes

include/pybind11/pybind11.h Outdated Show resolved Hide resolved

Skylion007 reviewed Jan 23, 2022

View reviewed changes

include/pybind11/pybind11.h Show resolved Hide resolved

Skylion007 reviewed Jan 23, 2022

View reviewed changes

include/pybind11/pybind11.h Outdated Show resolved Hide resolved

Skylion007 requested changes Jan 23, 2022

View reviewed changes

rwgk force-pushed the sh_property branch from 40b30ec to a281ef8 Compare January 28, 2022 03:34

rwgk added 14 commits February 1, 2022 16:00

WIP stash-kind-of-commit

c936890

Minimal reproducer.

f1625dc

Using named namespace to fix double registration error in Google envi…

94c13ca

…ronment.

Adding link to PyCLIF test this was reduced from.

6082ec4

Using aliasing shared_ptr in field getter.

9b9e1bb

Emulating PyCLIF approach: https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233

PyPy xfail

c146120

Renaming in preparation for adding std::shared_ptr<Field>.

4a9228f

Adding is_shared_ptr condition (all tests work SH_AVL, SH_DEF).

526c8d6

clang-format

5cfd529

Also using getter_cpp_function for def_property_readonly.

b3329aa

1. Resolving clang-tidy error (redundant .get()). 2. It turns out t…

bf05ce8

…he `const_cast` is not needed.

Streamlining to &(c_sp.get()->*pm) inline.

6ec3be4

Replacing M m with const handle &hdl for improved readability.

fefb242

Adding some m_mptr, m_uqmp, m_shmp code (highly incomplete, but this …

776816e

…builds & tests).

rwgk added 14 commits February 1, 2022 16:00

Combining member_getter_cpp_function, struct member_setter_cpp_functi…

93a4389

…on pairs struct xetter_cpp_function with read, write.

Distinguishing between xetter_cpp_function::readonly, read, but read …

43052f0

…just forwards to readonly (to get started).

Special xetter_cpp_function::readonly to support m_valu_readonly.

0c36cfd

Making tests more complete, with cleanup.

9b8dca0

xetter_cpp_function implementation for std::unique_ptr

cca7dbb

Applying clang-tidy suggestion (looks much better).

160acc8

Polishing in pybind11.h: comments, moving helper functions out.

50fe88d

Polishing in test_class_sh_property.py: comments, moving code.

4ad7135

Minor tweak to comment.

ef81de9

Adjustment after git rebase -X theirs smart_holder

b0ef8ac

must_be_member_function_pointer<PM>

2d1f958

Making use of all_of, none_of as suggested by @Skylion007

5737c43

Adding comments (no code changes).

1eb3bc3

Adding comment: compact 4-character naming

64c5c4b

rwgk force-pushed the sh_property branch from f1e1b41 to 64c5c4b Compare February 2, 2022 00:08

rainwoodman reviewed Feb 2, 2022

View reviewed changes

charlesbeattie reviewed Feb 3, 2022

View reviewed changes

rwgk added a commit to rwgk/pybind11 that referenced this pull request Apr 2, 2022

Transferred net diff from PR pybind#3581, as-is.

9c7843b

rwgk added a commit to rwgk/pybind11 that referenced this pull request Apr 2, 2022

Removing trailing // (originally added to manipulate clang-format),…

d5a958a

… as suggested by @charlesbeattie back in Jan/Feb under PR pybind#3581.

rwgk mentioned this pull request Apr 2, 2022

[smart_holder] .def_readonly, .def_readwrite adaptors (continuation of PR #3581). #3844

Merged

rwgk closed this Apr 2, 2022

rwgk added a commit to rwgk/pybind11 that referenced this pull request May 13, 2022

Transferred net diff from PR pybind#3581, as-is.

fa0ad8a

rwgk added a commit to rwgk/pybind11 that referenced this pull request May 13, 2022

Removing trailing // (originally added to manipulate clang-format),…

7dee4ad

… as suggested by @charlesbeattie back in Jan/Feb under PR pybind#3581.

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3581

Closed

-    detail::enable_if_t<detail::type_uses_smart_holder_type_caster<T>::value
-                        && detail::type_uses_smart_holder_type_caster<D>::value
-                        && !std::is_pointer<D>::value           //
-                        && !detail::is_std_unique_ptr<D>::value //
-                        && !detail::is_std_shared_ptr<D>::value>> {
+    detail::enable_if_t<detail::all_of<
+                        detail::type_uses_smart_holder_type_caster<T>,
+                        detail::type_uses_smart_holder_type_caster<D>,
+                        !std::is_pointer<D>,           //
+                        !detail::is_std_unique_ptr<D>, //
+                        !detail::is_std_shared_ptr<D>>::value>> {

	static cpp_function readonly(PM pm, const handle &hdl) {
	static cpp_function readonly(PM &pm, const handle &hdl) {

		@@ -0,0 +1,90 @@
		#include "pybind11_tests.h"

		#include "pybind11/smart_holder.h"

[smart_holder] .def_readonly, .def_readwrite adaptors #3581

[smart_holder] .def_readonly, .def_readwrite adaptors #3581

Uh oh!

Conversation

rwgk commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

NOTE: The continuation of this work is under PR #3844.

Uh oh!

rwgk commented Jan 21, 2022

Uh oh!

Uh oh!

Skylion007 Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Skylion007 left a comment

Choose a reason for hiding this comment

Uh oh!

rwgk commented Jan 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charlesbeattie commented Feb 3, 2022

Uh oh!

rwgk commented Apr 2, 2022

Uh oh!

Uh oh!

rwgk commented Dec 29, 2021 •

edited

Loading

Skylion007 Jan 23, 2022 •

edited

Loading