Bug 131447 – Crash beneath DFG JIT code @ video.disney.com

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

[Michael Saboff]

We die executing code where we bailed out of a DFG compile.  We call SpeculativeJIT::bail() which, likely erroneously, sets m_compileOkay to true and then we continue compiling.  The resulting code has the basic block filled with breakpoint instructions from where we find the error to end compilation through the end of the code block.  We then happily continue compiling the next basic block.  At the end of the compilation, we install the compiled code.

I believe the fix is to stop the compilation and bubble up the failure.

[Michael Saboff]

<rdar://problem/16424871>

[Michael Saboff]

Created attachment 228974 [details]
Patch

[Geoffrey Garen]

This patch needs a regression test.

[Build Bot]

Comment on attachment 228974 [details]
Patch

Attachment 228974 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6233830256017408

New failing tests:
js/dfg-activation-register-overwritten-in-throw.html
js/dfg-arguments-osr-exit-multiple-blocks.html
js/dfg-array-push-bad-time.html
js/dfg-dead-unreachable-code-with-chain-of-dead-unchecked-nodes.html
js/dfg-check-array-non-array.html
js/dfg-cse-dead-get-scoped-var.html
js/dfg-constant-fold-uncaptured-variable-that-is-later-captured.html

[Build Bot]

Created attachment 228980 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 228974 [details]
Patch

Attachment 228974 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4636233284190208

New failing tests:
js/dfg-activation-register-overwritten-in-throw.html
js/dfg-arguments-osr-exit-multiple-blocks.html
js/dfg-array-push-bad-time.html
js/dfg-dead-unreachable-code-with-chain-of-dead-unchecked-nodes.html
js/dfg-check-array-non-array.html
js/dfg-cse-dead-get-scoped-var.html
js/dfg-constant-fold-uncaptured-variable-that-is-later-captured.html

[Build Bot]

Created attachment 228990 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Michael Saboff]

(In reply to comment #6)
> (From update of attachment 228974 [details])
> Attachment 228974 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/4636233284190208
> 
> New failing tests:
> js/dfg-activation-register-overwritten-in-throw.html
> js/dfg-arguments-osr-exit-multiple-blocks.html
> js/dfg-array-push-bad-time.html
> js/dfg-dead-unreachable-code-with-chain-of-dead-unchecked-nodes.html
> js/dfg-check-array-non-array.html
> js/dfg-cse-dead-get-scoped-var.html
> js/dfg-constant-fold-uncaptured-variable-that-is-later-captured.html

Working on these tests.  They are all timing out.

[Build Bot]

Comment on attachment 228974 [details]
Patch

Attachment 228974 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6317721704726528

New failing tests:
js/dfg-activation-register-overwritten-in-throw.html
js/dfg-arguments-osr-exit-multiple-blocks.html
js/dfg-array-push-bad-time.html
js/dfg-dead-unreachable-code-with-chain-of-dead-unchecked-nodes.html
js/dfg-check-array-non-array.html
js/dfg-cse-dead-get-scoped-var.html
js/dfg-constant-fold-uncaptured-variable-that-is-later-captured.html

[Build Bot]

Created attachment 228996 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Michael Saboff]

Created attachment 229085 [details]
Updated patch

The real problem was an incorrect speculation check.

[Geoffrey Garen]

Comment on attachment 229085 [details]
Updated patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        The prior check in the 32 bit version of speculateMisc() checked that the value is
> +        either a Misc or an Int32 followed by a check that the value is a Misc.  The first
> +        check masked the second and therefore it didn't get performed.  The fix is to change
> +        the first check to not be an Int32.

Rather than saying "the check was" I would say "the recorded type was". The type checks were correct, and this patch doesn't change them. What was incorrect was the filtered type we recorded in the abstract interpreter.

> Source/JavaScriptCore/tests/stress/test-spec-misc.js:16
> +    x * 2;

Is this relevant? It looks like dead code.

[Michael Saboff]

