Closed Bug 1489994 Opened 6 years ago Closed 6 years ago

ARM64-sim crashes on anyref-global-prebarrier.js


(Core :: JavaScript: WebAssembly, enhancement)

Not set



Tracking Status
firefox64 --- fixed


(Reporter: lth, Assigned: lth)




(1 file)

Just build m-i form arm64 simulator and jit-test wasm/gc/anyref-global-prebarrier.js.  It looks like this happens in the second call to set(), when we set the value back to null, which is as expected.

I don't have a full diagnosis yet but from where i'm sitting it could look like the code for the prebarrier, which was emitted by Ion, has been emitted assuming that the PSP (x28) has been set up.  But wasm code does not use the PSP; wasm code almost certainly needs a separate prebarrier stub on arm64.
What worries me is that the build bots have not seen this.  Either we're not running with --wasm-gc on the bots, or we're not running arm64 code on the bots at all, or the error has been masked somewhere.
And if it's a problem for the prebarrier, it's likely also a problem for the arguments rectifier and the exception tail functions, since they appear to be obtained in the same way.
Blocks fennec due to risk of arguments rectifier and exception tail functionality also being affected.
Blocks: Fennec-ARM64
... and of course once we're not using the PSP but the real SP, basically no shared code in jit/ (or indeed jit code in jit/arm64/) works because it has a lot of individual push and pop operations, which are verboten on ARM64.

The obvious avenue to try is to save/restore x28 around the call to the barrier stub and to move the sp to x28 for the duration of the barrier call, we'll see.
Unblocking Fennec - I have inspected the paths for the arguments rectifier and the exception signaling, and they do set up the correct SP before executing.  It's only the prebarrier that's the problem.
No longer blocks: Fennec-ARM64
Aouch. See also bug 1464157, which suggests that having our own barrier in wasm would be better anyways. Maybe a chance to kill two birds with one stone.
Aha!!  Well, this week I will take whatever bird I can get with whatever stone I have, I have other patches to land :)
Assignee: nobody → lhansen
Attachment #9007802 - Flags: review?(bbouvier)
See Also: → 1464157
Comment on attachment 9007802 [details] [diff] [review]

Review of attachment 9007802 [details] [diff] [review]:

Alrighty then. Thanks!

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +5655,5 @@
>          masm.loadPtr(Address(scratch, Instance::offsetOfPreBarrierCode()), scratch);
> +# ifdef JS_CODEGEN_ARM64
> +        // The prebarrier stub assumes the PseudoStackPointer is set up.  We do
> +        // not need to save and restore x28 because it is not yet allocatable.
> +        MOZ_ASSERT(!GeneralRegisterSet::All().hasRegisterIndex(x28.asUnsized()));

Can this be a static_assert with no efforts?
Attachment #9007802 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #9)

> Can this be a static_assert with no efforts?

I don't think so, because most of the constants for ARM64 are const but not constexpr, ditto the methods of TypedRegisterSet.  And TypedRegisterSet::All() is just plain static, and I don't know where that falls on the spectrum either.  I can try it just to see what happens but I expect failure :)
Pushed by
On ARM64, make sure to switch to the PSP for the prebarrier stub when calling from wasm.  r=bbouvier
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.