-
Notifications
You must be signed in to change notification settings - Fork 199
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
base: master
Are you sure you want to change the base?
Conversation
…) 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.
CI failure is the usual Windows stderr-capturing flake |
There was a problem hiding this 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). |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
(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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
5f94322
to
a7ddeb1
Compare
736a2bb
to
8129beb
Compare
f8a7212
to
eb8f4e3
Compare
c15c31c
to
41b241b
Compare
11af926
to
57a6ff0
Compare
4240a97
to
e1cb670
Compare
56d7e93
to
e80edb1
Compare
c30294a
to
af57451
Compare
d7117a4
to
983d6c0
Compare
f9e5e0b
to
30e96b7
Compare
f3e2796
to
bff96e2
Compare
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.