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
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.
CCing Keith and Mark in case this is immediately obvious to them, we'll have a look at it ASAP.
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).
Created attachment 382065 [details] 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
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.
(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)
As gardening, skipping test in Bug 203636 until this is fixed.
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.
(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.
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)
*** This bug has been marked as a duplicate of bug 204457 ***