Bug 184372 - Executing known edge types may reveal a contradiction causing us to emit an exit at a node that is not allowed to exit
Summary: Executing known edge types may reveal a contradiction causing us to emit an e...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-06 15:33 PDT by Saam Barati
Modified: 2018-04-10 18:41 PDT (History)
14 users (show)

See Also:


Attachments
patch (5.22 KB, patch)
2018-04-06 16:16 PDT, Saam Barati
fpizlo: review-
Details | Formatted Diff | Diff
the patch (19.45 KB, patch)
2018-04-09 15:27 PDT, Filip Pizlo
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-sierra (3.16 MB, application/zip)
2018-04-09 17:53 PDT, EWS Watchlist
no flags Details
the patch (19.25 KB, patch)
2018-04-09 17:57 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
better patch (7.95 KB, patch)
2018-04-09 18:36 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
even better patch (7.86 KB, patch)
2018-04-09 18:39 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (7.86 KB, patch)
2018-04-09 18:39 PDT, Filip Pizlo
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2018-04-06 15:33:12 PDT
...
Comment 1 Saam Barati 2018-04-06 15:42:13 PDT
<rdar://problem/37019655>
Comment 2 Saam Barati 2018-04-06 16:16:59 PDT
Created attachment 337401 [details]
patch
Comment 3 Saam Barati 2018-04-06 16:18:02 PDT
Comment on attachment 337401 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:36
> +        This IR is really hard to generate. I haven't been able to make a test
> +        for this. But I have been able to reproduce it on a website. I think
> +        it's really rare that we generate this IR because our profiling information
> +        is usually really solid and consistent.

It might be worth trying to reproduce this by creating an intrinsic that turns into a check in FixupPhase. That'd allow us to generate a ton of non-profiling lead checks. What do folks think?
Comment 4 EWS Watchlist 2018-04-06 16:19:51 PDT
Attachment 337401 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Filip Pizlo 2018-04-06 20:42:11 PDT
Comment on attachment 337401 [details]
patch

Isn’t the bug here that the SetLocal would try to emit a check for a known edge?

It feels shady that you’re handling only the case of AI state becoming invalid. There’s a sauntle version of this bug where AI state isn’t invalid: like a cell check that wasn’t visible to AI. 

Therefore the correct fix is to make lowCell not emit a check for KnownCell. Similarly other edge uses should not emit checks for known edges, regardless of AI state. 

