Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate our assumption that the base class is a primary (offset-zero) base #213

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Validate our assumption that the base class is a primary (offset-zero) base #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions include/nanobind/nb_class.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,16 @@ class class_ : public object {
sizeof...(Ts) == !std::is_same_v<Base, T> + !std::is_same_v<Alias, T>,
"nanobind::class_<> was invoked with extra arguments that could not be handled");

// Fail on virtual bases -- they need a this-ptr adjustment, but they're
// not amenable to the runtime test in the class_ constructor (because
// a C-style cast will do reinterpret_cast if static_cast is invalid).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do a static_cast and let that generate an error message then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would break nb::class_<Derived, Base> where Base is a private or ambiguous base of Derived; in such a situation, is_base_of is true but static_cast doesn't work. Currently that works fine as long as Base is the primary base of Derived. If you don't think it's important to support non-public bases, then I agree that just using static_cast seems preferable.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should go out of our way to support non-public bases. In fact, the intention of such an interface is precisely to avoid the sort of static_cast that nanobind would otherwise perform when passing Derived to functions expecting Base.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough! That should allow removing the virtual-base-specific checks.

// Primary but inaccessible (ambiguous or private) bases will also
// fail is_accessible_static_base_of; the !is_convertible option is to
// avoid mis-detecting them as virtual bases.
static_assert(!std::is_convertible_v<T*, Base*> ||
detail::is_accessible_static_base_of<Base, T>::value,
"nanobind does not support virtual base classes");

template <typename... Extra>
NB_INLINE class_(handle scope, const char *name, const Extra &... extra) {
detail::type_init_data d;
Expand All @@ -354,6 +364,13 @@ class class_ : public object {
if constexpr (!std::is_same_v<Base, T>) {
d.base = &typeid(Base);
d.flags |= (uint32_t) detail::type_init_flags::has_base;

if (uintptr_t offset = (uintptr_t) (Base*) (T*) 0x1000 - 0x1000) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the other operations in class_::class_ are noexcept, which an intentional choice to avoid C++ compilers generating unwind tables for most binding code. So putting detail::raise in here seems a little concerning (even if "disabled" by the condition in virtually all cases). Could there be a way of capturing this in a constexpr fashion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I'm not aware of any reasonable way to check the offset of a base class subobject at compile time. There is an unreasonable way:

template <class Container> struct offset_helper {
  private:
    union U {
        U() {}
        ~U() {}
        char c[sizeof(Container)];
        Container o;
    };
    static U u;
#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wundefined-var-template"
#endif
    static constexpr size_t offset(const void* loc) {
#if defined(__clang__) || __GNUC_GE__(11, 0)
        for (size_t i = 0; i != sizeof(Container); ++i) {
            if (((void const*) &(u.c[i])) == loc) {
                return i;
            }
        }
        throw "unable to compute offset of member";
#else
        return (size_t) loc - (size_t) & (u.o);
#endif
    }
  public:
    template <class Base> static constexpr size_t base_offset() {
        return offset(static_cast<const Base*>(&u.o));
    }
#ifdef __clang__
#pragma clang diagnostic pop
#endif
};

But I think this is too heavyweight for nanobind; for example, it takes compilation time that's linear in the size of the bound class (although I haven't measured the constant factor).

offsetof produces a constant expression, and as of C++17 it conditionally supports non-standard-layout classes, but I don't know of any way to get it to locate a base class subobject if you don't know a name within that class.

Would changing the raise to a fail meet your needs here?

detail::raise("nanobind::class_<>: base class %s is at offset "
"%td within %s! nanobind does not support "
"multiple inheritance", typeid(Base).name(),
offset, typeid(T).name());
}
}

if constexpr (!std::is_same_v<Alias, T>)
Expand Down
8 changes: 8 additions & 0 deletions include/nanobind/nb_traits.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,14 @@ struct detector<std::void_t<Op<Arg>>, Op, Arg>
avoid redundancy when combined with nb::arg(...).none(). */
template <typename T> struct remove_opt_mono { using type = T; };

template <typename Base, typename Derived>
struct is_accessible_static_base_of {
template <typename B = Base, typename D = Derived>
static decltype(static_cast<D*>(std::declval<B*>()), std::true_type()) check(B*);
static std::false_type check(...);
static constexpr bool value = decltype(check((Base*)nullptr))::value;
};

NAMESPACE_END(detail)

template <typename... Args>
Expand Down
19 changes: 19 additions & 0 deletions tests/test_classes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,4 +477,23 @@ NB_MODULE(test_classes_ext, m) {
m.def("polymorphic_factory_2", []() { return (PolymorphicBase *) new AnotherPolymorphicSubclass(); });
m.def("factory", []() { return (Base *) new Subclass(); });
m.def("factory_2", []() { return (Base *) new AnotherSubclass(); });

// Test detection of unsupported base/derived relationships
// (nanobind requires base subobjects to be at offset zero in the
// derived class)
struct DerivedBecomesPolymorphic : Struct {
virtual ~DerivedBecomesPolymorphic() {}
};
struct DerivedNonPrimary : Big, Struct {};
struct DerivedVirt : virtual Struct {};
m.def("bind_newly_polymorphic_subclass", []() {
nb::class_<DerivedBecomesPolymorphic, Struct>(
nb::handle(), "DerivedBecomesPolymorphic");
});
m.def("bind_subclass_with_non_primary_base", []() {
nb::class_<DerivedNonPrimary, Struct>(nb::handle(), "DerivedNonPrimary");
});
static_assert(!nb::detail::is_accessible_static_base_of<Struct, DerivedVirt>::value);
// this fails at compile time:
//nb::class_<DerivedVirt, Struct>(nb::handle(), "DerivedVirt");
}
7 changes: 7 additions & 0 deletions tests/test_classes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,3 +631,10 @@ def name(self):
assert t.go(d2) == 'Rufus says woof'
finally:
t.Dog.name = old


def test35_non_primary():
with pytest.raises(RuntimeError, match="not support multiple inheritance"):
t.bind_newly_polymorphic_subclass()
with pytest.raises(RuntimeError, match="not support multiple inheritance"):
t.bind_subclass_with_non_primary_base()