Code statically enabled by ENABLE_BULKMEM contradicts wasmBulkMemSupported for Cranelift

RESOLVED FIXED in Firefox 67

Status

()

enhancement
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

(... i suspect this is the case for other combinations of defines and testing function, sigh)

The issue is the following: on a regular nightly build, ENABLE_BULKMEM_OPS is defined, so the code in Module::initSegments will do partial initialization with pleasure. Unfortunately, there's a test in wasm/import-export.js which checks wasmBulkMemSupported() first (which dynamically returns false for Cranelift): since it returns false, it asserts that the previous behavior is applied (i.e. no partial initialization), so the test fails.

The right fix would be to implement bulk memory operations in Cranelift. Maybe there's something to be done in Module::initSegments in the meanwhile, though...

Yeah, this will be true also for reference types within a matter of days.

I think we should fix this in the test cases if possible, not in the VM, because the VM logic is brittle already.

For test cases that fail with Cranelift we could just add an additional guard to the top of the test case, !wasmCraneliftEnabled(). This is not 100% ideal but it should work OK. If we all know about this then we'll all be aware that as Cranelift grows, we get to remove these guards.

This assumes that wasmCraneliftEnabled() checks the command line flag.

Bulk memory operations can be enabled at the same time Cranelift is, in which
case partial initialization of elements/segments/etc. implementation is
statically implemented, but dynamically controlled by wasmBulkMemoryOps().

Cranelift doesn't implement bulk memory operations yet, so wasmBulkMemoryOps()
returns false, while the static behavior is there. Thus, we have to temporarily
disable partial initialization tests when running with Cranelift. This will go
away when Cranelift supports bulk memory operations.

Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d953c8003c6
Add a wasmUsesCranelift() function to check usage of Cranelift; r=lth
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.