Closed Bug 1597989 Opened 5 months ago Closed 4 months ago

Bump Cranelift to 152e31796970262068b4fe93e10395385bf9218c

Categories

(Core :: Javascript: WebAssembly, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox73 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

No description provided.

Failures when running the test suite with Spidermonkey:

Related to bulk memory operators:

wasm/spec/bulk.wast.js
--test-wasm-await-tier2 wasm/spec/bulk.wast.js
--disable-wasm-huge-memory wasm/spec/bulk.wast.js
wasm/spec/memory_fill.wast.js
wasm/spec/memory_copy.wast.js
--disable-wasm-huge-memory wasm/spec/memory_fill.wast.js
--test-wasm-await-tier2 wasm/spec/memory_fill.wast.js
--disable-wasm-huge-memory wasm/spec/memory_copy.wast.js
--test-wasm-await-tier2 wasm/spec/memory_copy.wast.js
--disable-wasm-huge-memory wasm/spec/memory_init.wast.js
wasm/spec/memory_init.wast.js
--test-wasm-await-tier2 wasm/spec/memory_init.wast.js
wasm/spec/table_copy.wast.js
--test-wasm-await-tier2 wasm/spec/table_copy.wast.js
--disable-wasm-huge-memory wasm/spec/table_copy.wast.js
--test-wasm-await-tier2 wasm/spec/table_init.wast.js
--disable-wasm-huge-memory wasm/spec/table_init.wast.js
wasm/spec/table_init.wast.js

Other failures (may require further investigation):

wasm/spec/type.wast.js
--disable-wasm-huge-memory wasm/spec/type.wast.js
--test-wasm-await-tier2 wasm/spec/type.wast.js
Priority: -- → P3

The issue with type.wast is that multi-value is disabled when cranelift is enabled, so there are expected failures around types with multiple return values. But the validation code wouldn't check if cranelift was enabled, only multi-value, so the tests would pass. Updating the validation code to not allow multi-value with cranelift led to more failures.

The simplest option I saw was to just enabled multi-value with cranelift, as IIUC there is now support. This resolved all the test failures (besides bulk-memory).

The simplest option I saw was to just enabled multi-value with cranelift, as IIUC there is now support. This resolved all the test failures (besides bulk-memory).

There's missing glue between Cranelift and Spidermonkey to have proper support for multi value types in wasm, so I expect this won't work at runtime.

The right fix is to have the Validate function consider if multi values can be enabled (by adding another parameter to CompilerEnvironment) and see in which places we need to check for this; probably when decoding the type section and in misc places in the code section.

Let get this done in a follow-up.

This hasn't landed yet, and there are new test failures that were not there last week:

    wasm/import-export.js

/home/ben/code/mozilla-inbound/js/src/jit-test/lib/asserts.js:72:23 Error: Assertion failed: expected exception LinkError, got RuntimeError: index out of bounds
Stack:
  assertErrorMessage@/home/ben/code/mozilla-inbound/js/src/jit-test/lib/asserts.js:72:23
  assertSegmentFitError@/home/ben/code/mozilla-inbound/js/src/jit-test/tests/wasm/import-export.js:22:27
  @/home/ben/code/mozilla-inbound/js/src/jit-test/tests/wasm/import-export.js:498:22

    wasm/tables.js

/home/ben/code/mozilla-inbound/js/src/jit-test/lib/asserts.js:72:23 Error: Assertion failed: expected exception LinkError, got RuntimeError: index out of bounds
Stack:
  assertErrorMessage@/home/ben/code/mozilla-inbound/js/src/jit-test/lib/asserts.js:72:23
  assertSegmentFitError@/home/ben/code/mozilla-inbound/js/src/jit-test/tests/wasm/tables.js:14:27
  @/home/ben/code/mozilla-inbound/js/src/jit-test/tests/wasm/tables.js:25:22

Probably the same underlying issue.

This is due to https://phabricator.services.mozilla.com/D54598, which introduced assertion methods based on bulk-memory being properly implemented, so maybe the right path forward is to review and land bulk mem in Cranelift...

Attachment #9110250 - Attachment description: Bug 1597989: Output of mach rust vendor for the Cranelift bump; r?rhunt → Bug 1597989: Output of mach rust vendor for the Cranelift bump; r=rhunt

The eagerBoundsCheck variable was desynchronized from compiler feature support
for bulk memory operations, namely for Cranelift. This manually checks for
Cranelift usage, and this will need to be reverted once bulk memory opcodes
support land in Cranelift.

Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c803ec19d69
Bump cranelift to 152e317969702620; r=rhunt
https://hg.mozilla.org/integration/autoland/rev/aa13508625cf
Output of mach rust vendor for the Cranelift bump; r=rhunt
https://hg.mozilla.org/integration/autoland/rev/827afbfc6f34
Tweak eager segments bound checks behavior for Cranelift; r=rhunt
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
Regressions: 1601150
Blocks: 1610542
No longer blocks: cranelift
You need to log in before you can comment on or make changes to this bug.