Math.round gets miscompiled

RESOLVED FIXED in mozilla25



6 years ago
6 years ago


(Reporter: simon.lindholm10, Assigned: jandem)


23 Branch
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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



(2 attachments)



6 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 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.)

Comment 1

6 years ago
This is slightly smaller, automatic and deterministic:
Just open the Web Console; an `if (Math.round(r) === -1) console.log(Math.round(r));` should proceed to print `0`.

Comment 2

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

Comment 3

6 years ago
Errors as line 174.

Comment 4

6 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.


6 years ago
Attachment #764064 - Attachment mime type: application/octet-stream → text/javascript

Comment 5

6 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?

Comment 6

6 years ago
Ah, I can reproduce the problem if I disable the SSE 4.1 LRound codegen path for negative numbers...
Ever confirmed: true

Comment 7

6 years ago

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

Comment 8

6 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: --- → ?


6 years ago
status-firefox23: unaffected → affected

Comment 9

6 years ago
Posted patch PatchSplinter Review
Fixes the test and passes jit-tests with the SSE 4.1 path disabled.

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

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: ? → -

Comment 12

6 years ago
Just a reminder not to forget to land this.

Comment 13

6 years ago
Pushed with comment addressed:

Green on Try with the SSE 4.1 path disabled:
Last Resolved: 6 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.