120080 2013-08-20 12:57:17 -0700 REGRESSION (r153222, 32-bit): NULL JSValue() seen when running peacekeeper benchmark 2013-09-05 06:35:26 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 chris_curtis chris_curtis abrhm bfulgham commit-queue compnerd fpizlo ggaren jbriance kadam mark.lam msaboff oliver ossy peavo zarvai oldest_to_newest 919176 0 chris_curtis 2013-08-20 12:57:17 -0700 This is a follow up error coming from https://bugs.webkit.org/show_bug.cgi?id=119812. The JSValue that is passed into errorDescriptionForValue should never be isEmpty(). 920153 1 jbriance 2013-08-23 02:42:54 -0700 After bisseting a lot, I think this empty JSValue in errorDescriptionForValue() regression appeared in r153222 (in the middle of the FTL). The test page I use to stimulate the issue: http://peacekeeper.futuremark.com/ (no need to launch the benchmark, just loading the front page). My environment: sh4 32-bit with LLINT and baseline JIT, without DFG JIT, using Qt port. The different tests I made: - with r153221: no problem, page is loaded correctly and errorDescriptionForValue() is never called with empty JSValue() - with r153222: errorDescriptionForValue() IS called with empty JSValue() - with r153222 with JIT disabled (through JSC_useJIT=false): no problem, page is loaded correctly and errorDescriptionForValue() is never called with empty JSValue() If this could help, with latest revision (r154475) with the same environment + DFG JIT enabled, I get this: - with r154475: errorDescriptionForValue() IS called with empty JSValue() - with r154475 with DFG JIT disabled (through JSC_useDFGJIT=false): errorDescriptionForValue() IS called with empty JSValue() - with r154475 with JIT disabled (through JSC_useJIT=false): no problem, page is loaded correctly Oliver, could you please take a quick look to your modifications related to 32-bit JIT made in r153222 that could lead to this ? Thanks, 920308 2 ggaren 2013-08-23 11:18:15 -0700 <rdar://problem/14820996> 921183 3 jbriance 2013-08-26 15:08:03 -0700 I tested with a configuration which is more common: X86 32-bit with LLINT, baseline JIT and DFG_JIT enabled, using Qt port. I get the same results with http://peacekeeper.futuremark.com/ on r154475: - with r154475: errorDescriptionForValue() IS called with empty JSValue() - with r154475 with DFG JIT disabled (through JSC_useDFGJIT=false): errorDescriptionForValue() IS called with empty JSValue() - with r154475 with JIT disabled (through JSC_useJIT=false): no problem, page is loaded correctly 921714 4 ap 2013-08-27 15:51:34 -0700 See also: bug 119395. 921886 5 209859 jbriance 2013-08-28 02:25:35 -0700 Created attachment 209859 Revert changes introduced by r153222 in emitSlow_op_get_argument_by_val for 32-bit I didn't get this NULL JSValue when reverting the change introduced by r153222 in emitSlow_op_get_argument_by_val for 32-bit (JITOpcodes32_64.cpp). It doesn't create regressions for me (sh4 32-bit, Qt port, LLINT+baseline JIT+DFG) with: - Tools/Scripts/run-javascriptcore-tests - Tools/Scripts/run-fast-jsc - SunSpider 1.0 - v8-v4 test suite 922025 6 209859 oliver 2013-08-28 08:50:26 -0700 Comment on attachment 209859 Revert changes introduced by r153222 in emitSlow_op_get_argument_by_val for 32-bit Weird, this looks fine as the 64bit path doesn't use the slow path machinery, but i'm curious as the LLINT does, and we use the SlowPathCall type for numerous other sites. I wonder if JITSlowPathCall::call isn't getting the windows calling convention right? 922027 7 jbriance 2013-08-28 08:56:52 -0700 (In reply to comment #6) > (From update of attachment 209859 [details]) > Weird, this looks fine as the 64bit path doesn't use the slow path machinery, but i'm curious as the LLINT does, and we use the SlowPathCall type for numerous other sites. I wonder if JITSlowPathCall::call isn't getting the windows calling convention right? To be honest, this is an empirical fix, based on the diff between r153221 and 153222 + diff between 32-bit and 64-bit. It turns out that this fixes the issue and doesn't create regressions for me (sh4 32-bit Linux, Qt, LLINT+baseline JIT+DFG), but I won't be able to explain why it fixes it yet :/ 922028 8 oliver 2013-08-28 09:02:15 -0700 (In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 209859 [details] [details]) > > Weird, this looks fine as the 64bit path doesn't use the slow path machinery, but i'm curious as the LLINT does, and we use the SlowPathCall type for numerous other sites. I wonder if JITSlowPathCall::call isn't getting the windows calling convention right? > To be honest, this is an empirical fix, based on the diff between r153221 and 153222 + diff between 32-bit and 64-bit. > It turns out that this fixes the issue and doesn't create regressions for me (sh4 32-bit Linux, Qt, LLINT+baseline JIT+DFG), but I won't be able to explain why it fixes it yet :/ Okay, I _believe_ that the SlowPathCall is incorrectly reloading the call frame register from topCallFrame, should be writing the callFrameRegister to topCallFrame instead as topCallFrame is no longer guaranteed to be a reasonable call frame after we've returned to the hit - it's only valid while in host code. 922056 9 209859 darin 2013-08-28 09:42:48 -0700 Comment on attachment 209859 Revert changes introduced by r153222 in emitSlow_op_get_argument_by_val for 32-bit All WebKit bug fixes need a to come with test that shows the bug and prevents future regression. This fix looks OK but we need a test. 922623 10 209859 msaboff 2013-08-29 13:10:30 -0700 Comment on attachment 209859 Revert changes introduced by r153222 in emitSlow_op_get_argument_by_val for 32-bit After some sleuthing as to what was going on, this is the right fix. The reason is due to where the results from the create_arguments call end up. slow_path_create_arguments will put the results in the virtual register specified by currentInstruction[1].u.operand as it is intended to be the used for processing op_create_arguments. When using cgi_op_create_arguments, the result is left in regT0 and regT1, and we need to explicitly emit the stores as this patch does. Notice that we actually want the result in currentInstruction[2].u.operand instead of the operand at index 1. That is because when processing op_get_argument_by_value we only call create_arguments when the arguments virtual register contains the EmptyValueTag, effectively materializing the arguments as needed. 922625 11 209859 darin 2013-08-29 13:12:57 -0700 Comment on attachment 209859 Revert changes introduced by r153222 in emitSlow_op_get_argument_by_val for 32-bit Michael, what about a test case? 922642 12 209859 commit-queue 2013-08-29 13:34:30 -0700 Comment on attachment 209859 Revert changes introduced by r153222 in emitSlow_op_get_argument_by_val for 32-bit Clearing flags on attachment: 209859 Committed r154839: <http://trac.webkit.org/changeset/154839> 922643 13 commit-queue 2013-08-29 13:34:33 -0700 All reviewed patches have been landed. Closing bug. 922647 14 msaboff 2013-08-29 13:42:11 -0700 (In reply to comment #11) > (From update of attachment 209859 [details]) > Michael, what about a test case? I'll work on one. 922706 15 msaboff 2013-08-29 15:47:39 -0700 (In reply to comment #14) > (In reply to comment #11) > > (From update of attachment 209859 [details] [details]) > > Michael, what about a test case? > > I'll work on one. Test landed in change set r154846 <http://trac.webkit.org/changeset/154846> 924994 16 peavo 2013-09-05 06:35:26 -0700 *** Bug 119395 has been marked as a duplicate of this bug. *** 209859 2013-08-28 02:25:35 -0700 2013-08-29 13:34:30 -0700 Revert changes introduced by r153222 in emitSlow_op_get_argument_by_val for 32-bit bug-120080.patch text/plain 1478 jbriance SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTU0NzI2KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBA CisyMDEzLTA4LTI4ICBKdWxpZW4gQnJpYW5jZWF1ICA8amJyaWFuY2VAY2lzY28uY29tPgorCisg ICAgICAgIFJFR1JFU1NJT04ocjE1MzIyMiwgMzItYml0KTogTlVMTCBKU1ZhbHVlKCkgc2VlbiB3 aGVuIHJ1bm5pbmcgcGVhY2VrZWVwZXIgYmVuY2htYXJrLgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTIwMDgwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBqaXQvSklUT3Bjb2RlczMyXzY0LmNwcDoKKyAg ICAgICAgKEpTQzo6SklUOjplbWl0U2xvd19vcF9nZXRfYXJndW1lbnRfYnlfdmFsKTogUmV2ZXJ0 IGNoYW5nZXMgaW50cm9kdWNlZCBieSByMTUzMjIyIGluIHRoaXMgZnVuY3Rpb24uCisKIDIwMTMt MDgtMjMgIEFuZHkgRXN0ZXMgIDxhZXN0ZXNAYXBwbGUuY29tPgogCiAgICAgICAgIEZpeCBpc3N1 ZXMgZm91bmQgYnkgdGhlIENsYW5nIFN0YXRpYyBBbmFseXplcgpJbmRleDogU291cmNlL0phdmFT Y3JpcHRDb3JlL2ppdC9KSVRPcGNvZGVzMzJfNjQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9K YXZhU2NyaXB0Q29yZS9qaXQvSklUT3Bjb2RlczMyXzY0LmNwcAkocmV2aXNpb24gMTU0NzI0KQor KysgU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9KSVRPcGNvZGVzMzJfNjQuY3BwCSh3b3JraW5n IGNvcHkpCkBAIC0xMjYyLDggKzEyNjIsOSBAQCB2b2lkIEpJVDo6ZW1pdFNsb3dfb3BfZ2V0X2Fy Z3VtZW50X2J5X3ZhCiAgICAgbGlua1Nsb3dDYXNlKGl0ZXIpOwogICAgIGxpbmtTbG93Q2FzZShp dGVyKTsKIAotICAgIEpJVFNsb3dQYXRoQ2FsbCBzbG93UGF0aENhbGwodGhpcywgY3VycmVudElu c3RydWN0aW9uLCBzbG93X3BhdGhfY3JlYXRlX2FyZ3VtZW50cyk7Ci0gICAgc2xvd1BhdGhDYWxs LmNhbGwoKTsKKyAgICBKSVRTdHViQ2FsbCh0aGlzLCBjdGlfb3BfY3JlYXRlX2FyZ3VtZW50cyku Y2FsbCgpOworICAgIGVtaXRTdG9yZShhcmd1bWVudHMsIHJlZ1QxLCByZWdUMCk7CisgICAgZW1p dFN0b3JlKHVubW9kaWZpZWRBcmd1bWVudHNSZWdpc3Rlcihhcmd1bWVudHMpLCByZWdUMSwgcmVn VDApOwogICAgIAogICAgIHNraXBBcmd1bWVudHNDcmVhdGlvbi5saW5rKHRoaXMpOwogICAgIEpJ VFN0dWJDYWxsIHN0dWJDYWxsKHRoaXMsIGN0aV9vcF9nZXRfYnlfdmFsX2dlbmVyaWMpOwo=