Bug 61416

Summary: [JSC] malfunction during arithmetic condition check with negative number (-2147483648)
Product: WebKit Reporter: Hojong Han <hojong.han>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, commit-queue, ggaren, gyuyoung.kim, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Adding the condition to check 0x80000000 when making ARM instruction CMN for JIT
none
Patch
none
Patch
none
Patch none

Description Hojong Han 2011-05-24 21:23:28 PDT
Example below doesn't work properly
------------------------------------------------------------------
<script type="text/javascript">
  var f = 10;
  if (f < -2147483648) {alert("wrong");}
</script>
------------------------------------------------------------------

The condition to check 0x80000000 is necessary when making CMN instruction for JIT.
Comment 1 Hojong Han 2011-05-25 01:47:24 PDT
Created attachment 94754 [details]
Adding the condition to check 0x80000000 when making ARM instruction CMN for JIT
Comment 2 Gyuyoung Kim 2011-05-25 02:50:35 PDT
Comment on attachment 94754 [details]
Adding the condition to check 0x80000000 when making ARM instruction CMN for JIT

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

> Source/JavaScriptCore/ChangeLog:3
> +        Not reviewed.

Please Change "Not reviewed" with Reviewed by NOBODY (OOPS!).
Comment 3 Hojong Han 2011-05-25 03:36:26 PDT
Comment on attachment 94754 [details]
Adding the condition to check 0x80000000 when making ARM instruction CMN for JIT

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 87279)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>+2011-05-25  Hojong Han  <hojong.han@samsung.com>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Adding the condition to check 0x80000000 when making ARM instruction CMN for JIT.
>+        https://bugs.webkit.org/show_bug.cgi?id=61416
>+
>+        * assembler/MacroAssemblerARM.h:
>+        (JSC::MacroAssemblerARM::branch32):
>+
> 2011-05-24  Keishi Hattori  <keishi@webkit.org>
> 
>         Reviewed by Kent Tamura.
>Index: Source/JavaScriptCore/assembler/MacroAssemblerARM.h
>===================================================================
>--- Source/JavaScriptCore/assembler/MacroAssemblerARM.h	(revision 87276)
>+++ Source/JavaScriptCore/assembler/MacroAssemblerARM.h	(working copy)
>@@ -416,7 +416,7 @@
>             m_assembler.ldr_un_imm(ARMRegisters::S0, right.m_value);
>             m_assembler.cmp_r(left, ARMRegisters::S0);
>         } else {
>-            ARMWord tmp = m_assembler.getOp2(-right.m_value);
>+            ARMWord tmp = (right.m_value == 0x80000000)?ARMAssembler::INVALID_IMM:m_assembler.getOp2(-right.m_value);
>             if (tmp != ARMAssembler::INVALID_IMM)
>                 m_assembler.cmn_r(left, tmp);
>             else
Comment 4 Hojong Han 2011-05-25 03:51:20 PDT
Created attachment 94768 [details]
Patch
Comment 5 Geoffrey Garen 2011-05-25 12:05:40 PDT
+            ARMWord tmp = (right.m_value == 0x80000000)?ARMAssembler::INVALID_IMM:m_assembler.getOp2(-right.m_value);

Spacing doesn't match the WebKit style guidelines. There should be spaces around "?" and ":".
Comment 6 Gavin Barraclough 2011-05-25 12:22:31 PDT
A fix like this really ought to be accompanied by a regression test.

This can be really easy, you should be able to just find a test in the LayoutTests/fast/js directory with 'arithmetic' or 'comparisons' in the name, add something like:

var testValue = 10;
shouldBeFalse("testValue < -2147483648");

And run the layout test to get updated results (which will just be a line saying PASS).

cheers,
G.
Comment 7 Hojong Han 2011-05-26 01:59:52 PDT
Created attachment 94944 [details]
Patch
Comment 8 Hojong Han 2011-05-26 02:05:00 PDT
Thank you for comment. 
I ran "Tools/Script/check-webkit-style", there're mistakes though.
> +            ARMWord tmp = (right.m_value == 0x80000000)?ARMAssembler::INVALID_IMM:m_assembler.getOp2(-right.m_value);
> 
> Spacing doesn't match the WebKit style guidelines. There should be spaces around "?" and ":".
Comment 9 Hojong Han 2011-05-26 02:18:14 PDT
I'm appreciated your comment and trying to set layout regression test,
but there's one thing difficult for me is that this issue happens at elf port with ARM and there is not an easy way to run the test.
Could you let me know another way for test or something I don't catch?
> A fix like this really ought to be accompanied by a regression test.
> 
> This can be really easy, you should be able to just find a test in the LayoutTests/fast/js directory with 'arithmetic' or 'comparisons' in the name, add something like:
> 
> var testValue = 10;
> shouldBeFalse("testValue < -2147483648");
> 
> And run the layout test to get updated results (which will just be a line saying PASS).
> 
> cheers,
> G.
Comment 10 Gyuyoung Kim 2011-05-26 02:23:00 PDT
(In reply to comment #9)
> I'm appreciated your comment and trying to set layout regression test,
> but there's one thing difficult for me is that this issue happens at elf port with ARM and there is not an easy way to run the test.
> Could you let me know another way for test or something I don't catch?
> > A fix like this really ought to be accompanied by a regression test.
> > 
> > This can be really easy, you should be able to just find a test in the LayoutTests/fast/js directory with 'arithmetic' or 'comparisons' in the name, add something like:
> > 
> > var testValue = 10;
> > shouldBeFalse("testValue < -2147483648");
> > 
> > And run the layout test to get updated results (which will just be a line saying PASS).
> > 
> > cheers,
> > G.

EFL port doesn't have DRT yet. But, DRT  for EFL port is going to contribute WebKit mainline soon. I think you may be able to test arthmetic test by EWebLauncher or jsc executable manually.
Comment 11 Gyuyoung Kim 2011-05-26 02:31:32 PDT
> EFL port doesn't have DRT yet. But, DRT  for EFL port is going to contribute WebKit mainline soon. I think you may be able to test arthmetic test by EWebLauncher or jsc executable manually.

Or, you can use DRT in GTK port.
Comment 12 Hojong Han 2011-05-26 06:41:24 PDT
(In reply to comment #6)
> A fix like this really ought to be accompanied by a regression test.
> 
> This can be really easy, you should be able to just find a test in the LayoutTests/fast/js directory with 'arithmetic' or 'comparisons' in the name, add something like:
> 
> var testValue = 10;
> shouldBeFalse("testValue < -2147483648");
> 
> And run the layout test to get updated results (which will just be a line saying PASS).
> 
> cheers,
> G.

I added test code like below into the JS file (Source/JavaScriptCore/tests/mozilla/ecma/Array/15.4.1.1.js) for JSC regression test and then ran "./jsc_efl -s -f shell.js 15.4.1.1.js"

[Test case code]
array[item++] = new TestCase( SECTION,	"var f = 10; var g; if(f < -2147483648) g = false; else g = true; g.toString()",  "true",  eval("var f = 10; var g; if(f < -2147483648) g = false; else g = true; g.toString()") );

[Before fixed]
var f = 10; var g; if(f < -2147483648) g = false; else g = true; g.toString() = false FAILED! expected: true

[After fixed]
var f = 10; var g; if(f < -2147483648) g = false; else g = true; g.toString() = true PASSED!
Comment 13 Gavin Barraclough 2011-05-26 16:31:56 PDT
Comment on attachment 94944 [details]
Patch

Under the circumstances, adding this to a jsc test seems fine to me.  Please put up a patch containing the added test case & new test results too.
Comment 14 Hojong Han 2011-05-26 22:58:57 PDT
Created attachment 95118 [details]
Patch
Comment 15 Hojong Han 2011-05-26 23:07:44 PDT
(In reply to comment #13)
> (From update of attachment 94944 [details])
> Under the circumstances, adding this to a jsc test seems fine to me.  Please put up a patch containing the added test case & new test results too.

I added new patch including test case, except test results because newly added test case with a fix doesn't throw any message when it's passed.
Comment 16 Zoltan Herczeg 2011-05-26 23:31:25 PDT
Nice catch! -INT_MIN == INT_MIN
I like this patch, although I can't give you an r+. Hopefully someone will do.
Comment 17 Gyuyoung Kim 2011-05-29 22:41:02 PDT
(In reply to comment #16)
> Nice catch! -INT_MIN == INT_MIN
> I like this patch, although I can't give you an r+. Hopefully someone will do.

Hello Geoffrey, 

How do you think about this patch ? It seems to me Zoltan give an internal review "+".
Comment 18 Geoffrey Garen 2011-05-30 16:15:17 PDT
Comment on attachment 95118 [details]
Patch

r=me
Comment 19 WebKit Commit Bot 2011-05-30 16:50:03 PDT
Comment on attachment 95118 [details]
Patch

Clearing flags on attachment: 95118

Committed r87702: <http://trac.webkit.org/changeset/87702>
Comment 20 WebKit Commit Bot 2011-05-30 16:50:10 PDT
All reviewed patches have been landed.  Closing bug.