[smart_holder] fix smart_holder regressions with multiple inheritance #3635

virtuald · 2022-01-21T08:52:19Z

Description

The smart_holder does not handle multiple inheritance correctly, and this test demonstrates that. The tests work fine with normal holders.

Investigation so far

I believe I've identified the source of the problem, but I don't have any good ideas for a fix at the present moment (@rwgk if you have ideas but not time to implement them, let me know and I can give it a shot).

My gdb investigation was done using this python code:

from pybind11_tests import multiple_inheritance as m
o = m.MVF()
assert o.b == 1

Lots of stuff happens, and at some point modified_type_caster_generic_load_impl::load_impl gets called (smart_holder_type_casters.h:162). This eventually calls try_implicit_casts, which recursively tries to convert to the correct type.

If you compare to the original type caster, the modified smart holder caster does basically the same logic, with a key difference:

I believe the original type caster recursives until it finds the source type, then converts each value until it gets to the target type (MVF to MVE, MVE to MVD0, MVD0 to MVC, MVC to MVB) and so the final value is an MVB, which is stored in value and is the correct type
The smart_holder type caster only stores the implicit_cast, but does not convert the value

This is bad because at some point smart_holder_type_caster_load::convert_type on line 525 calls the implicit_cast
function from the modified_type_caster_generic_load_impl.

The implicit_cast function is a lambda from pybind11.h:1489

return static_cast<Base *>(reinterpret_cast<type *>(src));

If I'm interpreting my GDB correctly, Base is MVB and type is MVC when it's called in this case.

Which is not what it should be doing. I think (right? since src isn't a MVC, it's an MVF?).

What it probably should be doing is recursively calling the implicit cast function on the value, converting MVF to MVE, MVE to MVD0 .. etc, just like the original caster did. Or maybe it should also be storing the converted value? It's not 100% clear to me why it's different here, but I'm guessing it has something to do with ownership.

rwgk · 2022-01-21T18:09:49Z

Thanks a lot Dustin! It looks complicated, I'll need to carve out some time to dig into this. Do you see a way to reduce the new test, based on your analysis? That would make it easier for me to get my head around the problem. But I'll look into this regardless, asap.

virtuald · 2022-01-21T19:17:39Z

Looking at the rest results, it seems that you only need to keep MVB, MVC, and MVD0 to reproduce the problem -- so you could delete MVD1, MVE, MVF.

When the issue is finally resolved, I think it would be good to keep the whole test.

virtuald · 2022-01-22T06:13:54Z

I believe I have a fix for the issue, using my original intuition stated above. This creates a std::vector of implicit casters that is appended to during the recursive type caster walk, and once it's time to do the actual cast it calls them in order.

I don't really like adding the std::vector to the type caster, but it's simplest? However, it's getting allocated every time we create a type caster, so it's probably a bad idea.

rwgk · 2022-01-22T06:59:07Z

Awesome, thanks! I can only quickly look right now (late night here). It looks good at first glance. I don't think the overhead matters much (we could run ubench/holder_comparison.cpp to get basic timings), and maybe we can find tricks or compromises to minimize the overhead. I'll play with this as soon as I get a chance; it's at the top of the pile.

virtuald · 2022-01-22T08:26:46Z

include/pybind11/detail/smart_holder_type_casters.h

