boxDouble should not clobber the source register

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 wontfix, firefox55- fixed, firefox56- fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Created attachment 8887000 [details] [diff] [review]
Patch

On x86 without SSE 4.1, boxDouble can clobber the source double register. This is surprising and breaks some CacheIR code. evilpie's patch for bug 1344483 triggers a jit-test failure.

This patch makes this more explicit by adding an additional temp argument to boxDouble. Most callers can pass the same register as the source register, but callers that want to preserve the source can pass a different register.
Attachment #8887000 - Flags: review?(evilpies)
(Assignee)

Comment 1

a year ago
We should probably backport this to 55. Fortunately this only affects pretty old CPUs (x86 without SSE 4.1).
status-firefox54: --- → wontfix
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
Comment on attachment 8887000 [details] [diff] [review]
Patch

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

Nice, thanks for looking into this, I would probably not have found this.
Attachment #8887000 - Flags: review?(evilpies) → review+

Comment 3

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e73020a1e023
Fix boxDouble to not clobber the source register on x86 CPUs without SSE 4.1. r=evilpie

Comment 4

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e73020a1e023
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Please request Beta approval on this when you're comfortable doing so.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 6

a year ago
Comment on attachment 8887000 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1341067.
[User impact if declined]: Broken websites for users with old CPUs.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[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?]: No.
[Why is the change risky/not risky?]: Patch is mechanical for the most part, only changes behavior for users on 32-bit x86 CPUs that don't support SSE 4.1  
[String changes made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #8887000 - Flags: approval-mozilla-beta?
Comment on attachment 8887000 [details] [diff] [review]
Patch

jit fix for some x86 cpus, beta55+
Attachment #8887000 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 8

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/b3eff505c8d1
status-firefox55: affected → fixed
tracking-firefox55: ? → -
tracking-firefox56: ? → -
You need to log in before you can comment on or make changes to this bug.