Closed Bug 1649929 Opened 7 months ago Closed 6 months ago

wasm/limits.js jit-test requires shared memory, which breaks Cranelift/aarch64 WebAssembly tests

Categories

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

ARM64
Unspecified
defect

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox81 --- fixed

People

(Reporter: cfallin, Assigned: jseward)

References

Details

Attachments

(1 file)

In Bug 1606624, it seems that the wasm/limits.js jit-test was created (or merged from other tests) in such a way that it requires shared-memory support to pass tests. This breaks Cranelift WebAssembly jit-tests on aarch64.

We can temporarily disable the whole wasm/limits.js test until we have shared memory support on Cranelift/aarch64 (Bug 1649927), but ideally we would actually split out the shared-memory portions of the test and disable those.

Separately, we need to establish CI on aarch64 to ensure that this doesn't go unnoticed in the future. (We had an initial conversation about this ~3 weeks ago and it will be tracked by Bug 1649928.)

Blocks: 1648885
Blocks: 1649932
Priority: -- → P2

The severity field is not set for this bug.
:lth, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(lhansen)

Lars is on holiday. Setting S4 as it is not something our users face at the moment (Cranelift on aarch64, which is Nightly only, pref'd off by default).

Severity: -- → S4
Flags: needinfo?(lhansen)
Assignee: nobody → lhansen
Status: NEW → ASSIGNED

Chris, steps to reproduce? I don't seem to be able to. I have an arm64 simulator build; I have removed the guard in the test file; I have added --test-also=--wasm-compiler=cranelift to directives.txt; I have verified that that's one of the configs being run. But I'm not seeing any error.

Flags: needinfo?(cfallin)

We disabled this test on Cranelift for now, but the error should reappear if one disables this directive:

https://searchfox.org/mozilla-central/source/js/src/jit-test/tests/wasm/limits.js#1

However, :jseward is getting close to landing Wasm atomics in the new aarch64 backend, so I think that we can probably just wait for that to happen in order to resolve this issue (the timeline for that wasn't as clear when I created this bug), unless we think it's important to have this test active in the meantime.

Flags: needinfo?(cfallin)

The problem was really (see comment 3 about "the guard in the test file") that I removed the directive, and still could not reproduce. Happy to not worry further though :-)

Assignee: lhansen → jseward

Oh, sorry, I'm clearly in need of more caffeine. On re-examination, it's likely that Cranelift wasn't selected because there's no --shared-memory=off (unless that's just omitted here?). Anyway, yes, no need to burn more time on this. Thanks for taking a look!

That makes sense, I should have considered that!

This patch re-enables the wasm/limits.js test, which we had previously
disabled on Cranelift because it depended on shared memory. Now that the
Cranelift/aarch64 Wasm backend supports shared memory, there is no
reason to exclude this test any more.

Pushed by cfallin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4494f9a71ce5
re-enable wasm/limits.js JIT-test now that Cranelift/aarch64 backend supports shared memory. r=jseward
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Regressions: 1657522
You need to log in before you can comment on or make changes to this bug.