Closed Bug 1341650 Opened 3 years ago Closed 3 years ago

wasm: runtime crash involving grow_memory/current_memory and i64

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(4 files, 1 obsolete file)

Attached file testcase.js
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.
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.
Attached patch 1.driveby.patchSplinter Review
Drive-by fixes: spacing, grammar, register coalescing.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8840010 - Flags: review?(lhansen)
Attached patch 2.fix-test.patch (obsolete) — Splinter Review
Fix and test.
Attachment #8840011 - Flags: review?(lhansen)
Attached patch 2.fix-ion.patchSplinter Review
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)
Attachment #8840014 - Flags: review?(lhansen)
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 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 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+
(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.
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
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 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+
Duplicate of this bug: 1353350
You need to log in before you can comment on or make changes to this bug.