Bug 203406 – Chakracore test max.js broken in armv7 and mips builds of JSC

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Paulo Matos]

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

[Paulo Matos]

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.

[Xan Lopez]

CCing Keith and Mark in case this is immediately obvious to them, we'll have a look at it ASAP.

[Xan Lopez]

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

[Xan Lopez]

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

[Mark Lam]

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.

[Xan Lopez]

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

[Guillaume Emont]

As gardening, skipping test in Bug 203636 until this is fixed.

[Xan Lopez]

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.

[Mark Lam]

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

[Xan Lopez]

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)

[Xan Lopez]

*** This bug has been marked as a duplicate of bug 204457 ***