Bug 181886

Summary: DFG abstract interpreter needs to properly model effects of some Math ops
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, fpizlo, keith_miller, mark.lam, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=184455
Attachments:
Description Flags
Patch
none
Rebased Patch
saam: review+, ews-watchlist: commit-queue-
Archive of layout-test-results from ews117 for mac-sierra none

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.