Bug 105087 - Some Math.pow test262 tests fail with MinGW-w64
Summary: Some Math.pow test262 tests fail with MinGW-w64
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 103747
  Show dependency treegraph
 
Reported: 2012-12-15 05:44 PST by Jonathan Liu
Modified: 2013-01-02 08:15 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.81 KB, patch)
2012-12-15 05:51 PST, Jonathan Liu
no flags Details | Formatted Diff | Diff
Patch (1.81 KB, patch)
2012-12-15 05:55 PST, Jonathan Liu
no flags Details | Formatted Diff | Diff
Patch (1.82 KB, patch)
2012-12-15 06:00 PST, Jonathan Liu
no flags Details | Formatted Diff | Diff
Patch (1.81 KB, patch)
2012-12-17 02:13 PST, Jonathan Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Liu 2012-12-15 05:44:35 PST
The following test262 tests fail with the latest MinGW-w64 runtime:
-S15.8.2.13_A14 Checking if Math.pow(x,y) equals to +Infinity, where x is -Infinity and y>0
-S15.8.2.13_A16 Checking if Math.pow(x,y) equals to +0, where x is -Infinity and y<0
-S15.8.2.13_A20 Checking if Math.pow(x,y) equals to +0, where x is -0 and y>0 and y is NOT an odd integer
-S15.8.2.13_A22 Checking if Math.pow(x,y) equals to +Infinity, where x is -0 and y<0 and y is NOT an odd integer

This is due to different behavior of pow() with the MinGW-w64 runtime compared with other C runtimes. I have already fixed this issue in Google V8 (http://code.google.com/p/v8/source/browse/trunk/src/assembler.cc?spec=svn13185&r=13185#1397).
Comment 1 Jonathan Liu 2012-12-15 05:51:21 PST
Created attachment 179590 [details]
Patch
Comment 2 WebKit Review Bot 2012-12-15 05:53:40 PST
Attachment 179590 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/runtime/MathObject.cpp:236:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/MathObject.cpp:245:  y_int is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/runtime/MathObject.cpp:246:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jonathan Liu 2012-12-15 05:55:44 PST
Created attachment 179592 [details]
Patch
Comment 4 WebKit Review Bot 2012-12-15 05:58:02 PST
Attachment 179592 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/runtime/MathObject.cpp:236:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/MathObject.cpp:246:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Jonathan Liu 2012-12-15 06:00:05 PST
Created attachment 179593 [details]
Patch
Comment 6 Simon Hausmann 2012-12-17 01:42:52 PST
Comment on attachment 179593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179593&action=review

> Source/JavaScriptCore/runtime/MathObject.cpp:235
> +#ifdef __MINGW64_VERSION_MAJOR

I think this should be #if COMPILER(MINGW64)

> Source/JavaScriptCore/runtime/MathObject.cpp:244
> +    if ((x == 0.0 || isinf(x)) && isfinite(y)) {
> +        double f;
> +        if (modf(y, &f) != 0.0)
> +            return ((x == 0.0) ^ (y > 0.0)) ? std::numeric_limits<double>::infinity() : 0.0;
> +    }
> +
> +    if (x == 2.0) {

Isn't it dangerous to compare doubles to constants like that? Shouldn't it be done as a range comparison using a epsilon?
Comment 7 Jonathan Liu 2012-12-17 02:13:05 PST
Created attachment 179707 [details]
Patch
Comment 8 Jonathan Liu 2012-12-17 02:19:56 PST
The special cases apply only to exact values so it is fine in my opinion.
Comment 9 Simon Hausmann 2012-12-17 03:29:10 PST
Upstream "discussion" about the behavioral difference: http://comments.gmane.org/gmane.comp.gnu.mingw.w64.general/4658
Comment 10 WebKit Review Bot 2012-12-17 03:51:02 PST
Comment on attachment 179707 [details]
Patch

Clearing flags on attachment: 179707

Committed r137895: <http://trac.webkit.org/changeset/137895>
Comment 11 WebKit Review Bot 2012-12-17 03:51:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Sam Weinig 2012-12-27 20:35:54 PST
I don't think this is the best way to do this.  The preferred method is to add a function to MathExtras.h which implements pow correctly and call that, so that we don't litter the code with #ifdefs.
Comment 13 Jonathan Liu 2012-12-29 18:58:51 PST
(In reply to comment #12)
> I don't think this is the best way to do this.  The preferred method is to add a function to MathExtras.h which implements pow correctly and call that, so that we don't litter the code with #ifdefs.

According to the bug life cycle, this bug is fixed. Feel free to commit such a change separately.
Comment 14 Simon Hausmann 2013-01-02 08:15:36 PST
I've filed bug #105925 to address Sam's (valid) comment.