1559962 - Bulk memory: Implement spec change to determining copy direction and bounds checking

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Lars T Hansen [:lth]]

The Wasm CG meeting in A Coruña in June 2019 voted to change the bulk memory proposal in two respects: there will be a simpler test for copy direction (actual overlap will not be required, just a comparison of src and dest), and a zero-length operation is allowed even if the indices are out of bounds beyond the length of the array.

Agenda for the meeting here, minutes & slides will appear there:
https://github.com/WebAssembly/meetings/blob/master/2019/CG-06.md

Comment 1

[Lars T Hansen [:lth]]

Zero-byte-copy tracking issue: https://github.com/WebAssembly/bulk-memory-operations/issues/97
Copy direction tracking issue: https://github.com/WebAssembly/bulk-memory-operations/issues/96

Comment 2

[Lars T Hansen [:lth]]

Spec updates for zero-byte-copy:
https://github.com/WebAssembly/bulk-memory-operations/pull/100
https://github.com/WebAssembly/bulk-memory-operations/pull/101

Comment 3

[Ryan Hunt [:rhunt]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d49a94cd4befd725efd083c4b35d8dac5e75c96

I need to manually trigger jittests, I messed up trychooser.

I did a run locally which passed, so I'm okay with putting up the patches for review in the mean time.

Comment 4

[Ryan Hunt [:rhunt]]

There is one point in the spec that I'm not clear on.

Do the new rules for copy direction and zero length OOB access apply to active segments? Or is it only to the actual bulk memory instructions.

The patches here only affect the bulk memory instructions and not the active segment code path taken during instantiation.

Comment 5

[Ryan Hunt [:rhunt]]

Attachment: Bug 1559962 - Allow zero-length bulk memory operations even if they are OOB as per bulk memory operations spec change. r?jseward

https://github.com/WebAssembly/bulk-memory-operations/issues/97

Comment 6

[Ryan Hunt [:rhunt]]

Attachment: Bug 1559962 - Use simpler expression for determining copy direction as per bulk memory operations spec change. r?jseward

• https://github.com/WebAssembly/bulk-memory-operations/issues/96

Depends on D37798

Comment 7

[Ryan Hunt [:rhunt]]

Attachment: Bug 1559962 - Update test expectations for bulk memory operations spec changes. r?jseward

Depends on D37799

Comment 8

[Ryan Hunt [:rhunt]]

The jit-tests passed on try, ditto for WPT.

(In reply to Ryan Hunt [:rhunt] from comment #4)

There is one point in the spec that I'm not clear on.

Do the new rules for copy direction and zero length OOB access apply to
active segments? Or is it only to the actual bulk memory instructions.

The patches here only affect the bulk memory instructions and not the active
segment code path taken during instantiation.

I looked at the proposed spec modifications for validating active segments and didn't see any changes. Additionally did a simple test case in Chrome Canary and they reject zero sized segments placed OOBs.

Comment 9

[Lars T Hansen [:lth]]

Good question.

On the one hand, I think the changes also apply to active segments, for three reasons. One, when we initially implemented these instructions and gave them lazy bounds checking behavior, we also updated the bounds checking behavior for the active segments to match. Two, if you look at the spec interpreter's implementation of active-segment initialization, https://github.com/WebAssembly/bulk-memory-operations/blob/master/interpreter/exec/eval.ml#L465, you'll see that it applies the same Table.init method as the table_init instruction uses, with the same bounds checking behavior, https://github.com/WebAssembly/bulk-memory-operations/blob/master/interpreter/runtime/table.ml#L52. Three, the prose here, https://github.com/WebAssembly/bulk-memory-operations/blob/master/proposals/bulk-memory-operations/Overview.md#design, says that active segments are implemented in terms of the passive-instruction semantics. So we'd want to update for active segments too.

On the other hand, the current formal spec text here, https://github.com/WebAssembly/bulk-memory-operations/blob/master/document/core/exec/modules.rst#instantiation, does not seem to follow that. But that spec performs eager bounds checking (or so it seems to me; it's hard to see past all the math), so it's not current in any case.

I filed a request for clarification on issue 97.

Comment 10

[Ryan Hunt [:rhunt]]

From the spec issue, it sounds like this is intended to affect active segment initialization as well. I added a patch to do this as well, and will put it up for review.

It seems though that there are some specification tests that assert a wasm module containing a zero-sized segment is unlinkable. One example is 'spec/elem.wast.js:72:1'. I need to do some more digging to completely confirm this, as the spec modules are all in binary and so I need to dissassemble them to confirm.

/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:31:15 Error: #24 A wast module that is unlinkable.: FAIL.
/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:31:15 Error: Assertion failed: got false, expected true
/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:31:15 assert_true@/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:56:36
/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:31:15 assert_unlinkable/<@/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/../tests/wasm/spec/harness/sync_index.js:256:9
/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:31:15 test@/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:25:9
/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:31:15 uniqueTest@/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/../tests/wasm/spec/harness/sync_index.js:152:5
/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:31:15 assert_unlinkable@/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/../tests/wasm/spec/harness/sync_index.js:255:5
/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:31:15 @/Users/rhunt/src/mozilla/js/js/src/jit-test/tests/wasm/spec/elem.wast.js:72:1
/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:31:15 
Stack:
  test@/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/wasm-testharness.js:31:15
  uniqueTest@/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/../tests/wasm/spec/harness/sync_index.js:152:5
  assert_unlinkable@/Users/rhunt/src/mozilla/js/js/src/jit-test/lib/../tests/wasm/spec/harness/sync_index.js:255:5
  @/Users/rhunt/src/mozilla/js/js/src/jit-test/tests/wasm/spec/elem.wast.js:72:1
Exit code: 3

Comment 11

[Lars T Hansen [:lth]]

Source for the spec tests is here: https://github.com/WebAssembly/spec/blob/master/test/core/elem.wast and with changes for the bulk memory proposal here: https://github.com/WebAssembly/bulk-memory-operations/blob/master/test/core/elem.wast. Buyer beware, the sources for the originals may have diverged from our binary renditions of them, I'm not sure how often we import updates but "sometimes" is probably apposite.

ISTR the binary versions are generated from the sources using some build step in the repo, but I forget the specific mechanics; you'll have to investigate.

Comment 12

[Ryan Hunt [:rhunt]]

I'm going to split the work for tests across bug 1566516 and this bug.

I've got a rebased series of commits that also handle active segment initialization as well.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de4716950ef98c4d5cbdc8b9c4e98d0f63117153

Comment 13

[Ryan Hunt [:rhunt]]

Attachment: Bug 1559962 - Also apply zero-sized copy OOB rules to active segment initialization as per bulk memory operations spec changes. r?jseward

Depends on D37800

Comment 14

[Ryan Hunt [:rhunt]]

Attachment: Bug 1559962 - Remove unused importFuncs parameter from Module::instantiate. r?jseward

Depends on D38658

Comment 15

[Ryan Hunt [:rhunt]]

Attachment: Bug 1559962 - Update spec/core wasm tests for bulk memory operations repo changes. r?luke

This commit updates the core wasm tests again using 'spidermonkey-tree-tests'.
(https://github.com/eqrion/spec @ bc995a154acb02624cab98a7f6bb7e90f0328172)

The branch continues from the last vendored point of the spec and merges the
changes from the bulk-memory-operations spec, along with a fix for a test.

Depends on D38659

Comment 16

[Ryan Hunt [:rhunt]]

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=258199559&revision=3d1c9a3071122709ab9026727bd6c38beec9ece8

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d06cda4f5cfc
Remove unused importFuncs parameter from Module::instantiate. r=jseward
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc56f6f19743
Allow zero-length bulk memory operations even if they are OOB as per bulk memory operations spec change. r=jseward
https://hg.mozilla.org/integration/mozilla-inbound/rev/a49f28445682
Also apply zero-sized copy OOB rules to active segment initialization as per bulk memory operations spec changes. r=jseward
https://hg.mozilla.org/integration/mozilla-inbound/rev/1731c6c8abb5
Use simpler expression for determining copy direction as per bulk memory operations spec change. r=jseward
https://hg.mozilla.org/integration/mozilla-inbound/rev/931d646db38d
Update test expectations for bulk memory operations spec changes. r=jseward
https://hg.mozilla.org/integration/mozilla-inbound/rev/655d0fdda3c9
Update spec/core wasm tests for bulk memory operations repo changes. r=luke

Comment 18

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/d06cda4f5cfc
https://hg.mozilla.org/mozilla-central/rev/bc56f6f19743
https://hg.mozilla.org/mozilla-central/rev/a49f28445682
https://hg.mozilla.org/mozilla-central/rev/1731c6c8abb5
https://hg.mozilla.org/mozilla-central/rev/931d646db38d
https://hg.mozilla.org/mozilla-central/rev/655d0fdda3c9