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.
Created attachment 94754 [details] Adding the condition to check 0x80000000 when making ARM instruction CMN for JIT
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 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
Created attachment 94768 [details] Patch
+ 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 ":".
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.
Created attachment 94944 [details] Patch
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 ":".
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.
(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.
> 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.
(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 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.
Created attachment 95118 [details] Patch
(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.
Nice catch! -INT_MIN == INT_MIN I like this patch, although I can't give you an r+. Hopefully someone will do.
(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 on attachment 95118 [details] Patch r=me
Comment on attachment 95118 [details] Patch Clearing flags on attachment: 95118 Committed r87702: <http://trac.webkit.org/changeset/87702>
All reviewed patches have been landed. Closing bug.