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

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

[Saam Barati]

...

[Saam Barati]

<rdar://problem/37019655>

[Saam Barati]

Created attachment 337401 [details]
patch

[Saam Barati]

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?

[EWS Watchlist]

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.

[Filip Pizlo]

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.

[Saam Barati]

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

[Filip Pizlo]

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

[Filip Pizlo]

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.

[Filip Pizlo]

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?

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 337553 [details]
the patch

[Filip Pizlo]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

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.

[Filip Pizlo]

Created attachment 337566 [details]
the patch

EWS should be happier now.

[EWS Watchlist]

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.

[Filip Pizlo]

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

[Filip Pizlo]

Created attachment 337570 [details]
better patch

Incorporated feedback from discussion with Saam.

[Filip Pizlo]

Created attachment 337571 [details]
even better patch

[Filip Pizlo]

Created attachment 337572 [details]
the patch

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/230465/webkit

[Saam Barati]

This or https://bugs.webkit.org/show_bug.cgi?id=184400 seems to have caused a 3% kraken regression.

[Saam Barati]

(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