Bug 179821 – Update DFGSafeToExecute to be aware that ArrayPush is now a varargs node

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

[Robin Morisset]

https://bugs.webkit.org/show_bug.cgi?id=175823 made the ArrayPush node a varargs kind of node.
This occasionally causes an assertion failure when running with verboseCFA=1 because of a call to node->child1() in DFGSafeToExecute.h for ArrayPush.
I propose a simple fix, using "graph.varArgsChild(node, 1)" instead.

[Robin Morisset]

Created attachment 327179 [details]
Patch

[Mark Lam]

Comment on attachment 327179 [details]
Patch

r=me

[EWS Watchlist]

Comment on attachment 327179 [details]
Patch

Attachment 327179 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5276250

New failing tests:
http/tests/appcache/offline-access.html

[EWS Watchlist]

Created attachment 327192 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Robin Morisset]

This failure looks unlikely to be related to my patch.
I will try it again in a few hours.

[Yusuke Suzuki]

Oops, nice catch.

[Saam Barati]

Comment on attachment 327179 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSafeToExecute.h:474
> +    case ArrayPush:
> +        return node->arrayMode().alreadyChecked(graph, node, state.forNode(graph.varArgChild(node, 1)));

varArgChild(node, 1) is not equivalent to child1(). varArgChild(node, 0) is equivalent to child1() (ah, DFG naming).
You should make sure this is doing what you expect.

[Saam Barati]

Comment on attachment 327179 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGSafeToExecute.h:474
>> +        return node->arrayMode().alreadyChecked(graph, node, state.forNode(graph.varArgChild(node, 1)));
> 
> varArgChild(node, 1) is not equivalent to child1(). varArgChild(node, 0) is equivalent to child1() (ah, DFG naming).
> You should make sure this is doing what you expect.

Ignore me. This looks correct since the second varargs child is the array! Sorry for the noise.

[Robin Morisset]

Created attachment 327626 [details]
Patch

[Robin Morisset]

r="Saam Barati"
This is exactly the same patch as previously, just resubmitted since there has been a (probably) spurious failure last time.

[EWS Watchlist]

Comment on attachment 327626 [details]
Patch

Rejecting attachment 327626 [details] from review queue.

[e-mail] does not have reviewer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Robin Morisset]

It looks like the resubmitting made the system forget about the reviews, and it won't let me mark it as reviewed myself. Can someone do it for me, please?

[Saam Barati]

(In reply to Robin Morisset from comment #12)
> It looks like the resubmitting made the system forget about the reviews, and
> it won't let me mark it as reviewed myself. Can someone do it for me, please?

What you can do in this situation is fill out the Reviewed by in the changelog yourself and just cq+

[WebKit Commit Bot]

Comment on attachment 327626 [details]
Patch

Clearing flags on attachment: 327626

Committed r225170: <https://trac.webkit.org/changeset/225170>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35698191>