Bug 181886 - DFG abstract interpreter needs to properly model effects of some Math ops
Summary: DFG abstract interpreter needs to properly model effects of some Math ops
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-19 15:24 PST by Michael Saboff
Modified: 2018-04-10 10:20 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.46 KB, patch)
2018-01-19 15:42 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Rebased Patch (11.50 KB, patch)
2018-01-19 15:49 PST, Michael Saboff
sbarati: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-sierra (3.03 MB, application/zip)
2018-01-19 17:00 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2018-01-19 15:24:20 PST
Processing of many of the ArithXXX and CompareXXX nodes in the abstract interpreter don't take into account UntypedUse leading to bad values.
Comment 1 Michael Saboff 2018-01-19 15:26:12 PST
<rdar://problem/36186134>
Comment 2 Michael Saboff 2018-01-19 15:42:46 PST
Created attachment 331799 [details]
Patch
Comment 3 EWS Watchlist 2018-01-19 15:44:40 PST
Attachment 331799 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1070:  More than one command on the same line  [whitespace/newline] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Michael Saboff 2018-01-19 15:49:34 PST
Created attachment 331801 [details]
Rebased Patch
Comment 5 EWS Watchlist 2018-01-19 15:51:22 PST
Attachment 331801 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1070:  More than one command on the same line  [whitespace/newline] [4]
WARNING: This machine could support 4 simulators, but is only configured for 3.
WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Saam Barati 2018-01-19 15:52:18 PST
Comment on attachment 331801 [details]
Rebased Patch

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

r=me

> JSTests/stress/arith-nodes-abstract-interpreter-untypeduse.js:75
> +test(">=", function(a, b, c){

Should we just do all binary math ops in this test even if some were correct?
Comment 7 Michael Saboff 2018-01-19 15:53:45 PST
(In reply to Saam Barati from comment #6)
> Comment on attachment 331801 [details]
> Rebased Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331801&action=review
> 
> r=me
> 
> > JSTests/stress/arith-nodes-abstract-interpreter-untypeduse.js:75
> > +test(">=", function(a, b, c){
> 
> Should we just do all binary math ops in this test even if some were correct?

Yep.  It is a regression test.
Comment 8 Saam Barati 2018-01-19 16:10:55 PST
(In reply to Michael Saboff from comment #7)
> (In reply to Saam Barati from comment #6)
> > Comment on attachment 331801 [details]
> > Rebased Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=331801&action=review
> > 
> > r=me
> > 
> > > JSTests/stress/arith-nodes-abstract-interpreter-untypeduse.js:75
> > > +test(">=", function(a, b, c){
> > 
> > Should we just do all binary math ops in this test even if some were correct?
> 
> Yep.  It is a regression test.

Sorry, my comment wasn't clear at all. I agree! I meant to propose just add all of them, +, -, *, **, /, etc
Comment 9 EWS Watchlist 2018-01-19 17:00:19 PST
Comment on attachment 331801 [details]
Rebased Patch

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

New failing tests:
fast/block/positioning/fixed-container-with-relative-parent.html
Comment 10 EWS Watchlist 2018-01-19 17:00:20 PST
Created attachment 331820 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 11 Michael Saboff 2018-01-19 21:03:01 PST
(In reply to Saam Barati from comment #8)
> (In reply to Michael Saboff from comment #7)
> > (In reply to Saam Barati from comment #6)
> > > Comment on attachment 331801 [details]
> > > Rebased Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=331801&action=review
> > > 
> > > r=me
> > > 
> > > > JSTests/stress/arith-nodes-abstract-interpreter-untypeduse.js:75
> > > > +test(">=", function(a, b, c){
> > > 
> > > Should we just do all binary math ops in this test even if some were correct?
> > 
> > Yep.  It is a regression test.
> 
> Sorry, my comment wasn't clear at all. I agree! I meant to propose just add
> all of them, +, -, *, **, /, etc

I'll do that locally.
Comment 12 Michael Saboff 2018-01-22 10:36:54 PST
(In reply to Michael Saboff from comment #11)
> (In reply to Saam Barati from comment #8)
> > (In reply to Michael Saboff from comment #7)
> > > (In reply to Saam Barati from comment #6)
> > > > Comment on attachment 331801 [details]
> > > > Rebased Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=331801&action=review
> > > > 
> > > > r=me
> > > > 
> > > > > JSTests/stress/arith-nodes-abstract-interpreter-untypeduse.js:75
> > > > > +test(">=", function(a, b, c){
> > > > 
> > > > Should we just do all binary math ops in this test even if some were correct?
> > > 
> > > Yep.  It is a regression test.
> > 
> > Sorry, my comment wasn't clear at all. I agree! I meant to propose just add
> > all of them, +, -, *, **, /, etc
> 
> I'll do that locally.

I added unary +, +, -, *, /, %, **, prefix -- and prefix ++.
Comment 13 Michael Saboff 2018-01-22 10:37:57 PST
Committed r227341: <https://trac.webkit.org/changeset/227341>
Comment 14 Filip Pizlo 2018-04-10 10:13:28 PDT
Comment on attachment 331801 [details]
Rebased Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1615
> +        if (node->child1().useKind() == UntypedUse || node->child2().useKind() == UntypedUse)
> +            clobberWorld(node->origin.semantic, clobberLimit);

This is incorrect.  CompareEq is only effectful when isBinaryUseKind(UntypedUse).  For example, CompareEq(Untyped:, Other:) is not effectful.