Bug 120080 – REGRESSION (r153222, 32-bit): NULL JSValue() seen when running peacekeeper benchmark

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Chris Curtis]

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

[Julien Brianceau]

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,

[Geoffrey Garen]

<rdar://problem/14820996>

[Julien Brianceau]

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

[Alexey Proskuryakov]

See also: bug 119395.

[Julien Brianceau]

Created attachment 209859 [details]
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

[Oliver Hunt]

Comment on attachment 209859 [details]
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?

[Julien Brianceau]

(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 :/

[Oliver Hunt]

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

[Darin Adler]

Comment on attachment 209859 [details]
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.

[Michael Saboff]

Comment on attachment 209859 [details]
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.

[Darin Adler]

Comment on attachment 209859 [details]
Revert changes introduced by r153222 in emitSlow_op_get_argument_by_val for 32-bit

Michael, what about a test case?

[WebKit Commit Bot]

Comment on attachment 209859 [details]
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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Saboff]

(In reply to comment #11)
> (From update of attachment 209859 [details])
> Michael, what about a test case?

I'll work on one.

[Michael Saboff]

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

*** Bug 119395 has been marked as a duplicate of this bug. ***