Bug 187827 - DFG AbstractInterpreter: CheckArray filters array modes for DirectArguments only using NonArray
Summary: DFG AbstractInterpreter: CheckArray filters array modes for DirectArguments o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-19 16:05 PDT by Michael Saboff
Modified: 2018-07-20 16:48 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.48 KB, patch)
2018-07-20 11:26 PDT, Michael Saboff
saam: review+
Details | Formatted Diff | Diff
Updated patch that includes ScopedArguments change (5.21 KB, patch)
2018-07-20 14:17 PDT, Michael Saboff
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2018-07-19 16:05:10 PDT
In the Abstract Interpreter, when we process ArrayCheck nodes, for DirectArgument array types we filter array modes using the mask of 1 (NonIndexingShape).  DirectArguments has a ArrayStorageShape indexing type.
Comment 1 Michael Saboff 2018-07-19 16:05:37 PDT
<rdar://problem/42146858>
Comment 2 Michael Saboff 2018-07-20 09:58:12 PDT
Actually, DirectArguments structure can have either a NonArray indexingType or a ArrayStorageShape.  We need to allow for both possibilities.
Comment 3 Michael Saboff 2018-07-20 11:26:48 PDT
Created attachment 345466 [details]
Patch
Comment 4 Saam Barati 2018-07-20 11:30:14 PDT
Comment on attachment 345466 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        When filtering array modes for DirectArguments and ScopedArguments, we need to allow for the possibility that

not ScopedArguments.

> Source/JavaScriptCore/ChangeLog:10
> +        they can either be NonArray or NonArrayWithArrayStorage (aka ArrayStorageShape).

Might be worth a comment why this is. Also might worth noting why DirectArguments won't end up with Int32/Double/etc shapes.
(I'd put that explanation here or in a comment in the code perhaps, or both)
Comment 5 Saam Barati 2018-07-20 11:31:16 PDT
Actually, it might be worth checking out what ScopedArguments does for out of line arguments (the ones past the length of the arguments object). If like DirectArguments, it stores it in the Butterfly, you actually may need to handle ScopedArguments too
Comment 6 Michael Saboff 2018-07-20 14:13:07 PDT
(In reply to Saam Barati from comment #5)
> Actually, it might be worth checking out what ScopedArguments does for out
> of line arguments (the ones past the length of the arguments object). If
> like DirectArguments, it stores it in the Butterfly, you actually may need
> to handle ScopedArguments too

As we discussed, I added the change for ScopedArguments and a test as well.
Comment 7 Michael Saboff 2018-07-20 14:17:52 PDT
Created attachment 345481 [details]
Updated patch that includes ScopedArguments change
Comment 8 Saam Barati 2018-07-20 14:37:19 PDT
Comment on attachment 345481 [details]
Updated patch that includes ScopedArguments change

r=me
Comment 9 Michael Saboff 2018-07-20 16:48:18 PDT
Committed r234075: <https://trac.webkit.org/changeset/234075>