Closed
Bug 1341650
Opened 7 years ago
Closed 7 years ago
wasm: runtime crash involving grow_memory/current_memory and i64
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(4 files, 1 obsolete file)
1.50 KB,
application/javascript
|
Details | |
4.47 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
943 bytes,
patch
|
luke
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
3.69 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
I was playing with a toy program to test 4 GB heaps locally and I found out that it was crashing on x86 on trunk: it seems the TLS register is being clobbered and not reloaded at some point. Looking.
Assignee | ||
Comment 1•7 years ago
|
||
Duh, this is stupid: the TLS register must be reloaded before calling into CurrentMemory in the ion backend. There's also a different issue with the baseline compiler, where it throws a RuntimeError where it should not.
Assignee | ||
Comment 2•7 years ago
|
||
Drive-by fixes: spacing, grammar, register coalescing.
Assignee | ||
Comment 4•7 years ago
|
||
Actually, for obvious uplifting reasons, let's split the fix into two parts: one for ion to be backported, the other one for baseline that can ride the trains.
Attachment #8840011 -
Attachment is obsolete: true
Attachment #8840011 -
Flags: review?(lhansen)
Attachment #8840013 -
Flags: review?(luke)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8840014 -
Flags: review?(lhansen)
Comment 6•7 years ago
|
||
Comment on attachment 8840013 [details] [diff] [review] 2.fix-ion.patch Review of attachment 8840013 [details] [diff] [review]: ----------------------------------------------------------------- D'oh, and thanks!
Attachment #8840013 -
Flags: review?(luke) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8840010 [details] [diff] [review] 1.driveby.patch Review of attachment 8840010 [details] [diff] [review]: ----------------------------------------------------------------- OK. By and large the baseline compiler does not do many micro-optimizations like avoiding redundant loads (or if it does, it does so inconsistently), since there are too many places to worry about those things and we really need a proper layer for that (eg Cretonne). But wrap/extend are probably common enough that it's worth the bother.
Attachment #8840010 -
Flags: review?(lhansen) → review+
Comment 8•7 years ago
|
||
Comment on attachment 8840014 [details] [diff] [review] 3.fix-baseline.patch Review of attachment 8840014 [details] [diff] [review]: ----------------------------------------------------------------- Is there any particular reason you're not using the s-expression form in the test cases? The postorder stack code is hard to read, and if the sexpr form has the same effect it would somwhat be easier to understand the test case, which seems like a fairly big deal. In any case the test case needs a comment explaining what it does, it is very strange. (And a bug reference, since it's in "regress/".)
Attachment #8840014 -
Flags: review?(lhansen) → review+
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Lars T Hansen [:lth] from comment #8) > Comment on attachment 8840014 [details] [diff] [review] > 3.fix-baseline.patch > > Review of attachment 8840014 [details] [diff] [review]: > ----------------------------------------------------------------- > > Is there any particular reason you're not using the s-expression form in the > test cases? The postorder stack code is hard to read, and if the sexpr form > has the same effect it would somwhat be easier to understand the test case, > which seems like a fairly big deal. Well, I disagree with that: the postorder stack form doesn't have a lot of parenthesis, and generally contains fewer instructions per line, increasing readability in my opinion. The hard part is keeping on track with what's on the stack, but that only gets easier with time, and important sub-expressions can be separated by empty lines, which is nice. I've added comments explaining what each sub-expression does.
Assignee | ||
Updated•7 years ago
|
status-firefox52:
--- → affected
status-firefox53:
--- → affected
Comment 10•7 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b432965d1a6 A few drive-by nits fixing in WasmBaselineCompile.cpp; r=lth https://hg.mozilla.org/integration/mozilla-inbound/rev/78329879784e Pass TLS when calling wasm current_memory in the Ion backend; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/286d945249ac Don't clobber the last stack slot when calling into CurrentMemory/GrowMemory in the wasm baseline; r=lth
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8840013 [details] [diff] [review] 2.fix-ion.patch Approval Request Comment [Feature/Bug causing the regression]: wasm current_memory/grow_memory (difficult to pinpoint to a single bug: bug 1287967 would be the original one) [User impact if declined]: crashes when using wasm current_memory opcode [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: just land, but tests locally pass [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: not much [Why is the change risky/not risky?]: minimal patch [String changes made/needed]: n/a
Attachment #8840013 -
Flags: approval-mozilla-beta?
Attachment #8840013 -
Flags: approval-mozilla-aurora?
Comment 12•7 years ago
|
||
Comment on attachment 8840013 [details] [diff] [review] 2.fix-ion.patch wasm crash fix, aurora53+, beta52+
Attachment #8840013 -
Flags: approval-mozilla-beta?
Attachment #8840013 -
Flags: approval-mozilla-beta+
Attachment #8840013 -
Flags: approval-mozilla-aurora?
Attachment #8840013 -
Flags: approval-mozilla-aurora+
Comment 13•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/dc2033fee558
Comment 14•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/af8c05337f11
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8b432965d1a6 https://hg.mozilla.org/mozilla-central/rev/78329879784e https://hg.mozilla.org/mozilla-central/rev/286d945249ac
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 16•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/af8c05337f11
status-firefox-esr52:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•