Skip to content

Using dynamic_cast<AliasType> to determine pointee_depends_on_holder_owner. #2910

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

Merged
merged 7 commits into from
Mar 19, 2021

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Mar 18, 2021

This PR is a follow-on to PR #2902, implementing a refinement of a safety guard that previously prevented not only unsafe behavior, but also safe behavior. All relevant situations are exercised by the newly added test_class_sh_virtual_py_cpp_mix.py. Specifically, e.g. m.get_from_cpp_unique_ptr(m.Base()) fails before this PR.

The critical change in this PR is in smart_holder_type_casters.h:

-        v_h.holder<holder_type>().pointee_depends_on_holder_owner = has_alias;
+        v_h.holder<holder_type>().pointee_depends_on_holder_owner
+        = dynamic_raw_ptr_cast_if_possible<AliasType>(v_h.value_ptr<WrappedType>()) != nullptr;

has_alias is true for any instance of a py::class_ with an alias class (aka trampoline). E.g. it is true for m.Base() and PyBase() in test_class_sh_virtual_py_cpp_mix.py. The newly used dynamic_cast-based test is true only for PyBase().

The idea for using a dynamic_cast here goes back to:

return dynamic_cast<Alias<Class> *>(ptr) != nullptr;

The exception message for the safety guard (also in smart_holder_type_casters.h) was made more precise and helpful:

         if (self_life_support == nullptr && holder().pointee_depends_on_holder_owner) {
-            throw value_error("Ownership of instance with virtual overrides in Python cannot be "
-                             "transferred to C++.");
+            throw value_error("Alias class (also known as trampoline) does not inherit from "
+                              "py::virtual_overrider_self_life_support, therefore the "
+                              "ownership of this instance cannot safely be transferred to C++.");
         }

Not directly related to the behavior change, py::detail::virtual_overrider_self_life_support was moved to py::virtual_overrider_self_life_support, to reflect that it is part of the public interface.

The behavior change under this PR has an effect on test_class_sh_trampoline_shared_ptr_cpp_arg.py, which was adopted from PR #2839 previously. test_shared_ptr_alias_nonpython is now in line with the behavior under PR #2839, but test_shared_ptr_arg_identity behaves differently here. This may need another look, but it appears that the has_alias/is_alias logic under PR #2839 may need a refinement similar to what is implemented in this PR.

@rwgk rwgk force-pushed the sh_virtual_py_cpp_mix branch from 05cab00 to cac54ba Compare March 19, 2021 03:06
rwgk added 2 commits March 19, 2021 10:47
…ling for To = void. Adding corresponding missing test in test_class_sh_virtual_py_cpp_mix. Moving dynamic_raw_ptr_cast_if_possible to separate header.
@rwgk
Copy link
Collaborator Author

rwgk commented Mar 19, 2021

The CI is green, also with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT (#2879). Google-internal testing is also green. Merging here to unblock follow-on PyCLIF work. This change will get reviewed internally. I'll open a new PR if there are requests for changes.

@rwgk rwgk marked this pull request as ready for review March 19, 2021 19:18
@rwgk rwgk merged commit 5319ca3 into pybind:smart_holder Mar 19, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Mar 19, 2021
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Mar 19, 2021
@rwgk rwgk deleted the sh_virtual_py_cpp_mix branch March 19, 2021 19:19
rwgk added a commit to google/clif that referenced this pull request Mar 19, 2021
pybind/pybind11#2910

Also importing a few other changes from smart_holder branch, to bring third_party/pybind11 in sync with HEAD.

Two tests under third_party/clif have to be adjusted in connection with PR #2910.

The TGP test is for an intermediate snapshot of this CL, but it is highly unlikely that the changes made after that snapshot have any impact outside third_party/pybind11 or third_party/clif.

Manual retesting succeeds:
```
blaze test third_party/pybind11/... third_party/pybind11_abseil/... third_party/pybind11_protobuf/... third_party/clif/...
```

http://sponge2/c54daa2b-9216-4898-82ae-9d5ac6c8ff26

When testing with ASAN all test pass, except one breakage unrelated to this CL:

http://sponge2/03bd6edc-7b01-4b32-939d-fc760d81eaa0

The ASAN breakage was introduced with cl/363927286 and is being worked on separately.

  - 784092dfd29d861af6fa611c6920c172389d97c4 Missing cast from const unique_ptr& (#2904) by Robert Haschke <rhaschke@users.noreply.github.com>
  - 2ada792085777d9cc8b4006b60476f4a103b6870 Pure clang-format cleanup (after #2904), NO other changes. by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 245d31cb0369fa51d94641853bb7beb6ed144b4f Renaming PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS to PY... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 62a8d35831525d5dcc4f6a53717d36a11c143f2a Fixing oversight: clang-format pybind11.h (affecting smar... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 3f35af74410dcace27b6cf9e8203934e13a32249 Adding smart_holder branch to workflow_dispatch in all pl... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 5319ca3817c77bb1fae233086ceeb71ec142c36f Using `dynamic_cast<AliasType>` to determine `pointee_dep... by Ralf W. Grosse-Kunstleve <rwgk@google.com>

PiperOrigin-RevId: 363957176
@EricCousineau-TRI EricCousineau-TRI added the smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst label Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants