Bug 174918 - DFG should do caging
Summary: DFG should do caging
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 174917
  Show dependency treegraph
 
Reported: 2017-07-27 19:35 PDT by Filip Pizlo
Modified: 2017-08-11 09:49 PDT (History)
9 users (show)

See Also:


Attachments
the patch (7.15 KB, patch)
2017-08-08 21:08 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (7.96 KB, patch)
2017-08-08 22:10 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (8.04 KB, patch)
2017-08-08 22:56 PDT, Filip Pizlo
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-07-27 19:35:56 PDT
...
Comment 1 Filip Pizlo 2017-08-08 21:08:51 PDT
Created attachment 317673 [details]
the patch
Comment 2 Filip Pizlo 2017-08-08 22:10:39 PDT
Created attachment 317677 [details]
the patch
Comment 3 Filip Pizlo 2017-08-08 22:56:26 PDT
Created attachment 317679 [details]
the patch
Comment 4 Saam Barati 2017-08-09 09:15:48 PDT
Comment on attachment 317679 [details]
the patch

r=me
Comment 5 Mark Lam 2017-08-09 10:39:20 PDT
Comment on attachment 317679 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6172
> +        cageTypedArrayStorage(storageReg);

Pardon my ignorance, but the code in compileGetTypedArrayByteOffset() below tells me that JSArrayBufferView::vector may be null.  So, why is the null check not applied here?

If the null check is always necessary, maybe it should be moved into cageTypedArrayStorage() instead.
Comment 6 Filip Pizlo 2017-08-11 09:44:13 PDT
(In reply to Mark Lam from comment #5)
> Comment on attachment 317679 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317679&action=review
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6172
> > +        cageTypedArrayStorage(storageReg);
> 
> Pardon my ignorance, but the code in compileGetTypedArrayByteOffset() below
> tells me that JSArrayBufferView::vector may be null.  So, why is the null
> check not applied here?

Null becomes something like 0x3000000000, which is OK so long as:

- You don't try to ==/!= this pointer with anything else.
- That implies never comparing the pointer to null.

For any other purpose, this pointer is no worse than null itself.

GetIndexedPropertyStorage is generated for GetByVal/PutByVal, which only use the pointer for dereferencing.  They only dereference it in cases where it would not have been null, so it's OK that null would have become something like 0x3000000000.

On the other hand, compileGetTypedArrayByteOffset compares the `vector` to something else.  If the view is neutered, that other something else would be null.  So then if we let vector become 0x3000000000, then the logic would fail.  Also, since this code is not super perf sensitive (it's not like everyone queries byteOffset a lot, at least from what I can tell), it's probably OK to have this null branch.

> 
> If the null check is always necessary, maybe it should be moved into
> cageTypedArrayStorage() instead.

If we did that, then we'd pay the price of the null check in cases where we don't need it, like if we're just using the vector for a GetByVal/PutByVal.

On the other hand, the C++ equivalent of this stuff always does the null check.
Comment 7 Filip Pizlo 2017-08-11 09:48:31 PDT
Landed in https://trac.webkit.org/changeset/220596/webkit