Multiple inheritance fixes #812
Conversation
This breaks up the instance management functions in class_support.h a little bit so that other pybind11 code can use it. In particular: - added make_new_instance() which does what pybind11_object_new does, but also allows instance allocation without `value` allocation. This lets `cast.h` use the same instance allocation rather than having its own separate implementation. - instance registration is now moved to a `register_instance()`/deregister_instance()` pair (rather than having individual code add or remove things from `registered_instances` directory). - clear_instance() does everything `pybind11_object_dealloc()` needs except for the deallocation; this is helpful for factory construction which needs to be able to replace the internals of an instance without deallocating it. - clear_instance() now also calls `dealloc` when `holder_constructed` is true, even if `value` is false. This can happen in factory construction when the pointer is moved from one instance to another, but the holder itself is only copied (i.e. for a shared_ptr holder).
…d types Properly cast `instance_type::value` before calling `shared_from_this()` inside corresponding `class_::init_holder_helper`. Previous implementation throwed `bad_weak_ptr` when initializing holder in `type_caster_generic::cast` and source object being casted is held by `std::shared_ptr`. In that case, if policy is set to `take_ownership`, `init_holder_helper` creates new shared ptr and blindly takes ownership of passed object already managed by source shared ptr. This in turn leads to premature object destruction and segfault. This partially fixes pybind#801. UPDATE: this approach is wrong and do not work with tests from PR pybind#812 which contatins better solution.
|
@jagerman I thought I have found a solution by playing with casts inside I think your proposal is versatile and address the problem in its toots. Tomorrow I'll check it in my real-world project, but it has to work. |
When we are returned a base class pointer (either directly or via shared_from_this()) we detect its runtime type (using `typeid`), then end up essentially reinterpret_casting the pointer to the derived type. This is invalid when the base class pointer was a non-first base, and we end up with an invalid pointer. We could dynamic_cast to the most-derived type, but if *that* type isn't pybind11-registered, the resulting pointer given to the base `cast` implementation isn't necessarily valid to be reinterpret_cast'ed back to the backup type. This commit removes the "backup" type argument from the many-argument `cast(...)` and instead does the derived-or-pointer type decision and type lookup in type_caster_base, where the dynamic_cast has to be to correctly get the derived pointer, but also has to do the type lookup to ensure that we don't pass the wrong (derived) pointer when the backup type (i.e. the type caster intrinsic type) pointer is needed. Since the lookup is needed before calling the base cast(), this also changes the input type to a detail::type_info rather than doing a (second) lookup in cast().
This commits adds base class pointers of offset base classes (i.e. due to multiple inheritance) to `registered_instances` so that if such a pointer is returned we properly recognize it as an existing instance. Without this, returning a base class pointer will cast to the existing instance if the pointer happens to coincide with the instance pointer, but constructs a new instance (quite possibly with a segfault, if ownership is applied) for unequal base class pointers due to multiple inheritance.
|
I found another problem here, when returning an derived instance of an unregistered type as an offset base class pointer to a registered type. We would cast to most-derived, pass that to The update moves the type decision logic out of cast entirely: we should dynamic cast if the type is polymorphic and a registered type; otherwise we don't dynamic_cast at all, keeping the |
|
All my code works even without your latest update, and update didn't break it either. My issue #801 is solved. |
|
The latest update is a bit of an obscure case: you'd have to have something like: where A is not pybind11-registered, but |
|
It's great that pybind11 finally correctly manages even such a corner cases with multiple inheritance. Thumbs up! |
This fixes two issues with multiple inheritance.
The first is that when we cast a base class pointer, we correctly detect (via a
typeid) the derived class, but then do (in essense) a reinterpret_cast on it, which is wrong when the base class is offset (due to multiple inheritance). The fix is to dynamic_cast to the most derived type before passing the pointer on.The second fix is that we can often end up with multiple Python instances around the same C++ instance when we are given a base class pointer in the same situation as above: the pointer doesn't match anything in
registered_instances, so we treat it as a new type, even though, internally, it is really wrapping the same type. The commit to fix this adds intoregistered_instancesall the base type pointers (for base types that we know about) that aren't equal to the derived pointer. This is a bit of extra work as we have to do it recursively, and both during registering the instance and deregistering, but this is only done for types that actually have multiple bases somewhere above them in their inheritance tree.Fixes #801.
@uentity - I think this should fix at least some of your multiple inheritance issues; your feedback here would be very valuable.