Bug 69346

Summary: Assertion hit in JSC::DFG::SpeculativeJIT::compile on SL bots
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: fpizlo, oliver, pfeldman, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
the patch oliver: review+

Description Ryosuke Niwa 2011-10-04 10:20:01 PDT
inspector/console/console-format.html is hitting an assertion in JSC::DFG::SpeculativeJIT::compile on Snow Leopard Intel Debug (Tests).

crash log: http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r96591%20(2432)/inspector/console/console-format-crash-log.txt
Comment 1 Filip Pizlo 2011-10-04 17:20:38 PDT
Aha!  This looks like a broken assertion.  Fix forthcoming.
Comment 2 Filip Pizlo 2011-10-05 15:10:05 PDT
Full diagnosis:

DFG's OSR machinery assumes that each bytecode operation sets only one bytecode virtual register, and only does so when the operation has completed successfully.  But this is not the case for op_post_inc, which supports statements of the form "x = y++" where x and y are different variables.  This statement involves setting x to y's old value, then incrementing y (i.e. adding 1 to its old value and then setting it).

The assertion being hit in this failure was asserting that a SetLocal constitutes the end of a bytecode statement.  In op_post_inc, there are two SetLocals, with an ArithAdd in between.  Hence, it appears that OSR's assumptions, as well as the assertion, are wrong.

It turns out that the assumptions of OSR are pretty much right, but just need to be restated: in this case, x is dead prior to op_post_inc anyway.  So from OSR's standpoint, it's OK to execute the first SetLocal even if the ArithAdd or subsequent SetLocal will fail.

This what OSR is really assuming is that either a SetLocal constitutes the end of a bytecode operation, or its effect is harmless if the bytecode operation is executed again.

I don't know of an easy way to put this into an ASSERT statement, so I've removed the old ASSERT statement and am now verifying that this fixes the problem.
Comment 3 Filip Pizlo 2011-10-05 15:13:27 PDT
Created attachment 109872 [details]
the patch
Comment 4 Filip Pizlo 2011-10-05 15:59:17 PDT
Landed in r96762.