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

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Lars T Hansen [:lth]]

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.

Comment 1

[Lars T Hansen [:lth]]

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.

Comment 2

[Lars T Hansen [:lth]]

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.

Comment 3

[Lars T Hansen [:lth]]

Blocks fennec due to risk of arguments rectifier and exception tail functionality also being affected.

Comment 4

[Lars T Hansen [:lth]]

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

Comment 5

[Lars T Hansen [:lth]]

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.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 7

[Lars T Hansen [:lth]]

Aha!!  Well, this week I will take whatever bird I can get with whatever stone I have, I have other patches to land :)

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1489994-prebarrier-psp.patch

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9007802 [details] [diff] [review]
bug1489994-prebarrier-psp.patch

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?

Comment 10

[Lars T Hansen [:lth]]

(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 :)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b22c01fefae4
On ARM64, make sure to switch to the PSP for the prebarrier stub when calling from wasm.  r=bbouvier

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b22c01fefae4