-
Notifications
You must be signed in to change notification settings - Fork 77
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
Make it possible to subclass wasm C++ API classes for the implementor. #161
Conversation
Use some guarantees about standard layout of zero size classes to make a safe reinterpret_cast hierarchy so that the ExternType can find its kind.
Fix build of wasm-bin since ExternType::from was removed.
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.
Excellent, thanks a lot! The two main questions I have are:
-
Is there really no convenient way to allow implicit casting from
own<Derived>
toown<Base>
anymore? What is common C++ practice to work around that? -
Can we somehow maintain the former regularity and brevity of going from an interface type to its implementation?
} | ||
|
||
|
||
// Extern Types | ||
|
||
struct ExternTypeImpl { | ||
struct ExternTypeKind { |
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'm confused, why do we need this aux wrapper?
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 wrote the answer in response to a different comment: #161 (comment)
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.
So IIUC, you are saying that this (and the static_asserts) could be avoided if we used virtual inheritance on the Impl classes? As long as that does not leak to the .hh file and imposes no other overhead, I suppose I'd be fine with that.
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.
No, FuncType
etc. would need to virtually inherit from ExternType
too. Both inheritance edges need to be marked virtual for the bases to be merged.
https://isocpp.org/wiki/faq/multiple-inheritance#virtual-inheritance-where
src/wasm-v8.cc
Outdated
@@ -240,30 +217,22 @@ DEFINE_VEC(Val, vec, VAL) | |||
|
|||
// Configuration | |||
|
|||
struct ConfigImpl { | |||
struct ConfigImpl : public Config { |
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.
Let's either convert all these definitions to class
or drop the public
.
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.
Dropping public
breaks a lot, I've opted for making them classes.
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.
Dropping public breaks a lot, I've opted for making them classes.
I'm confused, what can it break? Isn't public simply the default for structs?
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'm not sure what I tested, maybe I tried class ConfigImpl : Config
which defaults to private inheritance. struct ConfigImpl : Config
works fine.
src/wasm-v8.cc
Outdated
@@ -642,16 +597,16 @@ struct FuncTypeImpl : ExternTypeImpl { | |||
} | |||
}; | |||
|
|||
template<> struct implement<FuncType> { using type = FuncTypeImpl; }; | |||
static_assert(std::is_standard_layout<ExternTypeImpl<FuncTypeImpl, FuncType, ExternKind::FUNC>>::value); |
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 are these assertions needed? Can they be avoided somehow?
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.
Of course asserts are just that, their only purpose is to sometimes make the build fail, so they can be removed. But in this case they're indicating something very important.
Recall my original plan was Config::method()
can safely cast Config*
to ConfigImpl*
because the only way to get a Config*
is through Config::make()
which always actually creates a ConfigImpl
?
ExternType
is the base of a type hierarchy. In C++ when you create an object, it starts by creating the base object first, so that's a second way to create ExternType
. This means that when you see an ExternType*
you don't know whether it's an ExternTypeImpl*
or a casted FuncType*
. Oops. How do you implement ExternType::methods()?
The obvious fix of saying "well, FuncTypImpl
derives from ExternTypeImpl
too" doesn't work either because now you have two distinct copies of ExternType
, and a given ExternType*
might be the one that's a base of FuncType
of FuncTypeImpl
or the one that's a base of ExternTypeImpl
of FuncTypeImpl
. Two copies of a base objects usually have to have distinct addresses (C++20 adds an attribute to permit merging them but it's merely a suggestion).
Quick aside, there's a common C++ fix for this: virtual inheritance. When inheriting virtually, you merge two copies of a base into one copy. Tada! But I'm continuing to assume we can't use virtual.
So here's what I'm going to do: we add a simple struct ExternTypeKind
that allows us to tell what the most-derived type of the object is. Once we know that, we're home free because we know what to cast to. Now, we need to be able to go from ExternType*
or FuncType*
etc. to the ExternTypeKind*
even though we don't know what casts have been performed previously. The standard term for this is pointer-interconvertible and it will allow us to use reinterpret_cast
to correctly cast between them. ExternTypeImpl
exists to follow those rules, one of which is being a standard-layout type. That's what the static assert is checking.
Then we can derive FuncTypeImpl
on top, and that can do whatever it likes including things which cause static_cast<> between FuncTypeImpl
and ExternType
to have a pointer adjustment.
Reference: https://eel.is/c++draft/basic.compound#4.3 . Also since it doesn't link to "standard-layout class": https://eel.is/c++draft/class.prop#3
include/wasm.hh
Outdated
template<class T> using ownvec = vec<own<T>>; | ||
|
||
template<class T> | ||
auto make_own(T* x) -> own<T> { return own<T>(x); } | ||
|
||
template<class To, class From> | ||
auto own_cast(own<From> x) -> own<To> { return make_own<To>(x.release()); } |
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.
auto own_cast(own<From> x) -> own<To> { return make_own<To>(x.release()); } | |
auto own_cast(own<From> x) -> own<To> { return own<To>(x.release()); } |
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 does make_own
exist? I noticed that wasm-v8.cc uses own<T>
directly and mimicked that in my changes, but in wasm.hh I thought that making perhaps a pointer should go through the make function.
Is it supposed to be a parallel to std::make_unique
? make_unique doesn't take a pointer, it takes the arguments that the T's c'tor would take and forwards them along. This wouldn't be useful to V8 because wasm-v8.cc uses new(std::nothrow)
instead of regular new
.
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've gone ahead and removed make_own
.
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.
make_own
exists for the same reason that make_pair
and friends exist in the std lib: to work around C++'s odd inability to infer template arguments for constructor invocations. So this would mostly be for convenience in user code, not necessarily the implementation.
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.
Okay, but in this case it might be confusing because it's different from make_unique<T>
while otherwise own<T>
is an alias to unique_ptr<T>
. I've put it back, but we still don't use it in wasm-v8.
Ordinarily you simply make the The trouble with a non-virtual d'tor is that you can, with no complaint from the compiler, create a I've been assuming we can't use Now,
Yep, I'll commit that.
Sorry for the confusion, I was always planning to put that back. That's part of the reason this was a draft PR.
|
template<class T> using ownvec = vec<own<T>>; | ||
|
||
template<class T> | ||
auto make_own(T* x) -> own<T> { return own<T>(x); } |
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 think this is still useful, see other comment.
} | ||
|
||
|
||
// Extern Types | ||
|
||
struct ExternTypeImpl { | ||
struct ExternTypeKind { |
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.
So IIUC, you are saying that this (and the static_asserts) could be avoided if we used virtual inheritance on the Impl classes? As long as that does not leak to the .hh file and imposes no other overhead, I suppose I'd be fine with that.
src/wasm-v8.cc
Outdated
@@ -1453,23 +1463,32 @@ auto Module::deserialize(Store* store_abs, const vec<byte_t>& serialized) -> own | |||
|
|||
|
|||
// TODO(v8): do better when V8 can do better. | |||
template<> struct implement<Shared<Module>> { using type = vec<byte_t>; }; | |||
auto impl(Shared<Module>* x) -> vec<byte_t>* { |
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.
IIUC, the implement
template no longer works for this because it requires a reinterpret_cast? Could that be fixed by introducing a SharedImpl subclass as well?
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.
You'd need to make vec<byte_t>
derive from the subclass too.
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.
No, that's not quite right. We could also have SharedImpl
derive from both Shared<Module>
and from vec<byte_t>
.
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.
Done. I made SharedImpl<T>
derive from vec<byte_t>
but when you add SharedImpl<T != Module>
you'll probably want to make that optional.
…h method declarations.
include/wasm.hh
Outdated
class Module; | ||
|
||
template<> | ||
class WASM_API_EXTERN Shared<Module> { |
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.
FYI it's possible to remove class Module;
here by instead writing:
template<>
class WASM_API_EXTERN Shared<class Module> {
I haven't done that because I expect most users of C++ to be surprised to discover that you're allowed to write a forward declaration using an elaborated type specifier inside the template argument of an explicit specialization. Regardless, it's an option that would be a little cleaner.
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 simply move the definition after Module?
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.
Shared<Module>
is used in the declaration of Module
. So either we forward declare Module
for Shared<Module>
, or we forward-declare Shared<>
for Module
. All things the same, I figured the forward declaration of a non-template is less likely to cause any confusion.
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.
Ah, right. Stylistically, it probably makes more sense to define Shared<Module>
next to Module
, but it's a minor point.
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.
Okay, I've done what I think you mean. If that isn't exactly what you meant, it's a quick PR to fix, I'd be happy to review it.
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.
Oh, I can't merge anyways. :)
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.
Looks good!
include/wasm.hh
Outdated
class Module; | ||
|
||
template<> | ||
class WASM_API_EXTERN Shared<Module> { |
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 simply move the definition after Module?
Thanks a lot! |
Fixes #119.