brson / wasm-opt-rs

Rust bindings for Binaryen's wasm-opt
Apache License 2.0
64 stars 10 forks source link

Miscellaneous notes on notes.md #95

Closed dtolnay closed 2 years ago

dtolnay commented 2 years ago

https://github.com/brson/wasm-opt-rs/blob/aad563327f248190a17ad48de8e0a3f667b76597/notes.md#L732-L735

It doesn't necessitate it, you can write them as self: Pin<&mut ModuleReader> instead of self: Pin<&mut Self> if you want, even with multiple extern types in the block. Using a block per type is a good idiom though.


https://github.com/brson/wasm-opt-rs/blob/aad563327f248190a17ad48de8e0a3f667b76597/notes.md#L754-L764

This sounds like after going high enough in the API, far enough removed from the original incorrectly-nonconst method, things happened to work out. It's great that it worked out but I suspect you got somewhat lucky here regarding the specific structure of this API.

Generally you would want to fix an incorrectly-nonconst situation at as low a level as possible, as close as possible to the original incorrect-nonconst C++ code. A more general purpose idiom for binding incorrectly-nonconst functions safely using cxx is to pass &UniquePtr<T> from Rust to C++, receive it as std::unique_ptr<T> const& in C++, then use T* operator->() const (https://en.cppreference.com/w/cpp/memory/unique_ptr/operator*) which turns a const unique_ptr into a non-const T.

// lib.rs

extern "C++" {
    type Thing;

    fn incorrectly_nonconst(thing: &UniquePtr<Thing>, arg: bool) -> i32;
}
// lib.cpp

int32_t incorrectly_nonconst(std::unique_ptr<Thing> const&, bool arg) {
    return thing->incorrectlyNonconst(arg);
}

https://github.com/brson/wasm-opt-rs/blob/aad563327f248190a17ad48de8e0a3f667b76597/notes.md#L906-L911

Yep, and the way to avoid a copy is by passing Pin<&mut CxxString> from Rust to C++, receiving it as std::string&, and calling std::move. let_cxx_string! (which you are already using) produces a local binding of type Pin<&mut CxxString>, so the deref from that to &CxxString in your Rust code is what forces a copy to be done afterward in C++.


https://github.com/brson/wasm-opt-rs/blob/aad563327f248190a17ad48de8e0a3f667b76597/notes.md#L912-L915

You can plug in your own catching logic to make C++ catch any kind of type on all signatures in the binding. See the very bottom of https://cxx.rs/binding/result.html. You'd stick the following somewhere in your shims.h or any other header include!ed in the FFI extern block:

namespace rust::behavior {
  template <typename Try, typename Fail>
  static void trycatch(Try &&func, Fail &&fail) noexcept try {
    func();
  } catch (const std::exception &e) {
    fail(e.what());
  } catch (const wasm::ParseException &e) {
    fail(???);
  } catch (const wasm::MapParseException &e) {
    fail(???);
  }
}
brson commented 2 years ago

This sounds like after going high enough in the API, far enough removed from the original incorrectly-nonconst method, things happened to work out. It's great that it worked out but I suspect you got somewhat lucky here regarding the specific structure of this API.

This is true. We do all our work in one big blast, in a run method. The underlying const correctness doesn't matter because it never escapes this function. I'll try to add some nuance to the text.

brson commented 2 years ago

Generally you would want to fix an incorrectly-nonconst situation at as low a level as possible, as close as possible to the original incorrect-nonconst C++ code. A more general purpose idiom for binding incorrectly-nonconst functions safely using cxx is to pass &UniquePtr<T> from Rust to C++, receive it as std::unique_ptr<T> const& in C++, then use T* operator->() const (https://en.cppreference.com/w/cpp/memory/unique_ptr/operator*) which turns a const unique_ptr into a non-const T.

I filed https://github.com/brson/wasm-opt-rs/issues/104 to try this in a future release. I think I can explain it in the blog post.

brson commented 2 years ago

Generally you would want to fix an incorrectly-nonconst situation at as low a level as possible, as close as possible to the original incorrect-nonconst C++ code. A more general purpose idiom for binding incorrectly-nonconst functions safely using cxx is to pass &UniquePtr<T> from Rust to C++, receive it as std::unique_ptr<T> const& in C++, then use T* operator->() const (https://en.cppreference.com/w/cpp/memory/unique_ptr/operator*) which turns a const unique_ptr into a non-const T.

Can this pattern be adapted to work on methods, or does this require operating only on free functions?

dtolnay commented 2 years ago

You'd need the wrapper to be a free function because C++ member functions don't have anything similar to Rust self: &Box<Self> methods receivers. But the underlying API called by the wrapper function can be a member function.

brson commented 2 years ago

I've also filed an issue https://github.com/brson/wasm-opt-rs/issues/107 to avoid string copies in shims.

brson commented 2 years ago

Ok, I believe I have addressed all issues here. Everything is reflected in the blog post, even if some I have punted to a future milestone to actually resolve in the code.

Thank you very much for your help.