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.

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

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

Conversation

oremanj
Copy link
Contributor

@oremanj oremanj commented May 12, 2023

This is easy to do at ~compile-time if the base is specified as a template argument. (It's not legal to reinterpret_cast in a constant expression, so the error is reported at runtime, but the test and error message are expected to be compiled out if the test doesn't fail, and I checked this on my system.)

Also detect and complain about accessible virtual bases at compile time. I'm not aware of a way to detect inaccessible virtual bases either at compile time or runtime, at least not without obtaining an instance pointer somehow.

It is sort of possible to validate these things at runtime if the base is specified as a Python object, but much more difficult (requires mucking around with implementation-defined subclasses of std::type_info). I can add an implementation of that for libstdc++ on Linux if desired, but it costs about 400 bytes in libnanobind and I don't know how to do it on other platforms. It didn't seem worth it to me.

…) base

This is easy to do at ~compile-time if the base is specified as a template argument. (It's not legal to `reinterpret_cast` in a constant expression, so the error is reported at runtime, but the test and error message are expected to be compiled out if the test doesn't fail.)

Also detect accessible virtual bases at compile time. (I'm not aware of a way to detect inaccessible virtual bases either at compile time or runtime, at least not without obtaining an instance pointer somehow.)

It is sort of possible to validate this if the base is specified as a Python object, but much more difficult (requires mucking around with implementation-defined subclasses of `std::type_info`). I can add an implementation of that for libstdc++ on Linux if desired, but it costs about 400 bytes in libnanobind and I don't know how to do it on other platforms. It didn't seem worth it to me.
@oremanj
Copy link
Contributor Author

oremanj commented May 12, 2023

CI failure is the usual Windows stderr-capturing flake

Copy link
Owner

@wjakob wjakob left a comment

Choose a reason for hiding this comment

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

A little worried that this will have a compile-time cost. Perhaps those checks could only be done in debug mode compiles?

@@ -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.

@wjakob
Copy link
Owner

wjakob commented May 22, 2023

(FYI, I am working towards a conference deadline and will get back to reviewing PRs in a few days -- sorry for the delay)

@@ -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?

@wjakob wjakob force-pushed the master branch 5 times, most recently from 5f94322 to a7ddeb1 Compare June 15, 2023 23:32
@wjakob wjakob force-pushed the master branch 5 times, most recently from 736a2bb to 8129beb Compare August 14, 2023 15:42
@wjakob wjakob force-pushed the master branch 3 times, most recently from f8a7212 to eb8f4e3 Compare August 23, 2023 07:56
@wjakob wjakob force-pushed the master branch 6 times, most recently from c15c31c to 41b241b Compare September 12, 2023 05:19
@wjakob wjakob force-pushed the master branch 5 times, most recently from 11af926 to 57a6ff0 Compare October 6, 2023 10:26
@wjakob wjakob force-pushed the master branch 10 times, most recently from 4240a97 to e1cb670 Compare March 3, 2024 19:35
@wjakob wjakob force-pushed the master branch 4 times, most recently from 56d7e93 to e80edb1 Compare March 11, 2024 17:04
@wjakob wjakob force-pushed the master branch 4 times, most recently from c30294a to af57451 Compare March 22, 2024 08:46
@wjakob wjakob force-pushed the master branch 4 times, most recently from d7117a4 to 983d6c0 Compare May 22, 2024 15:28
@wjakob wjakob force-pushed the master branch 2 times, most recently from f9e5e0b to 30e96b7 Compare September 9, 2024 14:54
@wjakob wjakob force-pushed the master branch 2 times, most recently from f3e2796 to bff96e2 Compare October 4, 2024 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants