Bug 114913

Summary: REGRESSION(r148790) Made 2 tests fail on x86 32bit
Product: WebKit Reporter: Allan Sandfeld Jensen <allan.jensen>
Component: JavaScriptCoreAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: NEW ---    
Severity: Normal CC: fpizlo, ggaren, kadam, mhahnenberg, msaboff, oliver, ossy, zarvai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 79666, 112239    
Attachments:
Description Flags
Patch
none
Patch fpizlo: review+

Description Allan Sandfeld Jensen 2013-04-21 05:49:49 PDT
The fast/canvas tests only confirmed failing on the Qt bots, while the two sputnik tests also fails on the GTK bot.

fast/canvas/canvas-arc-360-winding.html [ Failure ]
fast/canvas/canvas-fillPath-alpha-shadow.html [ Failure ]
fast/canvas/canvas-fillPath-gradient-shadow.html [ Failure ]
fast/canvas/canvas-fillPath-pattern-shadow.html [ Failure ]
fast/canvas/canvas-strokePath-alpha-shadow.html [ Failure ]
sputnik/Conformance/15_Native_Objects/15.8_Math/15.8.2/15.8.2.13_pow/S15.8.2.13_A24.html [ Failure ]
sputnik/Conformance/15_Native_Objects/15.8_Math/15.8.2/15.8.2.8_exp/S15.8.2.8_A6.html [ Failure ]
Comment 1 Allan Sandfeld Jensen 2013-04-22 06:35:42 PDT
The sputnik failure is related to bug 88519. The problem here being libc always uses x87 but doesn't configure the accuracy. Though unlike 88519 the problem is that it has too little accuracy if only given 64bit, exp and tan seems to need 80bit accuracy in x87 to give the right results back.
Comment 2 Allan Sandfeld Jensen 2013-04-22 06:38:53 PDT
Created attachment 199022 [details]
Patch
Comment 3 Allan Sandfeld Jensen 2013-04-22 07:15:14 PDT
The two sputnik tests works if resetX87Stack is implemented as "finit". Or if an the CPU status word is loaded directly with 0x03FF (80bit FPU precision).

The odd thing is though. That 0x02FF which we set the FPU precision to, is default on most operating systems. So the libc implementation is awefully fragile if it requires 80bit of FPU precision to give back the correct 64bit value.
Comment 4 Zoltan Arvai 2013-04-23 01:08:23 PDT
Tests marked to failure in http://trac.webkit.org/changeset/148948.
Comment 5 Zoltan Arvai 2013-04-23 01:32:15 PDT
For Ossy's suggestion failure mark changed to skip in http://trac.webkit.org/changeset/148950.
Please unskip it with proper fix.
Comment 6 Allan Sandfeld Jensen 2013-04-23 06:08:11 PDT
Created attachment 199223 [details]
Patch

Make the code more reliable by getting rid of the resteX87Stack instruction and instead free the two registers at every call pseudo instructions. Also unskip the tests now.
Comment 7 Allan Sandfeld Jensen 2013-04-26 08:02:57 PDT
Filip, any objections?
Comment 8 Allan Sandfeld Jensen 2013-05-13 09:59:34 PDT
Filip: Ping?
Comment 9 Allan Sandfeld Jensen 2013-08-14 04:12:30 PDT
I will have to apply this patch to the Qt snapshots of WebKit with or without review, since it fixes JS regressions.
Comment 10 Allan Sandfeld Jensen 2013-08-14 04:25:49 PDT
I strongly suspect this is also what makes the newly unskipped test in https://bugs.webkit.org/show_bug.cgi?id=119792 fail on 32bit x86.
Comment 11 Allan Sandfeld Jensen 2013-08-15 04:25:46 PDT
Thanks Filip :)
Comment 12 Allan Sandfeld Jensen 2013-08-15 04:27:50 PDT
Committed r154095: <http://trac.webkit.org/changeset/154095>
Comment 13 Allan Sandfeld Jensen 2013-08-15 04:30:13 PDT
Fixed the 5 tests caused by not resetting FPU registers.

There are still two failures left caused by libc calculating slightly differently in exp and pow when FPU precision is set to 64bit. We earlier set FPU precission to 64bit because two other sputnik tests tested that we did not have excessive precision.
Comment 14 Filip Pizlo 2013-08-15 09:02:41 PDT
(In reply to comment #13)
> Fixed the 5 tests caused by not resetting FPU registers.
> 
> There are still two failures left caused by libc calculating slightly differently in exp and pow when FPU precision is set to 64bit. We earlier set FPU precission to 64bit because two other sputnik tests tested that we did not have excessive precision.

Hmmm.  ECMAScript allows for all transcendental Math functions to just have reasonable precision; if Test262 is testing specific results then the test is wrong.  ECMAScript doesn't make any specific precision requirements.

So, it may not even be required to set FPU precision to 64bit, unless the failing tests are tests of basic arithmetic (+, -, *, /, etc), which is the only place where ECMAScript actually requires 64bit precision.  For Math tests, we should happily allow the tests to fail on x87, and we should lobby the Test262 folks to ammend the test to allow for wider precision.
Comment 15 Allan Sandfeld Jensen 2013-08-15 09:25:53 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > Fixed the 5 tests caused by not resetting FPU registers.
> > 
> > There are still two failures left caused by libc calculating slightly differently in exp and pow when FPU precision is set to 64bit. We earlier set FPU precission to 64bit because two other sputnik tests tested that we did not have excessive precision.
> 
> Hmmm.  ECMAScript allows for all transcendental Math functions to just have reasonable precision; if Test262 is testing specific results then the test is wrong.  ECMAScript doesn't make any specific precision requirements.
> 
> So, it may not even be required to set FPU precision to 64bit, unless the failing tests are tests of basic arithmetic (+, -, *, /, etc), which is the only place where ECMAScript actually requires 64bit precision.  For Math tests, we should happily allow the tests to fail on x87, and we should lobby the Test262 folks to ammend the test to allow for wider precision.

Unfortunately the two other tests are testing that basic arithmetics lose precision see https://bugs.webkit.org/show_bug.cgi?id=112239#c55 and https://bugs.webkit.org/show_bug.cgi?id=112239#c57