1847757 - Add debug assertions about canonical high-bits for i31ref values on 64-bit targets

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Ryan Hunt [:rhunt]]

AnyRef is a word-sized value and can store an immediate 31-bit value (i31ref), on 64-bit targets the upper 32-bits for i31ref are not in use but must be well defined so that ref.eq works as expected.

We create i31ref values in the JIT using 32-bit instructions which should leave the high 32-bits in a consistent state depending on the platform (either zero/sign extended). Theoretically, the same should be true of our C++ conversion routines. We should add some assertions though that this is the case, and maybe some stress tests.

Comment 1

[Ryan Hunt [:rhunt]]

The three masm operations that produce and consume i31ref are here [1]. In each case, we should add a call to debugAssertCanonicalInt32 [2] either before consuming the i31ref value or after producing the value.

The C++ version of creating an i31ref is here [3], and consuming an i31ref is here [4]. We don't have a debugAssertCanonicalInt32 in C++, but could probably add one?

[1] https://searchfox.org/mozilla-central/rev/3c72de9280ec57dc55c24886c6334d9e340500e8/js/src/jit/MacroAssembler.cpp#6521-6552
[2] https://searchfox.org/mozilla-central/rev/3c72de9280ec57dc55c24886c6334d9e340500e8/js/src/jit/MacroAssembler.cpp#7092
[3] https://searchfox.org/mozilla-central/rev/3c72de9280ec57dc55c24886c6334d9e340500e8/js/src/wasm/WasmAnyRef.h#209
[4] https://searchfox.org/mozilla-central/rev/3c72de9280ec57dc55c24886c6334d9e340500e8/js/src/wasm/WasmAnyRef.h#316

Comment 2

[Julien Pages [:jpages]]

Attachment: Bug 1847757 - wasm: Add debug assertions for i31ref operations.

This commit adds debug assertions to make sure that i31ref values
fit into the low-bits on 64-bits platforms.
The upper 32-bits should not be used on 64-bits platforms.

Comment 3

[Julien Pages [:jpages]]

The linked patch https://phabricator.services.mozilla.com/D212495 is failing with the new assertions on some examples on a x86 64-bits Linux.

The following tests are failing with baseline and Ion when the value -1 is passed through the JS API to wasm. This value ends up being 0x1ffffffff which is bigger than 0x7fffffff (max value for a i31ref).

https://searchfox.org/mozilla-central/source/js/src/jit-test/tests/wasm/gc/externref-boxing-struct.js#38
https://searchfox.org/mozilla-central/source/js/src/jit-test/tests/wasm/gc/externref-conversions.js#53

In theses cases, we have a value bigger than 32-bits in a register for a i31ref. This is not supposed to be the case, I'll continue to investigate.

This is captured in a pernosco session here: https://pernos.co/debug/7IljVVy5bc97HQuve4Hmzg/index.html#f{m[BLE,AUA_,t[AQ,tBI_,f{e[BK8,BM8_,s{aVoCQVeAA,bBQ,uBsHMLQ,oBvU3Ig___/

Ryan suggested to look at the entry wrappers between JS and wasm, that's the next step.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2769c2e33da7
wasm: Add debug assertions for i31ref operations. r=rhunt

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/2769c2e33da7