Closed Bug 1459893 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jseward, Assigned: jseward)

References

Details

Attachments

(1 file)

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.
Attachment #8973990 - Flags: review?(lhansen)
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?
Attachment #8973990 - Flags: review?(lhansen) → review+
(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.
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c81dfed2fa9f
Wasm: memory.fill and memory.copy: add validation code and test cases.  r=lth.
https://hg.mozilla.org/mozilla-central/rev/c81dfed2fa9f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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 :)
Assignee: nobody → jseward
You need to log in before you can comment on or make changes to this bug.