Skip to content
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

wasm_global_set has no way of communicating that it failed #131

Open
alexcrichton opened this issue Mar 27, 2020 · 8 comments
Open

wasm_global_set has no way of communicating that it failed #131

alexcrichton opened this issue Mar 27, 2020 · 8 comments

Comments

@alexcrichton
Copy link

Currently the signature is:

WASM_API_EXTERN void wasm_global_set(wasm_global_t*, const wasm_val_t*);

but this could fail either due to a type error or because the wasm_global_t is not mutable. Perhaps this API could return a bool instead to indicate whether the set operation succeeded or not?

@rossberg
Copy link
Member

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?

@alexcrichton
Copy link
Author

alexcrichton commented Mar 27, 2020

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 boolean
  • wasm_module_new returns a nullable pointer
  • wasm_instance_new returns a nullable pointer (plus a trap!)
  • wasm_table_set returns a bool
  • wasm_table_get returns a nullable pointer
  • wasm_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?

@sunfishcode
Copy link
Member

@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.

@rossberg
Copy link
Member

rossberg commented Mar 27, 2020

@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.

@rossberg
Copy link
Member

@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.

@rossberg
Copy link
Member

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.

@binji
Copy link
Member

binji commented Mar 27, 2020

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?

@rossberg
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants