-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Adding py::smart_holder (for smart-pointer interoperability). #2672
Conversation
@eacousineau QQ: did you consider building a custom holder type (using Wondering, could this work:
? |
Quick update, I tested with this change by @rhaschke (rebased #2046): master...rhaschke:rvalue-argument-fix2 With this change and a small tweak (expecting |
To track progress working with the rebased #2046 by @rhaschke (master...rhaschke:rvalue-argument-fix2):
|
7caf582
to
a4c2400
Compare
Current state: upstream master
https://github.com/RobotLocomotion/pybind11
|
I update this PR with highly experimental Before I worked on the |
To be honest, I think that you pursue the wrong approach to realize the automatic conversion of holder types. The Your approach essentially works around a major design flaw of pybind11, namely that it doesn't check for compatibility of holder types when passing them between python and C++: a holder type is simply reinterpret-casted to another one. #2644 (which nobody looked at yet) is an attempt to fix this issue, rejecting attempts to use incompatible holder types. However, you are striving for implicit holder conversion, the 4th and most complicated goal in my summary #2646. I think, if you want to tackle implicit holder conversion, you should mimic the existing code for implicit type conversions, namely implementing a runtime lookup to find the correct conversion mechanism. As outlined in #2646, I envision an explicit mechanism to handle holder type conversions. |
include/pybind11/vptr_holder.h
Outdated
auto u = std::get_if<0>(&vptr_); | ||
if (u) { | ||
auto result = std::shared_ptr<T>(std::move(*u)); | ||
vptr_ = result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwgk: what happens if the unique_ptr was disowned before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1f4f5e2 adds test_promotion_of_disowned_to_shared
, which works as desired.
28d1a0a
to
1f4f5e2
Compare
…erop_test. Main motivation for adding this test: inform work related to pybind/pybind11#2672 Includes a demonstration related to b/175568410 (marked with comments). PiperOrigin-RevId: 347386087
include/pybind11/vptr_holder.h
Outdated
explicit vptr(std::shared_ptr<T> s) : vptr_{s} { std::cout << std::endl << "explicit vptr(std::shared_ptr<T> s)" << std::endl; } | ||
|
||
int ownership_type() const { | ||
if (std::get_if<0>(&vptr_)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return vptr_.index()?
1f4f5e2
to
b63f0c6
Compare
Pure analysis of the current inner workings of pybind11. @rhaschke, @EricCousineau-TRI, @YannickJadoul: please let me know corrections.
Observations:
When converting a
This means the When passing a Python
For the case of a For the case of a |
CORRECTION 2022-05-02: rwgk/rwgk_tbx@7b63b9f — adding e0207d6 adds a minimal demonstration of UB in the handling of shared_ptr holder. The test is based on https://godbolt.org/z/4fdjaW by jorgbrown@ (thanks Jorg!). See below for the test result (segfault).
|
I ported It runs without a problem. |
47b81af adds an additional test designed to demonstrate UB in the shared_ptr holder handling, with root cause in the explicit cast here (this link is also in my comment from 4 days ago): pybind11/include/pybind11/pybind11.h Line 1505 in 30eb39e
The test segfaults in this line: The Boost.Python port of this additional test passes without a problem: rwgk/rwgk_tbx@7d7494a |
Haven't yet fully paged into all the work you've done here, but awesome! FWIW These are the tests in our fork that I had written to address the issue: |
Nope, hadn't considered that! Maybe that'd be better; however, it may still require additional expectations on the holder API, at least for my use cases. For example, to address #1145 (the slicing from inheritance), I wanted to add some form of Also, I read the above summary about the current breakdown. That seems about accurate! I had trouble disentangling this each time, so I ended up basically only supporting single-inheritance when it came down to holder conversion and/or ownership transfer: |
Tracking a line of thought (resulting in an UNPROVEN idea): This was really useful for me to learn about the caster mechanisms: https://gist.github.com/rwgk/4fe600fad45442a5d790b53195a4da51 It's an annotated/edited gdb stack trace, for the situation reported by @eacousineau under #1138. You can see in frames In frame This lead me to a new idea:
Complete code: https://godbolt.org/z/sPTae3 It runs to completion. I'm asking our C++ experts if this is well-defined. Edit ~3 hour later: undoing previous edit, we're still figuring this out. |
8dba00b adds an additional test designed to demonstrate UB in the handling of polymorphic pointers, with root cause in the pybind11/include/pybind11/cast.h Line 229 in 30eb39e
The new test segfaults as shown below. The Boost.Python port of this additional test passes without a problem: rwgk/rwgk_tbx@97919e3 EDIT 2021-01-01: The root problem is that the pybind11 EDIT 2021-01-12: pybind11 actually has functionality for handling thunks ( pybind11/include/pybind11/detail/internals.h Line 136 in 98f1bbb
|
@EricCousineau-TRI wrote:
I looked briefly at #1145 but I'm not clear, in particular what exactly is slicing? But maybe you can see faster if there is a connection, based on the even more reduced test under #2672 (comment)? The real root cause of what I'm observing is that a pointer obtained through a Here is a godbolt demo for the safe vs unsafe casting back: https://godbolt.org/z/8873Wd (For future reference, that godbolt code is archived here: https://github.com/rwgk/stuff/blob/56bfaddc45330253d5a8b1c8674db10cb9b633ad/godbolt/safe_vs_unsafe_cast_back_from_void_pointer.cpp) |
990814f
to
952086d
Compare
This comment is related to #1333, #2757, #2646. 9eea907 adds a test similar to the reproducer under #1333. Observations:
Speculative: I believe Boost.Python passes
|
9eea907
to
4b40a7c
Compare
…sfinae_hooks_only.h.
…aster for clarity.
…_uses_smart_holder_type_caster for clarity.
…ster_base_tag for simplicity.
920c633
to
4f5f441
Compare
…h (fixes MSVC and GCC failures after iwyu cleanup under PR pybind#2672).
…h (fixes MSVC and GCC failures after iwyu cleanup under PR pybind#2672).
…h (fixes MSVC and GCC failures after iwyu cleanup under PR pybind#2672).
Background:
We are working on changing PyCLIF to be based on pybind11.
pybind11 as-is (Nov 2020) does NOT support passing
unique_ptr
, by choice:https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html#std-unique-ptr
PyCLIF does: [QUESTION] How to pass std::unique_ptr ownership to class member? #2583
Removing the feature from PyCLIF would go against modern C++ best practices: [WIP] Towards a solution for custom-type rvalue arguments #2047 (comment) (It would also take weeks or even months to make the required changes in the Google codebase.)
Note also that pybind11 has this long-standing bug related to
shared_ptr
/unique_ptr
handling: pybind11 wrapped method segfaults if a function returnsunique_ptr<T>
when holder isshared_ptr<T>
#1138The purpose of this draft PR is to track experiments and observations, figuring out how to add support for modern C++ best practices involving
shared_ptr
/unique_ptr
, safe & bug free.