Bug 179821 - Update DFGSafeToExecute to be aware that ArrayPush is now a varargs node
Summary: Update DFGSafeToExecute to be aware that ArrayPush is now a varargs node
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on: 175823
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-17 09:00 PST by Robin Morisset
Modified: 2017-11-27 08:23 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.63 KB, patch)
2017-11-17 09:04 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (2.92 MB, application/zip)
2017-11-17 10:09 PST, EWS Watchlist
no flags Details
Patch (1.60 KB, patch)
2017-11-27 02:56 PST, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2017-11-17 09:00:26 PST
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.
Comment 1 Robin Morisset 2017-11-17 09:04:55 PST
Created attachment 327179 [details]
Patch
Comment 2 Mark Lam 2017-11-17 09:47:33 PST
Comment on attachment 327179 [details]
Patch

r=me
Comment 3 EWS Watchlist 2017-11-17 10:09:53 PST
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
Comment 4 EWS Watchlist 2017-11-17 10:09:54 PST
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
Comment 5 Robin Morisset 2017-11-17 10:22:54 PST
This failure looks unlikely to be related to my patch.
I will try it again in a few hours.
Comment 6 Yusuke Suzuki 2017-11-17 16:58:07 PST
Oops, nice catch.
Comment 7 Saam Barati 2017-11-26 17:28:15 PST
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.
Comment 8 Saam Barati 2017-11-26 17:30:42 PST
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.
Comment 9 Robin Morisset 2017-11-27 02:56:48 PST
Created attachment 327626 [details]
Patch
Comment 10 Robin Morisset 2017-11-27 02:57:56 PST
r="Saam Barati"
This is exactly the same patch as previously, just resubmitted since there has been a (probably) spurious failure last time.
Comment 11 EWS Watchlist 2017-11-27 02:58:19 PST
Comment on attachment 327626 [details]
Patch

Rejecting attachment 327626 [details] from review queue.

rmorisset@apple.com 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.
Comment 12 Robin Morisset 2017-11-27 03:18:18 PST
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?
Comment 13 Saam Barati 2017-11-27 07:52:18 PST
(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+
Comment 14 WebKit Commit Bot 2017-11-27 08:22:58 PST
Comment on attachment 327626 [details]
Patch

Clearing flags on attachment: 327626

Committed r225170: <https://trac.webkit.org/changeset/225170>
Comment 15 WebKit Commit Bot 2017-11-27 08:22:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2017-11-27 08:23:27 PST
<rdar://problem/35698191>