Bug 61416 – [JSC] malfunction during arithmetic condition check with negative number (-2147483648)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Hojong Han]

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.

[Hojong Han]

Created attachment 94754 [details]
Adding the condition to check 0x80000000 when making ARM instruction CMN for JIT

[Gyuyoung Kim]

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!).

[Hojong Han]

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 @@
>[phone]  Hojong Han  <[e-mail]>
>+
>+        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  <[e-mail]>
> 
>         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

[Hojong Han]

Created attachment 94768 [details]
Patch

[Geoffrey Garen]

+            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 ":".

[Gavin Barraclough]

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.

[Hojong Han]

Created attachment 94944 [details]
Patch

[Hojong Han]

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 ":".

[Hojong Han]

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.

[Gyuyoung Kim]

(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.

[Gyuyoung Kim]

> 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.

[Hojong Han]

(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!

[Gavin Barraclough]

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.

[Hojong Han]

Created attachment 95118 [details]
Patch

[Hojong Han]

(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.

[Zoltan Herczeg]

Nice catch! -INT_MIN == INT_MIN
I like this patch, although I can't give you an r+. Hopefully someone will do.

[Gyuyoung Kim]

(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 "+".

[Geoffrey Garen]

Comment on attachment 95118 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 95118 [details]
Patch

Clearing flags on attachment: 95118

Committed r87702: <http://trac.webkit.org/changeset/87702>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.