1636235 - Baseline: assorted SIMD corner case bugs

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

needResultRegisters() only allocates SIMD registers if which == RegKind::All, so we must free only in that situation too. Slightly surprising that we've not stumbled across that in an assertion, but needIntegerResultRegisters() is only used by br_table and it could be that no v128 test uses br_table; will test this hypothesis.

Comment 1

[Lars T Hansen [:lth]]

Test case, run with --wasm-simd in a debug build and we trigger the expected assert:

new WebAssembly.Module(wasmTextToBinary(`
  (module
    (memory (export "mem") 1 1)
    (func $f (result v128)
      (block $B1 (result v128)
        (v128.const i32x4 1 2 3 4)
        (br_table $B1 (i32.const 0)))))`))

Comment 2

[Lars T Hansen [:lth]]

Another one: when materializing values during popStackResults, we need to handle the ConstV128 case. (This is yet another instance of the general badness of allowing a default on an enum class, as an exhaustive switch would have caught this problem a long time ago.)

Comment 3

[Lars T Hansen [:lth]]

Attachment: Bug 1636235 - Fix corner case SIMD bugs. r=wingo

There was a missing guard when freeing the SIMD result register; it
was freed unconditionally, but this would fail for br_table, when only
the int register is needed.

There was a missing case when materializing constants during
popStackResults, we need to handle V128 here.

In both cases the underlying cause was iffy test coverage, though in
the latter case the problem is also that we allow the use of switch
with default for enum class value sets, thus masking problems where a
case that must be covered is not covered.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f3939d739c5
Fix corner case SIMD bugs. r=wingo

Comment 5

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9f3939d739c5