Closed
Bug 1507572
Opened 6 years ago
Closed 6 years ago
Assertion failure: byteSize == 8, at /js/src/jit/arm/MacroAssembler-arm.cpp:6578
Categories
(Core :: JavaScript: WebAssembly, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla65
People
(Reporter: bbouvier, Assigned: lth)
References
Details
Attachments
(2 files)
3.99 KB,
patch
|
bbouvier
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
16.06 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•6 years ago
|
||
Sure, I'll take it. Good bug for a Friday.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → ARM
Assignee | ||
Comment 2•6 years ago
|
||
The most distressing part of this bug by far is that we had no test cases to catch this.
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #9025577 -
Flags: review?(bbouvier)
Assignee | ||
Comment 4•6 years ago
|
||
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.
Assignee | ||
Comment 5•6 years ago
|
||
Some additional test work.
Attachment #9025583 -
Flags: review?(bbouvier)
Assignee | ||
Updated•6 years ago
|
status-firefox63:
--- → fix-optional
status-firefox64:
--- → affected
status-firefox65:
--- → affected
tracking-firefox63:
--- → ?
tracking-firefox64:
--- → ?
tracking-firefox65:
--- → ?
Reporter | ||
Comment 6•6 years ago
|
||
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.
Attachment #9025577 -
Flags: review?(bbouvier) → review+
Reporter | ||
Updated•6 years ago
|
Attachment #9025583 -
Flags: review?(bbouvier) → review+
Pushed by lhansen@mozilla.com:
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f970d9b9b00c
https://hg.mozilla.org/mozilla-central/rev/fc0bc3b27660
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 9•6 years ago
|
||
Please nominate this for Beta approval when you get a chance. Also, a clarification on the tracking request would be appreciated :)
Flags: needinfo?(lhansen)
Flags: in-testsuite+
Assignee | ||
Comment 10•6 years ago
|
||
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.
Flags: needinfo?(lhansen)
Assignee | ||
Comment 11•6 years ago
|
||
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:
Attachment #9025577 -
Flags: approval-mozilla-beta?
Updated•6 years ago
|
Comment 12•6 years ago
|
||
Comment on attachment 9025577 [details] [diff] [review]
bug1507572-store-unaligned-from-i64.patch
wasm fix for arm, approved for 64.0b11
Attachment #9025577 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 13•6 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•