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

Add files for testing MI issue. #4374

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bluescarni
Copy link
Contributor

Description

Test case for the issue described in #4365 .

@rwgk
Copy link
Collaborator

rwgk commented Dec 1, 2022

Quick thoughts:

  • The reinterpret_cast you found is definitely not good.
  • But if it also fails with smart_holder, that's not the (only) problem.
  • I don't fully understand the MI code, but I do know that it uses dynamic_cast<void *> which casts to the most derived type, and then handles the Python object as an object of that type, even if the original type was a base class.

Pure speculation: is there a general problem in how the MI code handles your types?

Which leads to the question: do you also see a problem if you use raw pointers instead of shared_ptr?

@rwgk
Copy link
Collaborator

rwgk commented Dec 1, 2022

The CI seems to be mostly working (still running at this minute). That's a great start.
From here it's easy: could you please add some basic test(s) in test_mi_debug.py?
To see how many jobs are failing.
Sometimes that gives useful clues.

@bluescarni
Copy link
Contributor Author

bluescarni commented Dec 1, 2022

Quick thoughts:

* The `reinterpret_cast` you found is definitely not good.

* But if it also fails with `smart_holder`, that's not the (only) problem.

* I don't fully understand the MI code, but I do know that it uses `dynamic_cast<void *>` which casts to the most derived type, and then handles the Python object as an object of that type, even if the original type was a base class.

I am not sure whether or not this is a problem specific to MI. I think this has more to do with a situation in which, for the same object, the pointer to base differs from the pointer to derived, and MI might be only an easy way to trigger this occurrence.

My understanding of what is going on largely agrees with yours: pybind11 infers the most derived type of the pointer to the base object, somehow encodes this type information somewhere and concludes that the two shared pointers to Base and Derived have the same most derived type.

Then, completely inexplicably to me, a direct, unchecked reinterpret_cast from std::shared_ptr<Base> to std::shared_ptr<Derived> is done. This is ALWAYS undefined behaviour (because the cast is done directly on the holder types, not on the internal pointers), it just happens to work in practice if the numerical values of the base and derived pointers are identical (as it is often the case for simple class hierarchies, though it is nowhere guaranteed in the C++ standard).

Thus, my impression is that automatic Base <-> Derived conversions of smart pointers in pybind11 might have always been broken from the C++ point of view, they just happen to work in practice in many cases due to implementation-defined behaviour.

@bluescarni
Copy link
Contributor Author

Which leads to the question: do you also see a problem if you use raw pointers instead of shared_ptr?

I don't know about raw pointers, but if I change the function to take as input a reference to Derived (rather than a shared pointer to Derived), everything works fine and the pointer values are correct.

@rwgk
Copy link
Collaborator

rwgk commented Dec 1, 2022

conversions of smart pointers in pybind11 might have always been broken from the C++ point of view

Yes, true. That's why I had no choice but to do something else, which became the smart_holder branch.

I'm very worried that you're seeing a problem also with the smart_holder branch. I need to find a block of time to debug, that's the biggest problem right now.

I don't know about raw pointers, but if I change the function to take as input a reference to Derived (rather than a shared pointer to Derived), everything works fine and the pointer values are correct.

That's an indication that the "up-down-sideways" cast code (which is ~completely independent of the smart pointer handling code) is working correctly.

But then again, why does the smart_holder not work for you? We need to find out.

rwgk added a commit to rwgk/pybind11 that referenced this pull request Dec 2, 2022
rwgk added a commit to rwgk/pybind11 that referenced this pull request Dec 3, 2022
rwgk added a commit to rwgk/pybind11 that referenced this pull request Dec 5, 2022
rwgk added a commit to rwgk/pybind11 that referenced this pull request Dec 9, 2022
rwgk added a commit that referenced this pull request Dec 13, 2022
* Content of PR #4374 applied on top of smart_holder branch.

* More tests, with USE_SH switch. [ci skip]

* Use `std::dynamic_pointer_cast<Base0>` [ci skip]

* All tests pass when using `m.make_derived_as_base0_raw_ptr()`, with `USE_SH` defined or not defined. [ci skip]

* WIP

* Debug LOOOK & one-line bug fix:

```diff
-        auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(src);
+        auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(std::shared_ptr<void>(src, const_cast<void *>(st.first)));
```

* Remove all print LOOOK and clang-format the fix.

* Resolve clang-tidy errors.

* Systematic test matrix.

* Bug fix in `smart_holder_type_caster<std::unique_ptr<T, D>>::cast()`

* Rename: test_mi_debug -> test_class_sh_mi_thunks

* Add `test_ptrdiff_derived_base0()`

* Miscellaneous polishing (naming, comments). No functional changes.

* Improve test_class_sh_mi_thunks.py implementation. No change in test coverage.

* Resolve clang-tidy error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants