1507572 - Assertion failure: byteSize == 8, at /js/src/jit/arm/MacroAssembler-arm.cpp:6578

Status: RESOLVED FIXED

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

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

The following test case (reduced from awsm) crashes an ARM shell when run with --no-wasm-ion:

let i = new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(`
(module
    (memory 1 1)
    (func $f (local i32)
        get_local 0
        i64.const 0
        i64.store16 align=1
    )
)
`))).exports;

Indeed, a misaligned store can be of less than 8 bytes, and we never implemented that. This is "only" a correctness bug, as far as I can tell.

Lars, are you interested in fixing this?

Comment 1

[Lars T Hansen [:lth]]

Sure, I'll take it.  Good bug for a Friday.

Comment 2

[Lars T Hansen [:lth]]

The most distressing part of this bug by far is that we had no test cases to catch this.

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1507572-store-unaligned-from-i64.patch

Comment 4

[Lars T Hansen [:lth]]

I should add: I put that up for review so that we can get it into the pipeline and maybe uplift to beta.  As a follow-up (today) I will look into the test case situation for the sub-word operations and make sure it's adequate.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1507572-test-cases.patch

Some additional test work.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9025577 [details] [diff] [review]
bug1507572-store-unaligned-from-i64.patch

Review of attachment 9025577 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +6582,5 @@
> +            break;
> +          case 4:
> +          case 2:
> +            emitUnalignedStore(&access, byteSize, ptr, val64.low);
> +            break;

Maybe add a default case with MOZ_CRASH to catch issues here? Otherwise, a plain `if/else` would do.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f970d9b9b00c
Generalize alignments for memory ops testing. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc0bc3b27660
correctly implement unaligned stores of i64 sub-fields.  r=bbouvier

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/f970d9b9b00c
https://hg.mozilla.org/mozilla-central/rev/fc0bc3b27660

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance. Also, a clarification on the tracking request would be appreciated :)

Comment 10

[Lars T Hansen [:lth]]

I'm sure I'm just abusing the tracking flags.  I just wanted to be sure that we would get this into beta, so I figured the tracking request would add visibility.

Comment 11

[Lars T Hansen [:lth]]

Comment on attachment 9025577 [details] [diff] [review]
bug1507572-store-unaligned-from-i64.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Obscure bugs on ARM for some WebAssembly content

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This adds logic where we had none, but should not affect any case where we already had correct code.

String changes made/needed:

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 9025577 [details] [diff] [review]
bug1507572-store-unaligned-from-i64.patch

wasm fix for arm, approved for 64.0b11

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/04a44178fb99
https://hg.mozilla.org/releases/mozilla-beta/rev/34b98cadb788