86330 2012-05-13 20:48:35 -0700 DFG performs incorrect constant folding on double-to-uint32 conversion in Uint32Array PutByVal 2012-05-14 09:54:41 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Mac (Intel) OS X 10.7 RESOLVED FIXED http://felixcloutier.com/documents/jsx-webkit/mock.xhtml InRadar P2 Normal --- 0 felixcca webkit-unassigned fpizlo oliver oldest_to_newest 622623 0 felixcca 2012-05-13 20:48:35 -0700 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. 622646 1 fpizlo 2012-05-13 22:13:57 -0700 (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. 622648 2 141637 fpizlo 2012-05-13 22:21:26 -0700 Created attachment 141637 reduced test case This ought to fail somewhere around iteration 70 in tip of tree. 622652 3 141638 fpizlo 2012-05-13 22:24:46 -0700 Created attachment 141638 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! 622656 4 fpizlo 2012-05-13 22:29:46 -0700 <rdar://problem/11442986> 622669 5 141638 darin 2012-05-13 22:49:16 -0700 Comment on attachment 141638 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. 622670 6 fpizlo 2012-05-13 22:55:45 -0700 (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. 622701 7 fpizlo 2012-05-13 23:47:47 -0700 Landed in http://trac.webkit.org/changeset/116925 Felix, please reopen or file a new bug if this does not fix the issue. 622998 8 felixcca 2012-05-14 08:20:24 -0700 (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. 623051 9 felixcca 2012-05-14 09:54:41 -0700 Yup, that fixed it. Thanks! 141637 2012-05-13 22:21:26 -0700 2012-05-13 22:21:26 -0700 reduced test case reduced_case.js text/plain 202 fpizlo ZnVuY3Rpb24gZm9vKGEpIHsKICAgIGFbMF0gPSAweDgwMDU0NjVjOwp9Cgp2YXIgYXJyYXkgPSBu ZXcgVWludDMyQXJyYXkoMSk7Cgpmb3IgKHZhciBpID0gMDsgaSA8IDEwMDA7ICsraSkgewogICAg Zm9vKGFycmF5KTsKICAgIGlmIChhcnJheVswXSAhPSAweDgwMDU0NjVjKQogICAgICAgIHRocm93 ICJFcnJvciBvbiBpdGVyYXRpb24gIiArIGk7Cn0KCg== 141638 2012-05-13 22:24:46 -0700 2012-05-13 22:49:16 -0700 the patch fix_uint32array_store_patch_1.diff text/plain 1437 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTE2OTE1KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBA CisyMDEyLTA1LTEzICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg REZHIHBlcmZvcm1zIGluY29ycmVjdCBjb25zdGFudCBmb2xkaW5nIG9uIGRvdWJsZS10by11aW50 MzIgY29udmVyc2lvbiBpbgorICAgICAgICBVaW50MzJBcnJheSBQdXRCeVZhbAorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODYzMzAKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAgICAgCisgICAgICAgIHN0YXRpY19jYXN0 PGludD4oZCkgaXMgd3JvbmcsIHNpbmNlIEpTIHNlbWFudGljcyByZXF1aXJlIHVzIHRvIHVzZSB0 b0ludDMyKGQpLgorCisgICAgICAgICogZGZnL0RGR1NwZWN1bGF0aXZlSklULmNwcDoKKyAgICAg ICAgKEpTQzo6REZHOjpTcGVjdWxhdGl2ZUpJVDo6Y29tcGlsZVB1dEJ5VmFsRm9ySW50VHlwZWRB cnJheSk6CisKIDIwMTItMDUtMTEgIEdhdmluIEJhcnJhY2xvdWdoICA8YmFycmFjbG91Z2hAYXBw bGUuY29tPgogCiAgICAgICAgIEludHJvZHVjZSBQcm9wZXJ0eU5hbWUgY2xhc3MKSW5kZXg6IFNv dXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHU3BlY3VsYXRpdmVKSVQuY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHU3BlY3VsYXRpdmVKSVQuY3BwCShyZXZp c2lvbiAxMTYwMzMpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR1NwZWN1bGF0aXZl SklULmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTk3NCw3ICsxOTc0LDcgQEAgdm9pZCBTcGVjdWxh dGl2ZUpJVDo6Y29tcGlsZVB1dEJ5VmFsRm9ySQogICAgICAgICB9CiAgICAgICAgIEdQUlRlbXBv cmFyeSBzY3JhdGNoKHRoaXMpOwogICAgICAgICBHUFJSZWcgc2NyYXRjaFJlZyA9IHNjcmF0Y2gu Z3ByKCk7Ci0gICAgICAgIG1faml0Lm1vdmUoSW1tMzIoc3RhdGljX2Nhc3Q8aW50PihkKSksIHNj cmF0Y2hSZWcpOworICAgICAgICBtX2ppdC5tb3ZlKEltbTMyKHRvSW50MzIoZCkpLCBzY3JhdGNo UmVnKTsKICAgICAgICAgdmFsdWUuYWRvcHQoc2NyYXRjaCk7CiAgICAgICAgIHZhbHVlR1BSID0g c2NyYXRjaFJlZzsKICAgICB9IGVsc2UgaWYgKGF0KHZhbHVlVXNlKS5zaG91bGRTcGVjdWxhdGVJ bnRlZ2VyKCkpIHsK