203406 2019-10-25 04:37:51 -0700 Chakracore test max.js broken in armv7 and mips builds of JSC 2020-07-17 01:20:00 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Other Unspecified RESOLVED DUPLICATE 204457 P2 Major --- 204457 1 pmatos webkit-unassigned guijemont keith_miller mark.lam xan.lopez oldest_to_newest 1583762 0 pmatos 2019-10-25 04:37:51 -0700 A recent patch has broken armv7 - chakracore test max.js is giving incorrect results: Bad behaviour: $ jsc --useDFGJIT=true --forceEagerCompilation=true --repatchBufferingCountdown=6 JSTests/ChakraCore/test/jsc-lib.js JSTests/ChakraCore/test/Math/max.js Infinity -Infinity opD = 0 , onD = 0 op = 0 , on = 0 , infp = Infinity , infn = -Infinity , infpD = Infinity , infnD = -Infinity Math.max(+0.0, -0.0) 0 Check (1 / Math.max(+0.0, -0.0) < 0) - false done Infinity Infinity -Infinity -Infinity undefined undefined NaN 1 Changing repatchBufferingCountdown to 7 gives the correct result: $ jsc --useDFGJIT=true --forceEagerCompilation=true --repatchBufferingCountdown=7 JSTests/ChakraCore/test/jsc-lib.js JSTests/ChakraCore/test/Math/max.js Infinity -Infinity opD = 0 , onD = 0 op = 0 , on = 0 , infp = Infinity , infn = -Infinity , infpD = Infinity , infnD = -Infinity Math.max(+0.0, -0.0) 0 Check (1 / Math.max(+0.0, -0.0) < 0) - false done Infinity Infinity Infinity -Infinity undefined undefined NaN 1 Notice the change from -Infinity to Infinity. Disabling DFGJit altogether also returns a good result. $ jsc --useDFGJIT=false JSTests/ChakraCore/test/jsc-lib.js JSTests/ChakraCore/test/Math/max.js Infinity -Infinity opD = 0 , onD = 0 op = 0 , on = 0 , infp = Infinity , infn = -Infinity , infpD = Infinity , infnD = -Infinity Math.max(+0.0, -0.0) 0 Check (1 / Math.max(+0.0, -0.0) < 0) - false done Infinity Infinity Infinity -Infinity undefined undefined NaN 1 1583774 1 pmatos 2019-10-25 05:10:40 -0700 This seems to have been introduced at r251468. This hash introduced a build failure so it's difficult to confirm this is the case but once the build failure was fixed we had this test failure. Before this commit at r251467 the failure didn't exist. 1583839 2 xan.lopez 2019-10-25 08:57:53 -0700 CCing Keith and Mark in case this is immediately obvious to them, we'll have a look at it ASAP. 1584399 3 xan.lopez 2019-10-28 02:49:10 -0700 FWIW I see the same result on x86-64, the result changes from Infinity to -Infinity when enabling DFG (the repatchBufferingCountdown option has no effect). 1584430 4 382065 xan.lopez 2019-10-28 04:33:08 -0700 Created attachment 382065 Reduced testcase I have reduced the test as much as I could. Seems it's relevant that there are a bunch of calls to Math.max before the bug (they can be any kind of calls, see the loop), and we need the previous foo() calls for the bug to be triggered. With no DFG the result is correct, with DFG we need the forceEagerCompilation parameters to see the bug: niraikanai:~/git/WebKit%./WebKitBuild/Debug/bin/jsc --useDFGJIT=false JSTests/ChakraCore/test/jsc-lib.js ~/js/chakra.max.js Infinity Infinity Infinity niraikanai:~/git/WebKit%./WebKitBuild/Debug/bin/jsc --useDFGJIT=true --forceEagerCompilation=true JSTests/ChakraCore/test/jsc-lib.js ~/js/chakra.max.js Infinity Infinity -Infinity 1584563 5 mark.lam 2019-10-28 13:03:55 -0700 Xan, in case you haven't already read this, check out: https://webkit.org/blog/6411/javascriptcore-csi-a-crash-site-investigation-story/ It details steps for debugging that you might find useful for this bug. 1584879 6 xan.lopez 2019-10-29 03:45:58 -0700 (In reply to Mark Lam from comment #5) > Xan, in case you haven't already read this, check out: > https://webkit.org/blog/6411/javascriptcore-csi-a-crash-site-investigation- > story/ > > It details steps for debugging that you might find useful for this bug. Yes, thank you. Seems the bug is in DFG, with the eager compilation parameters some optimization pass must be triggered that generates slightly different (and buggy) DFG code. Almost all of it is identical though, so hopefully it won't be that hard to narrow it down. (Also, I guess that means the bug probably is in the parts of the bytecode index patch that touch DFG classes) 1585588 7 guijemont 2019-10-30 15:34:12 -0700 As gardening, skipping test in Bug 203636 until this is fixed. 1592321 8 xan.lopez 2019-11-21 02:30:26 -0800 Hey, a bit more info about this: On x86-64 what's happening is that with the eager parameters we optimize the Math.max call, using what's in DFGSpeculativeJIT. For that platform the Math.max check is done with two comparisons, using the UCOMISD instruction. This instruction says 0.0 and -0.0 are equal, so the function just returns whatever value (which randomly turns out to be -0.0). This is wrong and gives us the -Infinity. (I have not checked but I assume something similar is happening in other architectures) A couple of questions: - I assume the fact that the DFG Math.max cannot do this properly is intentional? The code has been there for a while. In that case I suppose the bug is that we end up there. - If it's not intentional I guess we just figured out and need to change it. - I really don't understand how the BytecodeIndex patch ended up causing this. 1592416 9 mark.lam 2019-11-21 09:51:39 -0800 (In reply to Xan Lopez from comment #8) > Hey, a bit more info about this: > > On x86-64 what's happening is that with the eager parameters we optimize the > Math.max call, using what's in DFGSpeculativeJIT. For that platform the > Math.max check is done with two comparisons, using the UCOMISD instruction. > This instruction says 0.0 and -0.0 are equal, so the function just returns > whatever value (which randomly turns out to be -0.0). This is wrong and > gives us the -Infinity. > > (I have not checked but I assume something similar is happening in other > architectures) > > A couple of questions: > - I assume the fact that the DFG Math.max cannot do this properly is > intentional? The code has been there for a while. In that case I suppose the > bug is that we end up there. > - If it's not intentional I guess we just figured out and need to change it. > - I really don't understand how the BytecodeIndex patch ended up causing > this. It is incorrect for ArithMax to treat -0 as the same as 0. ArithMax is supposed to work like Math.max(), and https://www.ecma-international.org/ecma-262/6.0/#sec-math.max says that Math.max() should treat 0 > -0. Thanks for pointing out this issue. We'll address this issue for x86_64 (and Apple supported ports) in https://bugs.webkit.org/show_bug.cgi?id=204457. 1592422 10 xan.lopez 2019-11-21 10:12:30 -0800 Great. I'm still a bit puzzled about what changed for us to see this (incorrect) behavior now, since the code has been there for more than a year AFAICT. Probably would like to understand that to make sure there are no more lingering bugs here. (I'll CC in the other bug in case the fix has architecture dependent bits) 1672428 11 xan.lopez 2020-07-17 01:20:00 -0700 *** This bug has been marked as a duplicate of bug 204457 *** 382065 2019-10-28 04:33:08 -0700 2019-10-28 04:33:08 -0700 Reduced testcase chakra.max.js text/javascript 204 xan.lopez Zm9yICh2YXIgaT0wOyBpIDwgNjsgaSsrKQp7CiAgICBNYXRoLm1heChpKTsKfQoKLy9uZWdhdGl2 ZSB6ZXJvCmZ1bmN0aW9uIGZvbyhhLGIpCnsKICAgIHZhciBjID0gTWF0aC5tYXgoYSxiKTsKICAg IHJldHVybiAxL2M7Cn0KV1NjcmlwdC5FY2hvKGZvbygtMCwwKSk7CldTY3JpcHQuRWNobyhmb28o LTAsMCkpOwpXU2NyaXB0LkVjaG8oZm9vKDAsLTApKTsK