Bug 86330 – DFG performs incorrect constant folding on double-to-uint32 conversion in Uint32Array PutByVal

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Félix Cloutier]

Using r116899.

I hope you won't hate me too much for this bug report. I've been told on #webkit to report it anyways.

I'm working on a Playstation emulator written in Javascript. It generates Javascript code from MIPS instructions and executes them; but when the debugger is not attached, the execution goes wrong. At some point, the value 0x80054664 should be assigned to element #31 of a Uint32Array (called "this.gpr") through a very simple statement:

    this.gpr[31] = 0x8005465c;

But the statement is either not executed or does not write the correct value, because instead I get this.gpr[31] == 0x80000000, which is the value it had before the assignment.

When the code is run with the Javascript debugger enabled, it does write 0x80054664, so the problem seems to be with the "production" JS compiler.

Also, when this.gpr[31] is not a typed array element (like if I make this.gpr an object with getters and setters for indices 0-33), everything goes right.

The bug is also resilient to mocking. Setting up the state of the emulator to something that simply "looks like" the conditions in which the bug happens (mocking the GPR state and the function generated) doesn't trigger it. I would very much like to give you a complete test case, but for this I would need to send a PlayStation BIOS, and I can't do that.

Still, I've joined the URL of the mock, so you can get an idea of what's going on. The generated Javascript code that doesn't work under obscure circumstances can be found in mock.js (the "psx.memory.compiled.compiled[0x8005465c]" part). For all practical purposes, the only part of this function that should be executed is from line 230 to line 233.

If there's any way I could be more helpful, please tell me how, I'll gladly do it.

[Filip Pizlo]

(In reply to comment #0)
> Using r116899.
> 
> I hope you won't hate me too much for this bug report. I've been told on #webkit to report it anyways.
> 
> I'm working on a Playstation emulator written in Javascript. It generates Javascript code from MIPS instructions and executes them; but when the debugger is not attached, the execution goes wrong. At some point, the value 0x80054664 should be assigned to element #31 of a Uint32Array (called "this.gpr") through a very simple statement:
> 
>     this.gpr[31] = 0x8005465c;
> 
> But the statement is either not executed or does not write the correct value, because instead I get this.gpr[31] == 0x80000000, which is the value it had before the assignment.
> 
> When the code is run with the Javascript debugger enabled, it does write 0x80054664, so the problem seems to be with the "production" JS compiler.
> 
> Also, when this.gpr[31] is not a typed array element (like if I make this.gpr an object with getters and setters for indices 0-33), everything goes right.
> 
> The bug is also resilient to mocking. Setting up the state of the emulator to something that simply "looks like" the conditions in which the bug happens (mocking the GPR state and the function generated) doesn't trigger it. I would very much like to give you a complete test case, but for this I would need to send a PlayStation BIOS, and I can't do that.
> 
> Still, I've joined the URL of the mock, so you can get an idea of what's going on. The generated Javascript code that doesn't work under obscure circumstances can be found in mock.js (the "psx.memory.compiled.compiled[0x8005465c]" part). For all practical purposes, the only part of this function that should be executed is from line 230 to line 233.
> 
> If there's any way I could be more helpful, please tell me how, I'll gladly do it.

Hi Felix!  Thanks for the detailed bug report!  I think I just found the bug.  In dfg/DFGSpeculativeJIT.cpp, there is the following code:

        JSValue jsValue = valueOfJSConstant(valueUse.index());
        if (!jsValue.isNumber()) {
            terminateSpeculativeExecution(Uncountable, JSValueRegs(), NoNode);
            noResult(m_compileIndex);
            return;
        }
        double d = jsValue.asNumber();
        if (rounding == ClampRounding) {
            ASSERT(elementSize == 1);
            d = clampDoubleToByte(d);
        }
        GPRTemporary scratch(this);
        GPRReg scratchReg = scratch.gpr();
        m_jit.move(Imm32(static_cast<int>(d)), scratchReg);
        value.adopt(scratch);
        valueGPR = scratchReg;

This is in the compilePutByValForIntTypedArray() method.  I believe that the offending line is "m_jit.move(Imm32(static_cast<int>(d)), scratchReg);".  Basically, we're doing a C++ cast from a double value (we will treat 0x8005465c as a double because it is outside the int32 range) to an int.  But C++ casting, at least on most common architectures (x86), implies that double values outside the int32 range result in 0x80000000, which is a special signaling value indicating that the cast overflowed.

I believe that the correct fix is to change "static_cast<int>(d)" to "toInt32(d)".

But to confirm that this is really the problem, can you tell me if the following hold true:

1) The real code (not the mocked version) does not have a switch statement.  Currently our optimizing compiler does not yet support switch statements.  This might be the reason why your mocked code does not trigger the bug.

2) The real code does indeed store a constant into the UInt32Array, as opposed to storing a non-constant value.  In particular, the code does this:

this.gpr[31] = 0x8005465c;

and not this:

var x = 0x8005465c;
... bunch of stuff
this.gpr[31] = x;

Does that sound about right?

I'm working on a reduced test case and a fix now, assuming that my guess is right.

[Filip Pizlo]

Created attachment 141637 [details]
reduced test case

This ought to fail somewhere around iteration 70 in tip of tree.

[Filip Pizlo]

Created attachment 141638 [details]
the patch

This fixes the reduced version of the bug that I found.

Felix, can you check if this does the trick for you?  It may be that you've uncovered more than one bug!

[Filip Pizlo]

<rdar://problem/11442986>

[Darin Adler]

Comment on attachment 141638 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141638&action=review

Can you add a regression test for this? Normally we don’t land fixes without tests.

> Source/JavaScriptCore/ChangeLog:9
> +        static_cast<int>(d) is wrong, since JS semantics require us to use toInt32(d).

Sure would be nice to be more specific here about how close static_cast<int> was or wasn’t, such as what values it would give a wrong value for.

[Filip Pizlo]

(In reply to comment #5)
> (From update of attachment 141638 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141638&action=review
> 
> Can you add a regression test for this? Normally we don’t land fixes without tests.

Of course, I will LayoutTest-ify the attached test case.

> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        static_cast<int>(d) is wrong, since JS semantics require us to use toInt32(d).
> 
> Sure would be nice to be more specific here about how close static_cast<int> was or wasn’t, such as what values it would give a wrong value for.

Fixed the ChangeLog.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/116925

Felix, please reopen or file a new bug if this does not fix the issue.

[Félix Cloutier]

(In reply to comment #7)
> Landed in http://trac.webkit.org/changeset/116925
> 
> Felix, please reopen or file a new bug if this does not fix the issue.

I'm at work right now, so I'll check when I get home for lunch. In reply to the first message, though, the function in mock.js *is* the actual generated code: it's really assigning a constant, but the switch is really there.

Anyways, I'll tell you if it did the trick.

[Félix Cloutier]

Yup, that fixed it. Thanks!