DFG should do caging

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

...

[Filip Pizlo]

Created attachment 317673 [details]
the patch

[Filip Pizlo]

Created attachment 317677 [details]
the patch

[Filip Pizlo]

Created attachment 317679 [details]
the patch

[Saam Barati]

Comment on attachment 317679 [details]
the patch

r=me

[Mark Lam]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/220596/webkit