Bug 181886 – DFG abstract interpreter needs to properly model effects of some Math ops

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

[Michael Saboff]

Processing of many of the ArithXXX and CompareXXX nodes in the abstract interpreter don't take into account UntypedUse leading to bad values.

[Michael Saboff]

<rdar://problem/36186134>

[Michael Saboff]

Created attachment 331799 [details]
Patch

[EWS Watchlist]

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.

[Michael Saboff]

Created attachment 331801 [details]
Rebased Patch

[EWS Watchlist]

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.

[Saam Barati]

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?

[Michael Saboff]

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

[Saam Barati]

(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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Michael Saboff]

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

[Michael Saboff]

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

[Michael Saboff]

Committed r227341: <https://trac.webkit.org/changeset/227341>

[Filip Pizlo]

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.