Closed Bug 870356 Opened 7 years ago Closed 7 years ago

Math.round gets miscompiled


(Core :: JavaScript Engine, defect)

23 Branch
Not set



Tracking Status
firefox21 --- affected
firefox22 --- affected
firefox23 - affected
firefox24 - affected


(Reporter: simon.lindholm10, Assigned: jandem)



(2 files)

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.)
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`.
Bisection points to the baseline merge. (So Firefox 22 is fine.)
Errors as line 174.
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.
Attachment #764064 - Attachment mime type: application/octet-stream → text/javascript
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?
Ah, I can reproduce the problem if I disable the SSE 4.1 LRound codegen path for negative numbers...
Ever confirmed: true

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
Older problem but would be good to get this fixed in at least 23 and 24.
Attached 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.
Just a reminder not to forget to land this.
Pushed with comment addressed:

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