[smart_holder] diamond inheritance problem

[virtuald]

pybind11 has support for multiple inheritance, and there are some tests for it... but it doesn't seem to work for me. My questions are:

• Is this supposed to work? (I think it is)
• Am I doing it right? (I think so, but it doesn't work)

I'm currently using the smart_holder branch, and I use -DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT=1

Here's the testcase I wrote:

#pragma once

namespace inheritance {

struct MVIBase {

  virtual ~MVIBase() = default;

  int i = 1;

  int base_getI() { return i; }
};

struct MVIChild : virtual MVIBase {
  int ci = 2;

  int child_getI() { return i; }
  int child_getCI() { return ci; }
};

struct MVIGChildA : virtual MVIChild {
  int cia = 3;

  int gchildA_getI() { return i; }
  int gchildA_getCI() { return ci; }
  int gchildA_getCIA() { return cia; }
};

struct MVIGChildB : virtual MVIChild {
  int cib = 4;

  int gchildB_getI() { return i; }
  int gchildB_getCI() { return ci; }
  int gchildB_getCIB() { return cib; }
};

struct MVIGGChild : virtual MVIGChildA, virtual MVIGChildB {
  int gi = 5;

  int ggchild_getI() { return i; }
  int ggchild_getCI() { return ci; }
  int ggchild_getCIA() { return cia; }
  int ggchild_getCIB() { return cib; }
  int ggchild_getGI() { return gi; }
};

struct MVIGGGChild : virtual MVIGGChild {

  int gggchild_getI() { return i; }
  int gggchild_getCI() { return ci; }
  int gggchild_getCIA() { return cia; }
  int gggchild_getCIB() { return cib; }
  int gggchild_getGI() { return gi; }
};

} // namespace inheritance

And my bindings:

struct rpybuild_mvi_initializer {

  py::class_<typename inheritance::MVIBase> cls_MVIBase;

  py::class_<typename inheritance::MVIChild, MVIBase> cls_MVIChild;

  py::class_<typename inheritance::MVIGChildA, MVIChild> cls_MVIGChildA;

  py::class_<typename inheritance::MVIGChildB, MVIChild> cls_MVIGChildB;

  py::class_<typename inheritance::MVIGGChild, MVIGChildA, MVIGChildB>
      cls_MVIGGChild;

  py::class_<typename inheritance::MVIGGGChild, MVIGGChild> cls_MVIGGGChild;

  py::module &m;

  rpybuild_mvi_initializer(py::module &m)
      :

        cls_MVIBase(m, "MVIBase", py::multiple_inheritance()),

        cls_MVIChild(m, "MVIChild", py::multiple_inheritance()),

        cls_MVIGChildA(m, "MVIGChildA", py::multiple_inheritance()),

        cls_MVIGChildB(m, "MVIGChildB", py::multiple_inheritance()),

        cls_MVIGGChild(m, "MVIGGChild", py::multiple_inheritance()),

        cls_MVIGGGChild(m, "MVIGGGChild", py::multiple_inheritance()),

        m(m) {}

  void finish() {

    cls_MVIBase.def(py::init<>(), release_gil())
        .def("base_getI", &inheritance::MVIBase::base_getI, release_gil())

        .def_readwrite("i", &inheritance::MVIBase::i);

    cls_MVIChild.def(py::init<>(), release_gil())
        .def("child_getI", &inheritance::MVIChild::child_getI, release_gil())

        .def("child_getCI", &inheritance::MVIChild::child_getCI, release_gil())

        .def_readwrite("ci", &inheritance::MVIChild::ci);

    cls_MVIGChildA.def(py::init<>(), release_gil())
        .def("gchildA_getI", &inheritance::MVIGChildA::gchildA_getI,
             release_gil())

        .def("gchildA_getCI", &inheritance::MVIGChildA::gchildA_getCI,
             release_gil())

        .def("gchildA_getCIA", &inheritance::MVIGChildA::gchildA_getCIA,
             release_gil())

        .def_readwrite("cia", &inheritance::MVIGChildA::cia);

    cls_MVIGChildB.def(py::init<>(), release_gil())
        .def("gchildB_getI", &inheritance::MVIGChildB::gchildB_getI,
             release_gil())

        .def("gchildB_getCI", &inheritance::MVIGChildB::gchildB_getCI,
             release_gil())

        .def("gchildB_getCIB", &inheritance::MVIGChildB::gchildB_getCIB,
             release_gil())

        .def_readwrite("cib", &inheritance::MVIGChildB::cib);

    cls_MVIGGChild.def(py::init<>(), release_gil())
        .def("ggchild_getI", &inheritance::MVIGGChild::ggchild_getI,
             release_gil())

        .def("ggchild_getCI", &inheritance::MVIGGChild::ggchild_getCI,
             release_gil())

        .def("ggchild_getCIA", &inheritance::MVIGGChild::ggchild_getCIA,
             release_gil())

        .def("ggchild_getCIB", &inheritance::MVIGGChild::ggchild_getCIB,
             release_gil())

        .def("ggchild_getGI", &inheritance::MVIGGChild::ggchild_getGI,
             release_gil())

        .def_readwrite("gi", &inheritance::MVIGGChild::gi);

    cls_MVIGGGChild.def(py::init<>(), release_gil())
        .def_static("make_ggg", &inheritance::MVIGGGChild::make_ggg,
                    release_gil())

        .def("gggchild_getI", &inheritance::MVIGGGChild::gggchild_getI,
             release_gil())

        .def("gggchild_getCI", &inheritance::MVIGGGChild::gggchild_getCI,
             release_gil())

        .def("gggchild_getCIA", &inheritance::MVIGGGChild::gggchild_getCIA,
             release_gil())

        .def("gggchild_getCIB", &inheritance::MVIGGGChild::gggchild_getCIB,
             release_gil())

        .def("gggchild_getGI", &inheritance::MVIGGGChild::gggchild_getGI,
             release_gil())

        ;
  }

}; // struct rpybuild_mvi_initializer

Here's a script that shows the expected/actual output:

from rpytest import ft

fns = [
    ("base_getI", 1),
    ("child_getI", 1),
    ("child_getCI", 2),
    ("gchildA_getI", 1),
    ("gchildA_getCI", 2),
    ("gchildA_getCIA", 3),
    ("gchildB_getI", 1),
    ("gchildB_getCI", 2),
    ("gchildB_getCIB", 4),
    ("ggchild_getI", 1),
    ("ggchild_getCI", 2),
    ("ggchild_getCIA", 3),
    ("ggchild_getCIB", 4),
    ("ggchild_getGI", 5),
    ("gggchild_getI", 1),
    ("gggchild_getCI", 2),
    ("gggchild_getCIA", 3),
    ("gggchild_getCIB", 4),
    ("gggchild_getGI", 5),
]

attrs = [("i", 1), ("ci", 2), ("cia", 3), ("cib", 4), ("gi", 5)]

for cls in [
    ft.MVIBase,
    ft.MVIChild,
    ft.MVIGChildA,
    ft.MVIGChildB,
    ft.MVIGGChild,
    ft.MVIGGGChild,
]:
    clsname = cls.__name__
    o = cls()
    for (attr, expected) in attrs:
        actual = getattr(o, attr, None)
        if actual is not None:
            if actual == expected:
                print(f"OK: {clsname}.{attr} == {actual}")
            else:
                print(f"-FAIL: {clsname}.{attr} != {expected} (got {actual})")

    for (fnn, expected) in fns:
        fn = getattr(o, fnn, None)
        if fn:
            actual = fn()
            if actual == expected:
                print(f"OK: {clsname}.{fnn}() == {actual}")
            else:
                print(f"-FAIL: {clsname}.{fnn}() != {expected} (got {actual})")

    print()

print("Done")

And the output of that script on Linux, Python 3.10:

OK: MVIBase.i == 1
OK: MVIBase.base_getI() == 1

OK: MVIChild.i == 1
OK: MVIChild.ci == 2
OK: MVIChild.base_getI() == 1
OK: MVIChild.child_getI() == 1
OK: MVIChild.child_getCI() == 2

-FAIL: MVIGChildA.i != 1 (got 2)
OK: MVIGChildA.ci == 2
OK: MVIGChildA.cia == 3
-FAIL: MVIGChildA.base_getI() != 1 (got 2)
OK: MVIGChildA.child_getI() == 1
OK: MVIGChildA.child_getCI() == 2
OK: MVIGChildA.gchildA_getI() == 1
OK: MVIGChildA.gchildA_getCI() == 2
OK: MVIGChildA.gchildA_getCIA() == 3

-FAIL: MVIGChildB.i != 1 (got 2)
OK: MVIGChildB.ci == 2
OK: MVIGChildB.cib == 4
-FAIL: MVIGChildB.base_getI() != 1 (got 2)
OK: MVIGChildB.child_getI() == 1
OK: MVIGChildB.child_getCI() == 2
OK: MVIGChildB.gchildB_getI() == 1
OK: MVIGChildB.gchildB_getCI() == 2
OK: MVIGChildB.gchildB_getCIB() == 4

-FAIL: MVIGGChild.i != 1 (got 3)
-FAIL: MVIGGChild.ci != 2 (got 3)
OK: MVIGGChild.cia == 3
OK: MVIGGChild.cib == 4
OK: MVIGGChild.gi == 5
-FAIL: MVIGGChild.base_getI() != 1 (got 3)
-FAIL: MVIGGChild.child_getI() != 1 (got 2)
-FAIL: MVIGGChild.child_getCI() != 2 (got 3)
OK: MVIGGChild.gchildA_getI() == 1
OK: MVIGGChild.gchildA_getCI() == 2
OK: MVIGGChild.gchildA_getCIA() == 3
OK: MVIGGChild.gchildB_getI() == 1
OK: MVIGGChild.gchildB_getCI() == 2
OK: MVIGGChild.gchildB_getCIB() == 4
OK: MVIGGChild.ggchild_getI() == 1
OK: MVIGGChild.ggchild_getCI() == 2
OK: MVIGGChild.ggchild_getCIA() == 3
OK: MVIGGChild.ggchild_getCIB() == 4
OK: MVIGGChild.ggchild_getGI() == 5

-FAIL: MVIGGGChild.i != 1 (got 5)
-FAIL: MVIGGGChild.ci != 2 (got 5)
-FAIL: MVIGGGChild.cia != 3 (got 5)
-FAIL: MVIGGGChild.cib != 4 (got 1)
OK: MVIGGGChild.gi == 5
-FAIL: MVIGGGChild.base_getI() != 1 (got 5)
-FAIL: MVIGGGChild.child_getI() != 1 (got 3)
-FAIL: MVIGGGChild.child_getCI() != 2 (got 5)
-FAIL: MVIGGGChild.gchildA_getI() != 1 (got 2)
-FAIL: MVIGGGChild.gchildA_getCI() != 2 (got 3)
-FAIL: MVIGGGChild.gchildA_getCIA() != 3 (got 5)
Segmentation fault (core dumped)

If this is supposed to work, then I can dig in and figure out what's going on.

[virtuald]

@rwgk I haven't verified this (will verify tonight), but I suspect this might be a regression on the smart_holder branch, as I'm fairly certain that the last version of pybind11 I was using (2.6.x) didn't have this issue. It's not clear to me if the smart_holder branch tests actually cover the multiple inheritance tests that pybind11 already has?

Anyways, I'll dig into this tonight to see if this is in fact a regression.

[rwgk]

Hi Dustin, great to know you're looking into this, I'm keen to fix if necessary.

It's not clear to me if the smart_holder branch tests actually cover the multiple inheritance tests that pybind11 already has?

Short answer: most likely "yes".

For the smart_holder branch, the CI jobs actually run twice, once with unique_ptr as the default holder, then with smart_holder as the default holder, which will hit almost all tests, with narrowly defined exceptions that you can pin-point by looking for PYBIND11_TYPE_CASTER_BASE_HOLDER under tests/. For the "tests that pybind11 already has", currently that macro is only in test_class.cpp (5x) and test_smart_ptr.cpp (20x).

[virtuald]

Ok, thanks! I'll dig in and figure it out one way or another.

[virtuald]

I was able to verify that my testcase works with pybind11 v2.9.0 without smart holder, so it appears to be a regression in the smart_holder branch. I'll dig a bit more tonight, will try to make a PR with the testcase at a minimum.

[virtuald]

Fix is now at #3635