Bug 131447 - Crash beneath DFG JIT code @ video.disney.com
Summary: Crash beneath DFG JIT code @ video.disney.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-09 12:25 PDT by Michael Saboff
Modified: 2014-04-10 22:19 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.66 KB, patch)
2014-04-09 12:31 PDT, Michael Saboff
oliver: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (489.49 KB, application/zip)
2014-04-09 14:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (488.04 KB, application/zip)
2014-04-09 15:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (454.95 KB, application/zip)
2014-04-09 15:58 PDT, Build Bot
no flags Details
Updated patch (2.76 KB, patch)
2014-04-10 16:11 PDT, Michael Saboff
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2014-04-09 12:25:20 PDT
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.
Comment 1 Michael Saboff 2014-04-09 12:31:26 PDT
<rdar://problem/16424871>
Comment 2 Michael Saboff 2014-04-09 12:31:49 PDT
Created attachment 228974 [details]
Patch
Comment 3 Geoffrey Garen 2014-04-09 14:05:57 PDT
This patch needs a regression test.
Comment 4 Build Bot 2014-04-09 14:08:21 PDT
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
Comment 5 Build Bot 2014-04-09 14:08:24 PDT
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
Comment 6 Build Bot 2014-04-09 15:03:14 PDT
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
Comment 7 Build Bot 2014-04-09 15:03:20 PDT
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
Comment 8 Michael Saboff 2014-04-09 15:13:56 PDT
(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.
Comment 9 Build Bot 2014-04-09 15:58:18 PDT
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
Comment 10 Build Bot 2014-04-09 15:58:20 PDT
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
Comment 11 Michael Saboff 2014-04-10 16:11:35 PDT
Created attachment 229085 [details]
Updated patch

The real problem was an incorrect speculation check.
Comment 12 Geoffrey Garen 2014-04-10 19:17:54 PDT
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.
Comment 13 Michael Saboff 2014-04-10 19:55:24 PDT
(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.
Comment 14 Filip Pizlo 2014-04-10 20:10:37 PDT
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.
Comment 15 Filip Pizlo 2014-04-10 20:11:35 PDT
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.
Comment 16 Geoffrey Garen 2014-04-10 20:16:25 PDT
> 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").
Comment 17 Filip Pizlo 2014-04-10 20:24:56 PDT
(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.
Comment 18 Michael Saboff 2014-04-10 21:39:36 PDT
(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.
Comment 19 Michael Saboff 2014-04-10 22:19:40 PDT
Committed r167112: <http://trac.webkit.org/changeset/167112>