Fix a long-standing bug in the handling of Python multiple inheritance #30056
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
July 29, 2023 14:03
This reverts commit 4c2407d.
… *>()` introduced with PR google#2152 The `reinterpret_cast<instance *>(self)` is unsafe if `__new__` is mocked, which was actually found in the wild: the mock returned `None` for `self`. This was inconsequential because `inst` is currently cast straight back to `PyObject *` to compute `all_type_info()`, which is empty if `self` is not a pybind11 `instance`, and then `inst` is never dereferenced. However, the unsafe detour through `instance *` is easily avoided and the updated implementation is less prone to accidents while debugging or refactoring.
It turns out the previous commit message is incorrect, the `inst` pointer is actually dereferenced, in the `value_and_holder` ctor here: https://github.com/pybind/pybind11/blob/f3e0602802c7840992c97f4960515777cad6a5c7/include/pybind11/detail/type_caster_base.h#L262-L263 ``` 259 // Main constructor for a found value/holder: 260 value_and_holder(instance *i, const detail::type_info *type, size_t vpos, size_t index) 261 : inst{i}, index{index}, type{type}, 262 vh{inst->simple_layout ? inst->simple_value_holder 263 : &inst->nonsimple.values_and_holders[vpos]} {} ```
rwgk
changed the title
wangxf123456
approved these changes
Sep 12, 2023
rwgk
pushed a commit
to google/clif
that referenced
this pull request
Sep 28, 2023
…fixed). pybind11 bug fix: google/pybind11clif#30056 For the full history of the bug fix see pybind/pybind11#4762. Tested manually with `_GEN_PYBIND11 = True` in google3/devtools/clif/python/clif_build_rule.bzl;l=56;rcl=559537444: * http://sponge2/b3a0816f-9acb-4f76-aebe-4d0637ea430f PiperOrigin-RevId: 565096151
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
For full background please see pybind/pybind11#4762.
The core of the added test is this:
Without this PR, the runtime behavior depends on the order in which
PCorPPCCare first instantiated, which can be highly surprising and potentially very difficult to debug. See pybind/pybind11#4762 (comment) for a full analysis.However, use cases appear to be surprisingly rare (pybind/pybind11#4762 (comment)). The only known use cases are through PyCLIF.
More-or-less as a side-effect, this PR also:
__init__unnecessarilty.reinterpret_casthere: https://github.com/pybind/pybind11/blob/8c7b8dd0ae74b36b7d42f77b0dd4096ebb7f4ab1/include/pybind11/detail/class.h#L193Suggested changelog entry: