Open alexcrichton opened 4 years ago
The global not being mutable qualifies as a type error as well. Both variants of error should never occur in a correct program. Do you have any specific use case why a program should be able to reflect on such failures in this particular instance?
Er sorry I'm not really sure how to answer that? I feel like "correct programs" also don't contain out of bounds errors, yet APIs like wasm_table_set
still returns a bool
. It feels wrong to design the API around what a "correct program" is since it seems like time has shown that inevitably everyone makes mistakes.
This to me feels like any number of other routine errors which can already be communicated elsewhere in the API:
wasm_module_validate
returns a booleanwasm_module_new
returns a nullable pointerwasm_instance_new
returns a nullable pointer (plus a trap!)wasm_table_set
returns a bool
wasm_table_get
returns a nullable pointerwasm_table_grow
returns a bool
wasm_memory_grow
returns a bool
(etc)
It seems like the C API is pretty aggressive about exposing to embedders what possible errors can happen when you're working with wasm, so why would this specifically be singled out as not returning an error? What would you expect engines to do if a non-mutable global is called with this API? Or if the wrong type wasm_val_t
is passed in?
@rossberg One reason a program might want to reflect on such failures would be to implement the official JS API, which is does provide the ability to reflect on such failures.
While it's a nice symmetry to think of the C API as a pure reflection of the wasm semantics, the C API like the official JS API has already deviated from that by not doing full up-front validation. In wasm, global.set
on an immutable global is caught at validation time, and the JS and C APIs don't reflect that behavior. So it's not a question of "should we match wasm or not?", it's "we're already not matching wasm, so what's the best thing we can do instead?"
Returning an error indicator here is simple to do, and the return value can be ignored by users who are really confident they never have an error. And it helps users who want it.
@alexcrichton, ah, but these are different classes of errors. Roughly, everything that corresponds to a legitimate Wasm error (e.g., trap or import mismatch) is reflected, because it is defined behaviour. For example, accessing at table out of bounds is defined to trap and thus such an error is captured in the API.
Accessing a global at the wrong type is plain invalid and simply undefined behaviour. Even table_set will do whatever if the types don't match, there is no check for that.
I don't think it's useful to require all sorts of sanity checks for incorrect usage of this (utterly unsafe) low-level API. That isn't the C way, and it would just slow it down. Of course, an implementation is still free to do so, but it's not required.
@sunfishcode, actually, the idea is to match Wasm behaviour precisely. Invalid uses are undefined in Wasm, and that carries over to the API. Unlike Wasm, we can't preclude invalid usage statically for this API, but as usual in C, it's up to the user to ensure correctness, and invalid use is just undefined behaviour.
PS: I should also point out that checks like verifying the type of a value passed to global_set or table_set aren't even possible in general. With reference types, there won't necessarily be enough information available at runtime to determine the exact type of the reference value.
I should also point out that checks like verifying the type of a value passed to global_set or table_set aren't even possible in general.
If that's true, then how will we ensure that the JS API isn't breaking wasm invariants either?
@binji, that's a good question. Either we have to restrict the types of globals/tables that can be used in JS. Or JS-embedded engines have to make all their references self-describing enough to enable these checks. That may come at a (space) cost that other engines might not necessarily want to pay.
Currently the signature is:
but this could fail either due to a type error or because the
wasm_global_t
is not mutable. Perhaps this API could return abool
instead to indicate whether the set operation succeeded or not?