1532714 - Code statically enabled by ENABLE_BULKMEM contradicts wasmBulkMemSupported for Cranelift

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

(... 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...

Comment 1

[Lars T Hansen [:lth]]

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.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1532714: Add a wasmUsesCranelift() function to check usage of Cranelift; r?lth

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.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d953c8003c6
Add a wasmUsesCranelift() function to check usage of Cranelift; r=lth

Comment 4

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9d953c8003c6