Math.round gets miscompiled

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: simon.lindholm10, Assigned: jandem)

Tracking

23 Branch
mozilla25
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 affected, firefox22 affected, firefox23- affected, firefox24- affected)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130508 Firefox/23.0
Build ID: 20130508031113

Steps to reproduce:

Visit http://simonsoftware.se/other/nanoacre-fxbug2/?debug=1. Open the Web Console. Right-click to move the character to a point roughly in the middle of the canvas in x direction, and slightly above the walls in y direction (so that it moves diagonally towards up-right). If nothing is logged to the console, F5 and try again with nearby points.


Actual results:

At some point, around ui.js:285, the x coordinate -2.842170943040401e-14 gets rounded to -1 using Math.round. (There is some related debug logging around there.) This is on Linux, x86.


Expected results:

Should round to 0.

(Sorry for the awful test-case, I didn't manage to minify it. Still, it reproduces very deterministically for me.)
(Reporter)

Comment 1

5 years ago
This is slightly smaller, automatic and deterministic: http://simonsoftware.se/other/nanoacre-fxbug3/
Just open the Web Console; an `if (Math.round(r) === -1) console.log(Math.round(r));` should proceed to print `0`.
(Reporter)

Comment 2

5 years ago
Bisection points to the baseline merge. (So Firefox 22 is fine.)
(Reporter)

Comment 3

5 years ago
Created attachment 764064 [details]
Test-case, run in jsshell

Errors as line 174.
(Reporter)

Comment 4

5 years ago
Alright, so I reduced it enough as to be runnable in jsshell, by replacing the canvas operations with eval(""). Let me know if there is anything else I can do to help.
(Assignee)

Updated

5 years ago
Attachment #764064 - Attachment mime type: application/octet-stream → text/javascript
(Assignee)

Comment 5

5 years ago
Simon, thanks a lot for reporting this and creating a shell testcase!

The testcase works for me though in a 32-bit shell, also tested on aurora. Can you tell me which revision you tested? 32-bit or 64-bit?
(Assignee)

Comment 6

5 years ago
Ah, I can reproduce the problem if I disable the SSE 4.1 LRound codegen path for negative numbers...
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 7

5 years ago
Testcase:

---
function f(x) {
    return Math.round(x);
}
assertEq(f(3.3), 3);
assertEq(f(-2.842170943040401e-14), -0);
---

Fails with --ion-eager and SSE 4.1 disabled:

Error: Assertion failed: got -1, expected -0
(Assignee)

Comment 8

5 years ago
Older problem but would be good to get this fixed in at least 23 and 24.
status-firefox23: --- → unaffected
status-firefox24: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
(Assignee)

Updated

5 years ago
status-firefox23: unaffected → affected
(Assignee)

Comment 9

5 years ago
Created attachment 764197 [details] [diff] [review]
Patch

Fixes the test and passes jit-tests with the SSE 4.1 path disabled.

This could use a careful review though..
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #764197 - Flags: review?(sstangl)
Comment on attachment 764197 [details] [diff] [review]
Patch

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

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +1313,4 @@
>          {
> +            // If input + 0.5 >= 0, input is a negative number >= -0.5 and the result is -0.
> +            Label negZero;
> +            masm.branchDouble(Assembler::DoubleGreaterThanOrEqual, temp, scratch, &negZero);

Could we make a bailoutIf() variant that would work here, instead of branching to the end, and having every other path jump over that code?
Attachment #764197 - Flags: review?(sstangl) → review+
(In reply to Jan de Mooij [:jandem] from comment #8)
> Older problem but would be good to get this fixed in at least 23 and 24.

We wouldn't block those releases on this fix, so not tracking. If low-risk, nominate patch for uplift.
status-firefox21: --- → affected
status-firefox22: --- → affected
tracking-firefox23: ? → -
tracking-firefox24: ? → -
(Reporter)

Comment 12

5 years ago
Just a reminder not to forget to land this.
(Assignee)

Comment 13

5 years ago
Pushed with comment addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a221ccef07

Green on Try with the SSE 4.1 path disabled:

https://tbpl.mozilla.org/?tree=Try&rev=33471e4cb2d3
https://hg.mozilla.org/mozilla-central/rev/b9a221ccef07
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.