Embind port

[kripken]

For #6225

Basic stuff works, but I am having trouble with the return policy @brendandahl

I am doing

#define DELEGATE_FIELD_NAME_VECTOR(id, field)                                  \
  .property(#field, &id::field, return_value_policy::reference())

Reference is what I want for fields which are vectors of names - we don't want to copy them, and they'll be around while the object is. But I get

emscripten/cache/sysroot/include/emscripten/bind.h:794:20: error: object of type 'ArenaVector<wasm::Name>' cannot be assigned because its copy assignment operator is implicitly deleted
  794 |         ptr.*field = MemberBinding::fromWireType(value);
      |                    ^
emscripten/cache/sysroot/include/emscripten/bind.h:1870:69: note: in instantiation of function template specialization 'emscripten::internal::MemberAccess<wasm::Switch, ArenaVector<wasm::Name>>::setWire<wasm::Switch>' requested here
 1870 |         auto setter = &MemberAccess<ClassType, FieldType>::template setWire<ClassType>;
      |                                                                     ^
3-binaryen/src/wasm-delegations-fields.def:287:1: note: in instantiation of function template specialization 'emscripten::class_<wasm::Switch, emscripten::base<wasm::Expression>>::property<ArenaVector<wasm::Name>, emscripten::return_value_policy::reference, void>' requested here
  287 | DELEGATE_FIELD_SCOPE_NAME_USE_VECTOR(Switch, targets)
      | ^
3-binaryen/src/binaryen-embind.cpp:99:4: note: expanded from macro 'DELEGATE_FIELD_SCOPE_NAME_USE_VECTOR'
   99 |   .property(#field, &id::field, return_value_policy::reference())
      |    ^
3-binaryen/src/mixed_arena.h:402:3: note: copy assignment operator is implicitly deleted because 'ArenaVector<wasm::Name>' has a user-declared move constructor
  402 |   ArenaVector(ArenaVector<T>&& other) : allocator(other.allocator) {
      |   ^
In file included from 3-binaryen/src/binaryen-embind.cpp:21:

That requirement for a move constructor or copy assignment operator confuse me, as the docs for a return value policy of reference say "Constructor: none". What am I doing wrong?

[brendandahl]

When using .property(#field, &id::field, ...) embind creates a getter/setter method for the field. In this case you can't assign to ArenaVector<wasm::Name> in a setter. You probably don't want a setter in this case, you can do a few things:

1. Make the field const so embind only generates a getter.
2. Add a getter method:

const ArenaVector<X>* getField() const {
    return &field;
}
...
property("field", &Foo::getField, return_value_policy::reference());

[brendandahl]

I played a bit more with your example and I am now running into some other issues using a getter. Since ArenaVector extends ArenaVectorBase<ArenaVector<int>, int> the bindings for all the methods in ArenaVectorBase<ArenaVector<int>, int> need to be put in their own class_ bindings. e.g.

  class_<ArenaVectorBase<ArenaVector<int>, int>>("Base")
    .function("insertAt", &ArenaVectorBase<ArenaVector<int>, int>::insertAt) 
    .function("getAt", &ArenaVectorBase<ArenaVector<int>, int>::operator[])
  ;
  class_<ArenaVector<int>, base<ArenaVectorBase<ArenaVector<int>, int>>>("Derived");

However, if I then try to do something like arena.insertAt(...) embind then fails with Cannot convert argument of type Derived const* to parameter type Base* . This is because the getter for the arena returns a const *, but then tries to convert the pointer to the base class as a non const pointer.

I'm still thinking about how we could better handle this, but a current alternative is to not use a property binding but instead use a function binding.

class_<Foo>("Foo").function("getField", +[](Foo& foo) { return &foo.field; }, return_value_policy::reference());

[brendandahl]

I thought about this a little more and the Cannot convert argument of type... does make sense in this case. We have a const pointer to ArenaVector so it makes sense we can't call non-const method e.g. insertAt. Calling operator[] works as expected with the const pointer since it's marked const. So if you don't need to write to the vector I guess a const pointer is fine otherwise you'll need to use a function binding like I mentioned above.

[kripken]

Thanks! I'll try a function here. I don't think we need to write to the vector, which is good.

[kripken]

That gets me a little further. Now I see that some of the automation I was hoping to get will not "just work" as I'd hoped. We can automate creation of builder.makeNop and several others, in just a single line, but it fails on overloads: builder.makeBlock has several overloads, so we'd need to manually write code to pick one. Maybe that isn't so bad but it does remove some benefit here.

[GulgDev]

What is the current state of this PR?

[kripken]

I haven't had any good ideas on moving forward here, unfortunately.

Ideally someone with more expertise in embind might take a look at it. There could be some C++ template magic or embind magic that could help here, that I am not aware of.

[GulgDev]

What are the main difficulties? Also, it looks like the changes in this PR would be breaking, as they are not compatible with the current JS API.

[kripken]

The main issue is that I was hoping this would save a lot of work, but it turns out exposing the C++ API using embind is not that simple due to overloads, as mentioned in #7239 (comment)

Given that, I'm not sure if this is worth it or not. But maybe there is a nice way that I am missing.

I think a breaking change might make sense here if it had large benefits - it would be a better path forward for the long term. But I don't quite see how to get such benefits here yet.

[brendandahl]

Do you want to expose all of the makeBlock overloads? I see some are templated too, so would you want multiple makeBlocks for each unique T?

We could add overload support to embind based on type, but it would still require the user to add a binding for each one they want to expose. I'm not aware of any magic c++ templating to match all the overloads (besides using the preprocessor to generate them). I've been hesitant to do type overloading since it would be slow and doesn't really mesh with JS, but we could probably make it so it doesn't affect non-overloaded functions.

[kripken]

Do you want to expose all of the makeBlock overloads?

Sort of, yes - my native hope was that we'd expose the C++ API to JS in a simple way, as close as possible. I guess overloading does make it difficult...

If there is a better way to do these bindings, I'm open to that. In general I feel like it would be cool if Binaryen had a great bindings story to JS - it could be a showcase for all the work we've done on embind, TypeScript, etc., on the Emscripten side.