| Summary: | Executing known edge types may reveal a contradiction causing us to emit an exit at a node that is not allowed to exit | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | benjamin, ddkilzer, ews-watchlist, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Saam Barati
2018-04-06 15:33:12 PDT
Created attachment 337401 [details]
patch
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? 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 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.
(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. (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. 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 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? 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. Created attachment 337553 [details]
the patch
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. 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 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 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 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. Created attachment 337566 [details]
the patch
EWS should be happier now.
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.
(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. Created attachment 337570 [details]
better patch
Incorporated feedback from discussion with Saam.
Created attachment 337571 [details]
even better patch
Created attachment 337572 [details]
the patch
This or https://bugs.webkit.org/show_bug.cgi?id=184400 seems to have caused a 3% kraken regression. (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 |