1857829 - Wasm GC: ref.eq gives wrong results when comparing i31 values

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect)

Description

[jerome]

Steps to reproduce:

I'm using this script:

<script>
  (async () => {
      const imports = {env:{v:()=>-1}};
      const wasmModule = await WebAssembly.instantiateStreaming(fetch('bug.wasm'),imports)
      wasmModule.instance.exports.f();
  })()
</script>

to load this Wasm code:

(module
   (import "env" "v" (func $g (result (ref eq))))
   (func (export "f")
      (local $v (ref eq))
      (local.set $v (call $g))
      (if (i32.eq (i31.get_s (ref.cast (ref i31) (local.get $v))) (i32.const -1))
         (then
           (if (i32.eqz (ref.eq (local.get $v) (ref.i31 (i32.const -1))))
              (then (unreachable))))))
)

Actual results:

I get an error RuntimeError: unreachable executed.

Expected results:

I do not expect an error, since $v is equal to -1, which was checked right before.

I have not looked at how i31 values are implemented, but I suspect either i31 values are not properly normalized at the interface between JavaScript and Wasm, or there is a bug in the implementation of ref.eq (at some point, there was a similar bug in V8 where ref.eq would do a 64bit comparison on 32bits compressed pointers, also comparing some random higher bits).

Comment 1

[jerome]

I think the issue is in function fromUint32Truncate (https://hg.mozilla.org/mozilla-central/file/tip/js/src/wasm/WasmAnyRef.h#l209). Assuming that casting to uintptr_t properly sets the higher bits, the integer should be tagged before being extended to 64bits:

  static AnyRef fromUint32Truncate(uint32_t value) {
    // Left shift the value by 1, truncating the high bit.
    uint32_t shiftedValue = value << 1;
    uint32_t taggedValue = shiftedValue | (uint32_t)AnyRefTag::I31;
    // Extend the value to the native pointer size.
    uintptr_t wideValue = (uintptr_t)taggedValue;
    return AnyRef(wideValue);
  }

Comment 2

[jerome]

Here is a simpler piece of code that triggers the bug (function fromUint32Truncate is also used to initialize globals).

(module
   (global $i (ref i31) (ref.i31 (i32.const -1)))
   (func (export "f")
      (if (i32.eqz (ref.eq (ref.i31 (i32.const -1)) (global.get $i)))
         (then (unreachable))))
)

Comment 3

[Yury Delendik (:yury)]

Attachment: Bug 1857829 - Normalize i31ref value in fromUint32Truncate. r?jseward

Comment 4

[jerome]

Looking at the proposed fix, it seems to me that it would be simpler to sign-extend the tagged value, rather than the input value.

  static AnyRef fromUint32Truncate(uint32_t value) {
    // Left shift the value by 1, truncating the high bit.
    uint32_t shiftedValue = value << 1;
    uint32_t taggedValue = shiftedValue | (uint32_t)AnyRefTag::I31;
    // Extend the value to the native pointer size.
    // See 64-bit GPRs carrying 32-bit values invariants in MacroAssember.h
#if defined(JS_CODEGEN_LOONG64) || defined(JS_CODEGEN_MIPS64) || \
    defined(JS_CODEGEN_RISCV64)
    // Sign extend the value to the native pointer size.
    uintptr_t wideValue = (uintptr_t) ((int32_t) taggedValue);
#else
    // Zero extend the value to the native pointer size.
    uintptr_t wideValue = (uintptr_t)taggedValue;
#endif
    return AnyRef(wideValue);
  }

Comment 5

[Yury Delendik (:yury)]

(In reply to jerome from comment #4)

uintptr_t wideValue = (uintptr_t) ((int32_t) taggedValue);

IIRC older versions of C might have some undefined behavior associated with cast of signed, hence we apply some shift operation (just to be sure).

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f77f9203e9ef
Normalize i31ref value in fromUint32Truncate. r=jseward

Comment 7

[Cosmin Sabou [:CosminS]]

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

Comment 8

[LuYahan]

Attachment: Bug 1857829 - Fix missing right bracket in fromUint32Truncate. r?jseward

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb3fbb9c4039
Fix missing right bracket in fromUint32Truncate. r=jseward

Comment 10

[Narcis Beleuzu [:NarcisB]]

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