Processing of many of the ArithXXX and CompareXXX nodes in the abstract interpreter don't take into account UntypedUse leading to bad values.
<rdar://problem/36186134>
Created attachment 331799 [details] Patch
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.
Created attachment 331801 [details] Rebased Patch
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 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?
(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.
(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 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
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
(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.
(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 ++.
Committed r227341: <https://trac.webkit.org/changeset/227341>
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.