IonMonkey: Incorrect fround(asin())

RESOLVED FIXED in Firefox 27



JavaScript Engine
4 years ago
4 years ago


(Reporter: Jesse Ruderman, Assigned: bbouvier)


(Blocks: 2 bugs, {regression, testcase})

Mac OS X
regression, testcase
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox26 unaffected, firefox27 fixed, firefox28 fixed)


(Whiteboard: [qa-])


(2 attachments, 1 obsolete attachment)



4 years ago
With --ion-eager, fails with what might be a rounding difference:

function f() { return Math.fround(Math.asin(0.015625)) }
assertEq(f(), f());

With --ion-eager, fails with a much larger difference:

function g() { return Math.fround(Math.asin(2)) }
assertEq(isNaN(g()), true);
assertEq(isNaN(g()), true);

Comment 1

4 years ago
Created attachment 8335736 [details]
bisect log

Of these 8 changesets, I suspect the regressor was:

user:        Benjamin Bouvier
date:        Thu Oct 17 08:50:56 2013 +0200
summary:     [Bug 918163] - Specialize some Maths function calls for Float32 in Ion. r=sstangl

Comment 2

4 years ago
Unlike arithmetic, sin/cos/etc do not have a exactly-defined result and there is not a commutative identity as there is with +,-,*,/.  Asking the Intel FP people about this, they said that there is a standard unit of variation accepted for trig functions (in any C standard library) and that, for fround'ed values, sin(float) vs. sin(double) would be within this unit of variance.  So, depending on what the delta is, that would explain the first example.

The second example is more concerning.
> Created attachment 8335736 [details]
> bisect log

I pushed fuzzing rev 8053b2cd842f to ignore these changesets.

Comment 4

4 years ago
Should we disable the f32 optimization for trig functions, or should we accept that fround(trig*()) is unstable and try to disable differential fuzzing for that combination?

Comment 5

4 years ago
Created attachment 8335985 [details] [diff] [review]
Best typo ever

Ahem. Typos! Very nice find.

That made me realize that the Float32 tests that check correctness for the trigo functions were not asserting. I created a new test file that I will land with this patch, once I'll be sure that it doesn't break any platform (try).
Assignee: nobody → benj
Attachment #8335985 - Flags: review?(luke)

Comment 6

4 years ago
Haha, turns out it was a real bug :)  As for the observability of sin vs. sinf: this should be within the spec and within expectations, so I think we're fine.  What I don't know is how easy it is to observe; we can wait and see if the fuzzers find anything but, if they do it should be a small delta so maybe we could use an assertWithinEpsilon() function instead of assertEq().

Comment 7

4 years ago
Comment on attachment 8335985 [details] [diff] [review]
Best typo ever

Haha.  Can you remove the extra space before 'break' so they all line up?  r+ with test.
Attachment #8335985 - Flags: review?(luke) → review+

Comment 8

4 years ago
Per the release calendar, setting status flags.
status-firefox26: --- → unaffected
status-firefox27: --- → affected
status-firefox28: --- → affected

Comment 9

4 years ago
Tests all green on try!
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28


4 years ago
status-firefox28: affected → fixed
Flags: in-testsuite+
Whiteboard: [qa-]

Comment 11

4 years ago
Created attachment 8371371 [details] [diff] [review]
Uplift patch

[Approval Request Comment]
Regression caused by (bug #): 918163
User impact if declined: important correctness issues
Testing completed (on m-c, etc.): m-i and m-c for a while
Risk to taking this patch (and alternatives if risky): no risk
String or IDL/UUID changes made by this patch: n/a

Got R+ from luke.
Attachment #8335985 - Attachment is obsolete: true
Attachment #8371371 - Flags: review+
Attachment #8371371 - Flags: approval-mozilla-release?

Comment 12

4 years ago
FWIW, this has been noticed by users, in bug 968522.
Blocks: 968522
Comment on attachment 8371371 [details] [diff] [review]
Uplift patch

We can take this as a ride-along with the stability-driven 27.0.1 release that will go out at the end of this week.  Please land this to the mozilla-release branch asap in order for it to get into tomorrow's builds.
Attachment #8371371 - Flags: approval-mozilla-release? → approval-mozilla-release+
Keywords: checkin-needed

Comment 14

4 years ago
Keywords: checkin-needed
I was hoping you'd land the tests for this too. I was going to push this with tests this morning.
status-firefox27: affected → fixed
Flags: needinfo?(benj)

Comment 16

4 years ago
Sure thing!
Flags: needinfo?(benj)

Comment 17

4 years ago
^ Annnnnd this is how I break mozilla-release... :(

And this is how I fix it:

FTR, it's a part of one of the patches in bug 930477 (landed one month before this landing, that's why we didn't catch it here), where the same issue showed up.

Should I ask for an a-posteriori approval for this one?
(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> And this is how I fix it:

Looks like it worked, thanks :)

> Should I ask for an a-posteriori approval for this one?

Nah, I think a=bustage and passing tests is good enough.


4 years ago
Duplicate of this bug: 968522
You need to log in before you can comment on or make changes to this bug.