Detect virtual bases as non-simple types

We currently automatically detect multiple inheritance (when registered
with pybind) to enable non-simple casting, but simple casting is also
invalid when there is single-base, virtual inheritance (#1256).  (This
happens to work under gcc/clang, but makes MSVC segfault when upcasting).

This commit adds detection for a specified base class being a virtual
base, and if so, turns on the `multiple_inheritance` flag to mark the
class and base as non-simple, and adds tests (which segfault under MSVC
without this fix).

(Earlier versions can work around the issue by explicitly passing a
`py::multiple_inheritance{}` option to the `py::class_<...>` constructor).

Co-authored-by: Jason Rhinelander <[email protected]>

Author: yeganer

include/pybind11/detail/common.h

@@ -592,6 +592,9 @@ template <typename Base, typename Derived> using is_strict_base_of = bool_consta
 template <typename Base, typename Derived> using is_accessible_base_of = bool_constant<
     std::is_base_of<Base, Derived>::value && std::is_convertible<Derived *, Base *>::value>;
 
+// Related to the above is also `is_virtual_base_of<Base, Derived>`; but it's provided in
+// `detail/is_virtual_base_of.h`
+
 template <template<typename...> class Base>
 struct is_template_base_of_impl {
     template <typename... Us> static std::true_type check(Base<Us...> *);

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 <[email protected]>
+
+    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 <typename Base, typename Derived, bool is_strict_base> 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 <typename Base, typename Derived> struct is_virtual_base_of_impl<Base, Derived, false> : std::false_type {};
+
+/// is_virtual_base_of<Base, Derived>::value is true if and only if Base is a virtual base of Derived.
+template <typename Base, typename Derived>
+using is_virtual_base_of = bool_constant<is_virtual_base_of_impl<Base, Derived, is_strict_base_of<Base, Derived>::value>::value>;
+
+NAMESPACE_END(detail)
+NAMESPACE_END(PYBIND11_NAMESPACE)
+
+#if defined(_MSC_VER)
+#  pragma warning(pop)
+#endif

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<Base *>(reinterpret_cast<type *>(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<Base, type>::value)
+            rec.multiple_inheritance = true;
     }
 
     template <typename Base, detail::enable_if_t<!is_base<Base>::value, int> = 0>

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_<Interface1>(m, "Interface1").def("run", &Interface1::run);
+    py::class_<Final, Interface1>(m, "Final").def(py::init<>());
+
+    py::class_<Interface2>(m, "Interface2").def("run", &Interface2::run);
+    py::class_<B_Concrete, Interface2>(m, "B_Concrete");
+    py::class_<C_Concrete, Interface2>(m, "C_Concrete");
+    py::class_<Diamond, B_Concrete, C_Concrete>(m, "Diamond").def(py::init<>());
+
+    m.def("run_virtual_inheritance", []() { Diamond d; return d.run(&d); },
+        "Runs the diamond test in c++");
 }
 
 

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