Rework JS API spec implementation limits - #2089
Conversation
rossberg
left a comment
rossberg
left a comment
There was a problem hiding this comment.
This all looks good to me. Have you compared by any chance whether the new limits introduced here are also listed in the core spec's Appendix? I wouldn't be surprised if that also has some gaps regarding newer features.
|
I don't think I've introduced any new limits. The core spec already allows syntactic limits on everything here (including both the number and size of elem segments), and execution limits on the size of tables and memories. GC things are well-covered by the core spec too. If it looks like I added any limits here, it's either because it was missing from the JS API spec only, or just part of the general refactoring I did to the section. |
| To <dfn>compile a WebAssembly module</dfn> from source bytes |bytes|, perform the following steps: | ||
| 1. Let |module| be [=module_decode=](|bytes|). If |module| is [=error=], return [=error=]. | ||
| 1. If [=module_validate=](|module|) is [=error=], return [=error=]. | ||
| 1. If any <a href="#limits-compile-time">compile-time limits</a> are exceeded, return [=error=]. |
There was a problem hiding this comment.
Note: This means that WebAssembly.validate will always fail when a module exceeds a compile-time limit, and should never fail when exceeding a runtime limit (even if instantiation would fail).
| 1. Let |result| be [=module_instantiate=](|store|, |module|, |imports|). | ||
| 1. If |result| is [=error=], throw an appropriate exception type: | ||
| * A {{LinkError}} exception for most cases which occur during linking. | ||
| * If a <a href="#limits-runtime">runtime implementation limit</a> is exceeded, throw a {{RangeError}}. |
There was a problem hiding this comment.
This is the case that covers e.g. a table with an initial size of 10,000,001 elements. Under this update to the spec, this would pass WebAssembly.validate and WebAssembly.compile but would always fail to instantiate due to this line.
eqrion
left a comment
eqrion
left a comment
There was a problem hiding this comment.
Sorry for the delay. This LGTM.
Ms2ger
left a comment
Ms2ger
left a comment
There was a problem hiding this comment.
(Partial review, ran out of time.)
| 1. Let |ret| be the [=mem_size=](|store|, |memaddr|). | ||
| 1. Let |store| be [=mem_grow=](|store|, |memaddr|, |delta|). | ||
|
|
||
| Note: This should check the <a href="#limits-runtime">runtime implementation limits</a>. |
There was a problem hiding this comment.
"should" is an RCF2119 keyword, please don't use it in notes.
There was a problem hiding this comment.
What would be the conventional way of writing guidance like this? My intent is to call the reader's attention to the place where runtime implementation limits are relevant.
| 1. Let |result| be [=module_instantiate=](|store|, |module|, |imports|). | ||
| 1. If |result| is [=error=], throw an appropriate exception type: | ||
| * A {{LinkError}} exception for most cases which occur during linking. | ||
| * If a <a href="#limits-runtime">runtime implementation limit</a> is exceeded, throw a {{RangeError}}. |
There was a problem hiding this comment.
This whole step is a mess, but that predates your change. Ideally core would tell us what happened rather than making us hand-wave like this.
| <h2 id="limits">Implementation-defined Limits</h2> | ||
|
|
||
| The WebAssembly core specification allows an implementation to define limits on the syntactic structure of the module. | ||
| The WebAssembly core specification allows an implementation to define limits on the syntactic structure of a module and on runtime resources. |
There was a problem hiding this comment.
I couldn't find where it says that. Do you happen to have a pointer?
There was a problem hiding this comment.
That would be the Execution section of the "implementation limitations" section here: https://webassembly.github.io/spec/core/appendix/implementation.html
| 1. If |maximum| is not empty: | ||
| 1. If |addrtype| is "i64" and |maximum| exceeds the [=memory type size limit=], throw a {{RangeError}} exception. |
There was a problem hiding this comment.
Why is this here? Is it guaranteed there's no other callers of mem_alloc? If there are others, should this restriction not also be applied there?
There was a problem hiding this comment.
This is so long ago now that I don't quite remember why this is structured this way. It seems to me that it could be covered by or folded into the {{RangeError}} case on the mem_alloc step below.
4 participants
Resolves #1863 by reworking how implementation limits are organized, and drawing a clear distinction between compile-time and runtime limits.
I have currently made the decision to throw a RangeError when instantiating a module that trips a runtime resource limit, e.g. instantiating a module with a table whose initial size exceeds the implementation limit. This was already reflected in some tests and mirrors what happens if constructing an oversize resource from JS, e.g. new WebAssembly.Table.
I've left a couple inline comments to highlight the impact this spec will have on engines.
A couple other things to note:
memoryandtablefolders and memory64 limits are only checked in test files with-memory64in the name, following what seems to be an existing pattern. However, I honestly have no idea where we're supposed to have tests for this stuff, since the WPT tests in this repo and in the main WPT repo are wildly out of sync, so please tell me if I should organize things differently.Important: Currently no major engines pass all of these tests. SpiderMonkey throws RuntimeError where I think we should now throw RangeError, V8 rejects tables at compile time when it should reject them at instantiation time, etc. This suite of updates attempts to resolve those discrepancies, but it's less clear than usual which behavior we want.