diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 4b2b08499d..4130d093a9 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -592,6 +592,9 @@ template using is_strict_base_of = bool_consta template using is_accessible_base_of = bool_constant< std::is_base_of::value && std::is_convertible::value>; +// Related to the above is also `is_virtual_base_of`; but it's provided in +// `detail/is_virtual_base_of.h` + template class Base> struct is_template_base_of_impl { template static std::true_type check(Base *); diff --git a/include/pybind11/detail/is_virtual_base_of.h b/include/pybind11/detail/is_virtual_base_of.h new file mode 100644 index 0000000000..e6a5a08fae --- /dev/null +++ b/include/pybind11/detail/is_virtual_base_of.h @@ -0,0 +1,48 @@ +/* + pybind11/detail/virtual_base.h -- provides detail::is_virtual_base_of + + Copyright (c) 2018 Jason Rhinelander + + All rights reserved. Use of this source code is governed by a + BSD-style license that can be found in the LICENSE file. +*/ + +#pragma once + +#include "common.h" + +// This metaprogramming template is in its own header because the only way to make gcc not generate +// a warning for the approach used to detect virtual inheritance (which comes from boost) is to tell +// GCC that this is a system header. + +#if defined(_MSC_VER) +# pragma warning(push) +# pragma warning(disable: 4250) // warning C4250: 'X': inherits 'Y::member' via dominance +# pragma warning(disable: 4584) // warning C4584: base-class 'X' is already a base-class of 'Y' +# pragma warning(disable: 4594) // warning C4594: indirect virtual base class is inaccessible +#elif defined(__GNUG__) +// Lie to GCC that this is a system header so as to not generated the unconditional warning for the +// inaccessible base caused by the implementation below when the base class is *not* virtual. +# pragma GCC system_header +#endif + +NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +NAMESPACE_BEGIN(detail) + +template struct is_virtual_base_of_impl { + struct X : Derived, virtual Base { char data[1024]; }; + struct Y : Derived { char data[1024]; }; + constexpr static bool value = sizeof(X) == sizeof(Y); +}; +template struct is_virtual_base_of_impl : std::false_type {}; + +/// is_virtual_base_of::value is true if and only if Base is a virtual base of Derived. +template +using is_virtual_base_of = bool_constant::value>::value>; + +NAMESPACE_END(detail) +NAMESPACE_END(PYBIND11_NAMESPACE) + +#if defined(_MSC_VER) +# pragma warning(pop) +#endif diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 977045d623..b6bcb8e110 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -44,6 +44,7 @@ #include "options.h" #include "detail/class.h" #include "detail/init.h" +#include "detail/is_virtual_base_of.h" NAMESPACE_BEGIN(PYBIND11_NAMESPACE) @@ -1075,6 +1076,12 @@ class class_ : public detail::generic_type { rec.add_base(typeid(Base), [](void *src) -> void * { return static_cast(reinterpret_cast(src)); }); + + // If the specified base class is a virtual base class, flip on the multiple inheritance + // flag. Even if, technically, multiple inheritance isn't actually involved, we still have + // to resort to the same non-simple casting to get to the base class. + if (detail::is_virtual_base_of::value) + rec.multiple_inheritance = true; } template ::value, int> = 0> diff --git a/tests/test_virtual_functions.cpp b/tests/test_virtual_functions.cpp index 336f5e45c6..ee92d50bce 100644 --- a/tests/test_virtual_functions.cpp +++ b/tests/test_virtual_functions.cpp @@ -289,6 +289,53 @@ TEST_SUBMODULE(virtual_functions, m) { // .def("str_ref", &OverrideTest::str_ref) .def("A_value", &OverrideTest::A_value) .def("A_ref", &OverrideTest::A_ref); + + // test_virtual_inheritance: + // Interface class, not meant to be instantiated + class Interface1 { + public: + virtual ~Interface1() = default; + virtual int run(Interface1*) = 0; + virtual int number() = 0; + }; + class Final: public virtual Interface1 { + public: + virtual int run(Interface1* ptr) { return ptr->number() + 1; } + virtual int number() { return 5; } + }; + + // Duplicate of the above (but we want a distinct type for the test) + class Interface2 { + public: + virtual ~Interface2() = default; + virtual int run(Interface2*) = 0; + virtual int number() = 0; + }; + + class B_Concrete : public virtual Interface2 { + public: + virtual int run(Interface2* ptr) { return ptr->number() + 2; } + }; + class C_Concrete : public virtual Interface2 { + public: + virtual int number() { return 2; } + }; + + #if defined(_MSC_VER) + # pragma warning(disable: 4250) // warning C4250 'Derived' inherits 'Base::method' via dominance + #endif + class Diamond : public C_Concrete, public B_Concrete { }; + + py::class_(m, "Interface1").def("run", &Interface1::run); + py::class_(m, "Final").def(py::init<>()); + + py::class_(m, "Interface2").def("run", &Interface2::run); + py::class_(m, "B_Concrete"); + py::class_(m, "C_Concrete"); + py::class_(m, "Diamond").def(py::init<>()); + + m.def("run_virtual_inheritance", []() { Diamond d; return d.run(&d); }, + "Runs the diamond test in c++"); } diff --git a/tests/test_virtual_functions.py b/tests/test_virtual_functions.py index 2a92476f99..8ea58bdfb7 100644 --- a/tests/test_virtual_functions.py +++ b/tests/test_virtual_functions.py @@ -369,3 +369,17 @@ def lucky_number(self): assert obj.unlucky_number() == -7 assert obj.lucky_number() == -1.375 assert obj.say_everything() == "BT -7" + + +def test_virtual_inheritance(): + """Issue #1256 - single inheritance from a virtual base class breaks upcasting""" + # This test only seems to fail (via segfault) under MSVC, but it's not an MSVC problem: we were + # previously erroneously treating a virtual base class as simple inheritance, but it is not. + final = m.Final() + assert final.run(final) == 6 + + # The diamond inheritance case here is similar, but wasn't actually an issue: pybind seeing the + # multiple inheritance already triggers the non-simple inheritance path: + diamond = m.Diamond() + assert diamond.run(diamond) == 4 + assert m.run_virtual_inheritance() == 4