1381438 - boxDouble should not clobber the source register

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 1

[Jan de Mooij [:jandem]]

We should probably backport this to 55. Fortunately this only affects pretty old CPUs (x86 without SSE 4.1).

Comment 2

[Tom S [:evilpie]]

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.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e73020a1e023

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you're comfortable doing so.

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Julien Cristau [:jcristau]]

Comment on attachment 8887000 [details] [diff] [review]
Patch

jit fix for some x86 cpus, beta55+

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b3eff505c8d1