@@ -251,7 +253,7 @@ class modified_type_caster_generic_load_impl {
    const std::type_info *cpptype = nullptr;
    void *unowned_void_ptr_from_direct_conversion = nullptr;
    const std::type_info *loaded_v_h_cpptype = nullptr;
-    void *(*implicit_cast)(void *) = nullptr;
+    std::vector<void *(*)(void *)> implicit_casts;
    value_and_holder loaded_v_h;
    bool reinterpret_cast_deemed_ok = false;


Should the safety guard value change due to this change?

Should the safety guard value change due to this change?

I'm thinking not. The safety guard was meant to be a very targeted sanity check, not a version check.

I've ~neglected versioning of the smart_holder internals because Google-internally we're basically building everything from sources, for each test or executable independently & hermetically. But for more safety externally, It's probably a good idea to give smart_holder internal versioning a little more thought; in a separate PR.

I'll rerun the ubench timings asap. Not sure if I get a large-enough time window over the weekend, but Monday almost certainly.

rwgk · 2022-01-25T07:57:50Z

Hi Dustin, some observations and questions:

I imported your PR into the Google dev environment and tested with the ASAN address sanitizer (standard procedure for all new code) and it's healthy as expected. — Caveat: I didn't get to run the global testing yet. I just initiated an overnight run. (I don't expect issues.)
I blindly ran the ubench timings and at worst I saw a 2% slowdown for what it exercises, more or less in the noise. (I hadn't done this for many months and wanted to get fresh numbers with and without your change.)
Then I commented out all implicit_cast code and the ubench timings ran anyway (I'm not surprised), so the only overhead could be from the extra std::vector member, but I doubt it. I'm tempted to take a shortcut and simply claim that the overhead is entirely insignificant. At worst I'm slightly wrong. Clearly, correct behavior is far more important. We can optimize later if we ever get the feeling it's worth it.
Could you please create a PR against master, with only the added tests? — My opinion is: clearly whatever we do in the future with casters, we want those test. — Small suggestion about naming: a name different than another_diamond would be nice, something like implicit_cast_diamond maybe, or pr3635_diamond, to help future readers of the code to easily connect back to useful information.
After the test is merged on master, I'll merge master into smart_holder, then we can rebase here, then merge this PR.
The // TODO: should this be a copy or move? is completely fine with me, the only suggestion I have is to use SMART_HOLDER_WIP: instead of TODO.

Skylion007 · 2022-01-25T16:08:53Z

tests/test_multiple_inheritance.cpp

+        .def_readwrite("e", &MVE::e);
+    // TODO: py::multiple_inheritance is required here, but pybind11 should
+    // be able to detect this by looking at MVE which is already bound...
+    py::class_<MVF, MVE>(m, "MVF", py::multiple_inheritance())


Actually, thinking about this. Is this indicating a more subtle bug with the smart holder? Why does the type_record only register a single base in rec.bases?

pybind11/include/pybind11/pybind11.h

Line 1206 in d434b5f

if (rec.bases.size() > 1 || rec.multiple_inheritance) {

or does it not realize MVE is non-simple:

pybind11/include/pybind11/pybind11.h

Line 1210 in d434b5f

else if (rec.bases.size() == 1) {

It records that it has a single base because MVF only has a single immediate base. To get the correct behavior it would need to examine all of its bases in turn for the multiple inheritance flag.

Actually, if it only examined its immediate bases for the flag, that would fix it, because the flag would be propagated through all derived classes.

That does fix it, I'll open another PR (#3650) to have a discussion about it.

virtuald · 2022-01-25T16:38:11Z

Thanks Ralf, I'll split the PR as suggested later today/tonight.

rwgk · 2022-01-25T19:34:17Z

Thanks Ralf, I'll split the PR as suggested later today/tonight.

Perfect.

The global testing came back unusually noisy, but after scrolling up and down through the failures for a while I'm thinking this PR didn't cause any failures. (I'll probably retry after we're through the splitting steps, hoping for cleaner results.)

rwgk · 2022-01-27T01:33:27Z

I'm not sure how to best handle the situation that the tests are on master already. There must be a way, but how?

My usual procedure is:

[smart_holder] git merge master #3653
When the tests look good: git checkout smart_holder; git pull . sh_merge_master; git push

But if I do that now the smart_holder branch will have many broken tests.

@virtuald would it work if you did this?

git rebase smart_holder (only if necessary)
git merge master
Verify that everything looks good.
git push (possibly with force)
Wait for tests.

Then me doing this?

git checkout smart_holder; git pull . smart-holder-mvi; git push

This wouldn't squash your commits, but I think that's better than us having to coordinate.
(I think it nice/better to not squash your commits here, if we want to look back later.)
Unless there is a better way?

rwgk · 2022-01-27T01:59:07Z

Oh ... face palm.

Could you simply remove the tests from this PR?
Then I'll merge by clicking the button here. Nothing broken.
Then I go through my standard procedure to git merge master.

The original pybind11 holder supported multiple inheritance by recursively creating type casters until it finds one for the source type, then converting each value in turn to the next type via typeinfo->implicit_cast The smart_holder only stored the last implicit_cast, which was incorrect. This commit changes it to create a list of implicit_cast functions that are appended to during the recursive type caster creation, and when the time comes to cast to the destination type, it calls all of them in the correct order.

virtuald · 2022-01-27T02:00:03Z

Just saw your note. I removed the tests.

rwgk · 2022-01-27T02:15:01Z

As soon as I see that the CI is happy I'll merge.

The one failure at this moment is a known flake, good to ignore.

rwgk · 2022-01-27T03:02:36Z

Thanks a lot Dustin! Merging now.

virtuald force-pushed the smart-holder-mvi branch 2 times, most recently from 2f5ac26 to 3c287cf Compare January 21, 2022 09:02

virtuald changed the title ~~[smart_holder] Add tests demonstrating smart_holder issues with multiple inheritance~~ [smart_holder] fix smart_holder regressions with multiple inheritance Jan 22, 2022

virtuald force-pushed the smart-holder-mvi branch from 5c10569 to d9ad43c Compare January 22, 2022 06:44

virtuald force-pushed the smart-holder-mvi branch 2 times, most recently from 67b55bc to a4755b7 Compare January 22, 2022 07:34

virtuald commented Jan 22, 2022

View reviewed changes

Skylion007 reviewed Jan 25, 2022

View reviewed changes

This was referenced Jan 26, 2022

Add tests demonstrating smart_holder issues with multiple inheritance #3649

Closed

Propagate py::multiple_inheritance to all children #3650

Merged

virtuald force-pushed the smart-holder-mvi branch 2 times, most recently from bb06cf5 to 9e6ce49 Compare January 26, 2022 07:26

virtuald force-pushed the smart-holder-mvi branch from 9e6ce49 to a6c47cc Compare January 27, 2022 01:59

rwgk merged commit fd8265e into pybind:smart_holder Jan 27, 2022

github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 27, 2022

rwgk removed the needs changelog Possibly needs a changelog entry label Jan 27, 2022

virtuald deleted the smart-holder-mvi branch January 27, 2022 17:19

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3635

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[smart_holder] fix smart_holder regressions with multiple inheritance #3635

[smart_holder] fix smart_holder regressions with multiple inheritance #3635

virtuald commented Jan 21, 2022

rwgk commented Jan 21, 2022

virtuald commented Jan 21, 2022

virtuald commented Jan 22, 2022

rwgk commented Jan 22, 2022

virtuald Jan 22, 2022

rwgk Jan 22, 2022

rwgk commented Jan 25, 2022

Skylion007 Jan 25, 2022 •

edited

Loading

virtuald Jan 25, 2022 •

edited

Loading

virtuald Jan 26, 2022 •

edited

Loading

virtuald commented Jan 25, 2022

rwgk commented Jan 25, 2022

rwgk commented Jan 27, 2022

rwgk commented Jan 27, 2022

virtuald commented Jan 27, 2022

rwgk commented Jan 27, 2022

rwgk commented Jan 27, 2022

[smart_holder] fix smart_holder regressions with multiple inheritance #3635

[smart_holder] fix smart_holder regressions with multiple inheritance #3635

Conversation

virtuald commented Jan 21, 2022

Description

Investigation so far

rwgk commented Jan 21, 2022

virtuald commented Jan 21, 2022

virtuald commented Jan 22, 2022

rwgk commented Jan 22, 2022

virtuald Jan 22, 2022

Choose a reason for hiding this comment

rwgk Jan 22, 2022

Choose a reason for hiding this comment

rwgk commented Jan 25, 2022

Skylion007 Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

virtuald Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

virtuald Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

virtuald commented Jan 25, 2022

rwgk commented Jan 25, 2022

rwgk commented Jan 27, 2022

rwgk commented Jan 27, 2022

virtuald commented Jan 27, 2022

rwgk commented Jan 27, 2022

rwgk commented Jan 27, 2022

Skylion007 Jan 25, 2022 •

edited

Loading

virtuald Jan 25, 2022 •

edited

Loading

virtuald Jan 26, 2022 •

edited

Loading