1459893 - Wasm: memory.fill and memory.copy: add validation code and test cases

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Julian Seward [:jseward]]

Bug 1446930 added a baseline implementation of memory.fill/copy, and
runtime test cases for them, but missed out validation code and test
cases for them.  This is a followup, to add those missing elements.

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1459893-bulkmem-validation-2.diff

Comment 2

[Lars T Hansen [:lth]]

Comment on attachment 8973990 [details] [diff] [review]
bug1459893-bulkmem-validation-2.diff

Review of attachment 8973990 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/wasm/memory-bulk.js
@@ +91,5 @@
> +            bodySection(
> +                [funcBody(
> +                    {locals:[],
> +                     body:[0x41, 0x0, 0x41, 0x0, 0x41, 0x0, // 3 x const.i32 0
> +                           MiscPrefix, opcode]})])]);

Sweet.

::: js/src/wasm/WasmValidate.cpp
@@ +820,5 @@
> +#ifdef ENABLE_WASM_BULKMEM_OPS
> +              case uint16_t(MiscOp::MemCopy):
> +                CHECK(iter.readMemCopy(ValType::I32, &nothing, &nothing, &nothing));
> +              case uint16_t(MiscOp::MemFill):
> +                CHECK(iter.readMemFill(ValType::I32, &nothing, &nothing, &nothing));

This is fine, though I wonder whether the argType argument (here I32) is really needed at all.  When would it ever be different?

Comment 3

[Julian Seward [:jseward]]

(In reply to Lars T Hansen [:lth] from comment #2)

Thanks for the review.
 
> ::: js/src/wasm/WasmValidate.cpp
> @@ +820,5 @@
> > +#ifdef ENABLE_WASM_BULKMEM_OPS
> > +              case uint16_t(MiscOp::MemCopy):
> > +                CHECK(iter.readMemCopy(ValType::I32, &nothing, &nothing, &nothing));
> > +              case uint16_t(MiscOp::MemFill):
> > +                CHECK(iter.readMemFill(ValType::I32, &nothing, &nothing, &nothing));
> 
> This is fine, though I wonder whether the argType argument (here I32) is
> really needed at all.  When would it ever be different?

Yeah, good point.  It might be different at some point in the future if we
implement 64 bit memories, and/or the committee changes any of the arguments,
eg to a base+offset form.  But for now it's irrelevant complexity, so I have
removed it.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c81dfed2fa9f
Wasm: memory.fill and memory.copy: add validation code and test cases.  r=lth.

Comment 5

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c81dfed2fa9f

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

To wit: using wasmEvalText is not only about code conciseness. It's indeed equivalent to x => new W.Instance(new W.Module(wasmTextToBinary(x))), but it also checks that WebAssembly.validate is consistent with the return value of new W.Module(). So using it in the first place would prevent to forget to update the validation algorithm. For next time :)