A good snort check here is that we want compilation to succeed even if AI thinks things are top. We have a lot of quirks like this that prevent that right now but we should fix them.
Comment 6 Saam Barati 2018-04-07 10:10:49 PDT
(In reply to Filip Pizlo from comment #5)
> Comment on attachment 337401 [details]
> patch
> 
> Isn’t the bug here that the SetLocal would try to emit a check for a known
> edge?
> 
> It feels shady that you’re handling only the case of AI state becoming
> invalid. There’s a sauntle version of this bug where AI state isn’t invalid:
> like a cell check that wasn’t visible to AI. 
> 
> Therefore the correct fix is to make lowCell not emit a check for KnownCell.
> Similarly other edge uses should not emit checks for known edges, regardless
> of AI state. 

That’s not what’s happening here. lowCell sees that the type is None (it checks AbstractValue::isClear()), and emits an exit.

> 
> A good snort check here is that we want compilation to succeed even if AI
> thinks things are top. We have a lot of quirks like this that prevent that
> right now but we should fix them.
Comment 7 Filip Pizlo 2018-04-07 10:37:36 PDT
(In reply to Saam Barati from comment #6)
> (In reply to Filip Pizlo from comment #5)
> > Comment on attachment 337401 [details]
> > patch
> > 
> > Isn’t the bug here that the SetLocal would try to emit a check for a known
> > edge?
> > 
> > It feels shady that you’re handling only the case of AI state becoming
> > invalid. There’s a sauntle version of this bug where AI state isn’t invalid:
> > like a cell check that wasn’t visible to AI. 
> > 
> > Therefore the correct fix is to make lowCell not emit a check for KnownCell.
> > Similarly other edge uses should not emit checks for known edges, regardless
> > of AI state. 
> 
> That’s not what’s happening here. lowCell sees that the type is None (it
> checks AbstractValue::isClear()), and emits an exit.

Right!  It emits an exit because it thinks that it’s job was to do a type check. The exit is just the result of folding the check to always fail. 

I think lowCell should just return a bottom value in this case. 

> 
> > 
> > A good snort check here is that we want compilation to succeed even if AI
> > thinks things are top. We have a lot of quirks like this that prevent that
> > right now but we should fix them.
Comment 8 Filip Pizlo 2018-04-07 10:51:50 PDT
The more I think about it, the more I like this framework for thinking about it:

- force exit only makes sense if it’s the result of folding a check. 

- therefore, lowCell is violating its “no check” contract with the known edge by having a force exit.
Comment 9 Filip Pizlo 2018-04-07 10:53:08 PDT
Comment on attachment 337401 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:28
> +        crash because @e is not allowed to exit.

At some point I thought we removed all “can exit” assertions in the compiler. 

Which assertion is crashing?
Comment 10 Filip Pizlo 2018-04-09 10:46:53 PDT
It's interesting that the DFG and FTL are *almost* doing the right thing, but not quite.  They already return a bottom value (DFG returns a dummy just-allocated register, FTL returns zero or something similar) in this case.  The real problem is that the fillSpeculate methods in the DFG and to some extent the lowBlah methods in FTL do not honor Known edge kinds.  This isn't simply a matter of reaching bottom AI state.  It's deeper than that: for example you could have a string flow into KnownInt without AI being isClear().  This can happen in cases where the frontend generated KnownInt because it *knew* that the code can only execute when the input was int, but AI wasn't smart enough to realize that it *should* have been bottom.  AI can be imprecise, so anytime the truth is that it should be bottom, it may approximate bottom with any type, like SpecString.

Therefore, all of the fillSpeculateBlah and lowBlah must honor mayHaveTypeCheck(edge.useKind()).  If that is false, they must not emit checks.  This means:

1) Not doing terminateSpeculativeExecution if they observe any kind of contradiction.

2) Not doing speculationCheck if the type they wanted wasn't proven.
Comment 11 Filip Pizlo 2018-04-09 15:27:07 PDT
Created attachment 337553 [details]
the patch
Comment 12 Filip Pizlo 2018-04-09 15:28:00 PDT
Comment on attachment 337553 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreter.h:2
> - * Copyright (C) 2013-2016 Apple Inc. All rights reserved.
> + * Copyright (C) 2013-2018 Apple Inc. All rights reserved.

I will revert.
Comment 13 EWS Watchlist 2018-04-09 15:29:02 PDT
Attachment 337553 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2645:  _dtc_source is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2646:  _dtc_edge is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2647:  _dtc_typesPassedThrough is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:131:  _ftc_lowValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:132:  _ftc_highValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:133:  _ftc_typesPassedThrough is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 EWS Watchlist 2018-04-09 17:53:43 PDT
Comment on attachment 337553 [details]
the patch

Attachment 337553 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7259840

New failing tests:
inspector/canvas/recording-2d.html
inspector/canvas/recording-webgl-snapshots.html
inspector/canvas/recording-webgl.html
Comment 15 EWS Watchlist 2018-04-09 17:53:45 PDT
Created attachment 337564 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 16 Saam Barati 2018-04-09 17:56:27 PDT
Comment on attachment 337553 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGUseKind.h:101
> +    case KnownInt32Use:
> +    case KnownBooleanUse:
> +    case KnownStringUse:
> +    case KnownCellUse:
> +    case KnownPrimitiveUse:

Do we really want to make this change? It seems like we can use these KnownBlah to remove type checks, for example:

a: Foo()
b: Bar(KnownCell:@a)
c: Baz(Cell:@a)

Let's say that Foo's type according to AI is BytecodeTop. We'll end up emitting a cell check in Baz.
Comment 17 Filip Pizlo 2018-04-09 17:57:17 PDT
Created attachment 337566 [details]
the patch

EWS should be happier now.
Comment 18 EWS Watchlist 2018-04-09 17:58:43 PDT
Attachment 337566 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2645:  _dtc_source is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2646:  _dtc_edge is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2647:  _dtc_typesPassedThrough is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:131:  _ftc_lowValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:132:  _ftc_highValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:133:  _ftc_typesPassedThrough is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Filip Pizlo 2018-04-09 18:00:17 PDT
(In reply to Saam Barati from comment #16)
> Comment on attachment 337553 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337553&action=review
> 
> > Source/JavaScriptCore/dfg/DFGUseKind.h:101
> > +    case KnownInt32Use:
> > +    case KnownBooleanUse:
> > +    case KnownStringUse:
> > +    case KnownCellUse:
> > +    case KnownPrimitiveUse:
> 
> Do we really want to make this change? It seems like we can use these
> KnownBlah to remove type checks, for example:
> 
> a: Foo()
> b: Bar(KnownCell:@a)
> c: Baz(Cell:@a)
> 
> Let's say that Foo's type according to AI is BytecodeTop. We'll end up
> emitting a cell check in Baz.

I made this change because it has the effect of removing a bunch of cases of bad KnownBlah assertions or checks.

For example, if I didn't make this change, then I'd have to add special cases to edge verification.  Maybe other places, too.
Comment 20 Filip Pizlo 2018-04-09 18:36:13 PDT
Created attachment 337570 [details]
better patch

Incorporated feedback from discussion with Saam.
Comment 21 Filip Pizlo 2018-04-09 18:39:07 PDT
Created attachment 337571 [details]
even better patch
Comment 22 Filip Pizlo 2018-04-09 18:39:55 PDT
Created attachment 337572 [details]
the patch
Comment 23 Filip Pizlo 2018-04-09 19:42:59 PDT
Landed in https://trac.webkit.org/changeset/230465/webkit
Comment 24 Saam Barati 2018-04-10 18:35:45 PDT
This or https://bugs.webkit.org/show_bug.cgi?id=184400 seems to have caused a 3% kraken regression.
Comment 25 Saam Barati 2018-04-10 18:41:47 PDT
(In reply to Saam Barati from comment #24)
> This or https://bugs.webkit.org/show_bug.cgi?id=184400 seems to have caused
> a 3% kraken regression.

Ignore me. Surprisingly, it seems it's actually this: https://bugs.webkit.org/show_bug.cgi?id=184322