Alexandru Ene (Jun 24 2020 at 16:05): Alexandru Ene (Jun 24 2020 at 16:05):I noticed that some of the wasm_***_delete(thing); methods will crash if thing is a nullptr. Should we allow deleting null (as a no-op), just as it works on C++/C with free or delete. E.g. happens with wasm_instance_delete(nulltpr);
Jakub Konka (Jun 24 2020 at 16:19):Personally, I'd leave that in as it is a nice reminder that you've done something wrong (?). On the other hand, it'd be so much better if the compiler caught that in the first place!
Alexandru Ene (Jun 24 2020 at 16:22):Keeping in mind this is C++ land and the compiler would not catch such things out of the box.
For a C++ programmer there's nothing wrong with calling delete on nullptr or free on a nullptr, as it's accepted by the standard and all implementations, wasm_***_delete(...) methods are doing things an unexpected way, leading to a sig11.
I don't think it's necessarely wrong to call delete on a nullptr. E.g .it happened to me as I was hacking about using a linker and instantiated my module like so: https://github.com/bytecodealliance/wasmtime/blob/3715e19c67ab7e59fadf750a1de2ba48f1e6f4ce/examples/wasi/main.c#L84, this in turn gives no wasm_module_instance_t* back so a destructor of a wrapper class called wasm_instance_delete(..). I found that surprising :D
Jakub Konka (Jun 24 2020 at 16:28):Yep, I can imagine! My motivation here is that it's nice for Wasm to catch things that may otherwise lead to undefined behaviour etc. Having said that, I'm not a C++ expert by any means so I'll trust your judgement here and other experts here. :-)
Alexandru Ene (Jun 24 2020 at 16:29):BTW, to clarify my ask above, this is about the HOST environment and the wasmtime VM C API, not WASM-related. E.g. using wasmtime from a C++ project.
Jakub Konka (Jun 24 2020 at 16:35):Ah, now I get it, duh, sorry!
Jakub Konka (Jun 24 2020 at 16:37):It's interesting that this happens since I thought wasm_instance_delete(..) for instance was a pure C function. :thinking:
Alex Crichton (Jun 24 2020 at 17:15):@Alexandru Ene I think you've got a good point, I know it's been pretty convenient in the past that free/etc accept NULL
Alex Crichton (Jun 24 2020 at 17:15):often makes error-handling much easier
Alex Crichton (Jun 24 2020 at 17:16):would you be up for making a PR for this? I think it may even be a one-line change
Alexandru Ene (Jun 24 2020 at 17:19):Yes, I can do that.
Alexandru Ene (Jul 20 2020 at 19:56):I am going to re-open this topic with a bunch of questions/findings.
It looks like wasm.hh has own<T> as unique_ptr<T>, while this may work at first sight, it seems to me like it's assuming that deletions will happen with the default delete T. Now this will get us into later trouble if wasmtime and the host program link to different versions of the stdlib for example. (the classic problem with allocating with a certain allocator and freeing with another one).
This also skips calling the functions available in the C api with WASM_***_delete(thing); where available.
More than this, I think I've found a type without a wasm_delete. There is no way to de-allocate a wasm_functype_t*, returned by wasm_functype_new.
fitzgen (he/him) (Jul 20 2020 at 21:17):re wasm.hh: note that wasmtime only implements the C API, not the C++ API.
we define wasm_functype_delete, but I see that it is in fact missing from wasm.h. @Alexandru Ene, do you want to file a bug upstream on https://github.com/WebAssembly/wasm-c-api/?
Alexandru Ene (Jul 20 2020 at 21:24):Ah right, I can open a PR to add the missing function.
Peter Huene (Jul 20 2020 at 21:24):wasm_functype_delete should come from WASM_DECLARE_OWN(functype)
Peter Huene (Jul 20 2020 at 21:24):which comes from WASM_DECLARE_TYPE(functype)
Alexandru Ene (Jul 20 2020 at 21:25):Oh, right, I managed to confuse myself in searching for that with github search :grinning:
Alex Crichton (Jul 20 2020 at 21:32):oh the docs also have the expanded set of functions
Last updated: Dec 23 2024 at 13:07 UTC