(In reply to comment #12)
> (From update of attachment 229085 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229085&action=review
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        The prior check in the 32 bit version of speculateMisc() checked that the value is
> > +        either a Misc or an Int32 followed by a check that the value is a Misc.  The first
> > +        check masked the second and therefore it didn't get performed.  The fix is to change
> > +        the first check to not be an Int32.
> 
> Rather than saying "the check was" I would say "the recorded type was". The type checks were correct, and this patch doesn't change them. What was incorrect was the filtered type we recorded in the abstract interpreter.

I actually did change runtime checking in speculateMisc() which was bogus.  The abstract interpreter was fine.  How does this sound for a ChangeLog entry:

The 32 bit path of speculateMisc() was incorrect.  It first emitted a checked for the value being either a Misc or an Int32, then it would check that the value was a Misc.  The first check covered the second and therefore the code for the second check wasn't emitted.  The missing second check would have correctly OSR exited.  Changed the first check to ensure that the value isn't an Int32.

> > Source/JavaScriptCore/tests/stress/test-spec-misc.js:16
> > +    x * 2;
> 
> Is this relevant? It looks like dead code.

The x + 2 is so we use x AND we speculate that it is an Int32.  I can add a comment if you like.

[Filip Pizlo]

Comment on attachment 229085 [details]
Updated patch

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

>>> Source/JavaScriptCore/ChangeLog:11
>>> +        the first check to not be an Int32.
>> 
>> Rather than saying "the check was" I would say "the recorded type was". The type checks were correct, and this patch doesn't change them. What was incorrect was the filtered type we recorded in the abstract interpreter.
> 
> I actually did change runtime checking in speculateMisc() which was bogus.  The abstract interpreter was fine.  How does this sound for a ChangeLog entry:
> 
> The 32 bit path of speculateMisc() was incorrect.  It first emitted a checked for the value being either a Misc or an Int32, then it would check that the value was a Misc.  The first check covered the second and therefore the code for the second check wasn't emitted.  The missing second check would have correctly OSR exited.  Changed the first check to ensure that the value isn't an Int32.

I don't think that either forms of the text accurately reflect what was going on.  I would say: "The 32-bit path of speculateMisc() uses an 'is not int32' check followed by 'tag not less than Undefined' check.  The first check was incorrectly elided if we knew that the value *was* [sic] an int32, when it should have been elided if we already knew that the value *was not* an int32."

>>> Source/JavaScriptCore/tests/stress/test-spec-misc.js:16
>>> +    x * 2;
>> 
>> Is this relevant? It looks like dead code.
> 
> The x + 2 is so we use x AND we speculate that it is an Int32.  I can add a comment if you like.

Type checks are never dead.  x * 2 forces an is-int check.

[Filip Pizlo]

r=me too.  I don't care too much about the changelog text, but let's make it clear that the thing that changed was making sure that we tweaked the rules for when the first check is elided, rather than changing anything about the code for the first check.

[Geoffrey Garen]

> The 32 bit path of speculateMisc() was incorrect.  It first emitted a checked for the value being either a Misc or an Int32

Did it? I see this:

m_jit.branch32(MacroAssembler::Equal, regs.tagGPR(), MacroAssembler::TrustedImm32(JSValue::Int32Tag)));

That's a check for "not int32". It is not a check for "either a Misc or an int32". After this check executes, the value can still be double, undefined, cell, or any subtype thereof -- none of which are "either a misc or an int32".

Perhaps when you say "either a misc or int32" you are referring to the "typesPassedThrough" value passed to the abstract interpreter. That value is not a type check -- it is a description of the checks you promise you have already done.

> , then it would check that the value was a Misc.  

Here's the check I see:

m_jit.branch32(MacroAssembler::Below, regs.tagGPR(), MacroAssembler::TrustedImm32(JSValue::UndefinedTag)));

Once again, I think you are referring to the "typesPassedThrough" value, and not the type check performed (which is actually a check for "not cell").

[Filip Pizlo]

(In reply to comment #16)
> > The 32 bit path of speculateMisc() was incorrect.  It first emitted a checked for the value being either a Misc or an Int32
> 
> Did it? I see this:
> 
> m_jit.branch32(MacroAssembler::Equal, regs.tagGPR(), MacroAssembler::TrustedImm32(JSValue::Int32Tag)));
> 
> That's a check for "not int32". It is not a check for "either a Misc or an int32". After this check executes, the value can still be double, undefined, cell, or any subtype thereof -- none of which are "either a misc or an int32".
> 
> Perhaps when you say "either a misc or int32" you are referring to the "typesPassedThrough" value passed to the abstract interpreter. That value is not a type check -- it is a description of the checks you promise you have already done.
> 
> > , then it would check that the value was a Misc.  
> 
> Here's the check I see:
> 
> m_jit.branch32(MacroAssembler::Below, regs.tagGPR(), MacroAssembler::TrustedImm32(JSValue::UndefinedTag)));
> 
> Once again, I think you are referring to the "typesPassedThrough" value, and not the type check performed (which is actually a check for "not cell").

Yeah, what Geoff said.

[Michael Saboff]

(In reply to comment #17)
> (In reply to comment #16)
> > > The 32 bit path of speculateMisc() was incorrect.  It first emitted a checked for the value being either a Misc or an Int32
> > 
> > Did it? I see this:
> > 
> > m_jit.branch32(MacroAssembler::Equal, regs.tagGPR(), MacroAssembler::TrustedImm32(JSValue::Int32Tag)));
> > 
> > That's a check for "not int32". It is not a check for "either a Misc or an int32". After this check executes, the value can still be double, undefined, cell, or any subtype thereof -- none of which are "either a misc or an int32".
> > 
> > Perhaps when you say "either a misc or int32" you are referring to the "typesPassedThrough" value passed to the abstract interpreter. That value is not a type check -- it is a description of the checks you promise you have already done.
> > 
> > > , then it would check that the value was a Misc.  
> > 
> > Here's the check I see:
> > 
> > m_jit.branch32(MacroAssembler::Below, regs.tagGPR(), MacroAssembler::TrustedImm32(JSValue::UndefinedTag)));
> > 
> > Once again, I think you are referring to the "typesPassedThrough" value, and not the type check performed (which is actually a check for "not cell").
> 
> Yeah, what Geoff said.

I'll land with the ChangeLog text that Phil suggested.

[Michael Saboff]

Committed r167112: <http://trac.webkit.org/changeset/167112>