Closed Bug 1316156 Opened 3 years ago Closed 3 years ago

Sonic/JSMESS/wasm crashes browser

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: azakai, Assigned: bbouvier)

References

Details

Attachments

(1 file, 1 obsolete file)

STR: Load http://assets.metacade.com/emulators/sonic-wasm.html , which is the Sonic game emulated in JSMESS compiled to wasm.

This crashes 100% of the time for me,

https://crash-stats.mozilla.com/report/index/62099980-650c-4a47-b800-95f792161108
https://crash-stats.mozilla.com/report/index/c03eeb26-5267-484a-8343-79db62161108

Interestingly, it doesn't appear at first glance to be a wasm issue? Trace is in js::frontend::Parser.
Yet it seems it must be wasm-related somehow, as the asm.js version (which should be identical, just replacing the wasm with asm.js) works ok,

http://assets.metacade.com/emulators/sonic.html
Actually, when I run this in a debug build, I see an assert in CodeGeneratorX86Shared::visitWasmStackArgI64, so this probably is related to wasm.  What's bizarre is we're in the !IsConstant() branch, but when I evaluate IsConstant(ins->arg()) in the debugger, it's true, so something is weird...
Wow, ins->arg() doesn't return twice the same value indeed (bits are getting modified), under gcc5.4.0 and clang3.8.0. We might fall under some sort of undefined behavior. Trying to understand a bit more and finding a fix.
Duplicate of this bug: 1316321
Indeed, I compiled with clang 3.8 (Apple LLVM 8.0.0 clang-800.0.42.1) for the test case in bug 1316321.
Attached patch usereg.patch (obsolete) — Splinter Review
As said on irc, lowering should ask for register-or-constant here, not constant or any use (that can end up in a stack slot, which is the case here). Now trying to make a test case.
Attachment #8809045 - Flags: review?(luke)
Attached patch usereg.patchSplinter Review
Test case actually trivial.
Assignee: nobody → bbouvier
Attachment #8809045 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8809045 - Flags: review?(luke)
Attachment #8809050 - Flags: review?(luke)
(confirmed it fixes the test case in bug 1316321 too)
Comment on attachment 8809050 [details] [diff] [review]
usereg.patch

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

Great spot!
Attachment #8809050 - Flags: review?(luke) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56280cec9fe2
Use a constant-or-register for int64 pass-stack-arg; r=luke
https://hg.mozilla.org/mozilla-central/rev/56280cec9fe2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: wasm
Component: JavaScript Engine → JavaScript Engine: JIT
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.