Bug 200139 – [JSC] Ensure PackedCellPtr only takes non-large-allocation pointers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Ensure PackedCellPtr only takes non-large-allocation pointers

[Yusuke Suzuki]

Created attachment 374910 [details]
Patch

[Yusuke Suzuki]

Created attachment 374911 [details]
Patch

[Saam Barati]

Comment on attachment 374910 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        But this fact only meets when the JSCell is not large allocation. Currently, we are using PackedCellPtr

really? We don't round large allocations?

[Yusuke Suzuki]

Created attachment 374912 [details]
Patch

[Keith Miller]

Comment on attachment 374910 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        But this fact only meets when the JSCell is not large allocation. Currently, we are using PackedCellPtr
> 
> really? We don't round large allocations?

grammar: only meets => only holds.

[Yusuke Suzuki]

Comment on attachment 374910 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:9
>>> +        But this fact only meets when the JSCell is not large allocation. Currently, we are using PackedCellPtr
>> 
>> really? We don't round large allocations?
> 
> grammar: only meets => only holds.

Fixed the grammar part, thanks.
Let's see LargeAllocation::isLargeAllocation implementation.

https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/heap/LargeAllocation.h#L58-L61
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/heap/LargeAllocation.h#L148-L149

LargeAllocation's HeapCell is intentionally not 16-byte aligned to encode this information into the cell pointer.

[Yusuke Suzuki]

Created attachment 374915 [details]
Patch

[Mark Lam]

Comment on attachment 374915 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        But this fact only meets when the JSCell is not large allocation. Currently, we are using PackedCellPtr

still not fixed: /meet/holds/

> Source/JavaScriptCore/ChangeLog:10
> +        only for the cell types which holds the above requirement. But we would like to ensure that statically.

this one should be 'meet' instead of 'holds'.

[Saam Barati]

Comment on attachment 374915 [details]
Patch

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

> Source/JavaScriptCore/heap/PackedCellPtr.h:43
> +        ASSERT(!pointer || !pointer->isLargeAllocation());

Can you just assert that it's 16 byte aligned? Seems more straight forward

[Yusuke Suzuki]

Comment on attachment 374915 [details]
Patch

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

Thanks, fixed.

>> Source/JavaScriptCore/ChangeLog:9
>> +        But this fact only meets when the JSCell is not large allocation. Currently, we are using PackedCellPtr
> 
> still not fixed: /meet/holds/

Oops, fixed.

>> Source/JavaScriptCore/ChangeLog:10
>> +        only for the cell types which holds the above requirement. But we would like to ensure that statically.
> 
> this one should be 'meet' instead of 'holds'.

Fixed.

[Yusuke Suzuki]

Comment on attachment 374915 [details]
Patch

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

>> Source/JavaScriptCore/heap/PackedCellPtr.h:43
>> +        ASSERT(!pointer || !pointer->isLargeAllocation());
> 
> Can you just assert that it's 16 byte aligned? Seems more straight forward

Sounds nice. Fixed.

[Yusuke Suzuki]

Committed r247843: <https://trac.webkit.org/changeset/247843>

[Radar WebKit Bug Importer]

<rdar://problem/53564083>

[Yusuke Suzuki]

It seems that std::invoke_result_t is missing in some clang environments. I think it is C++17 features, and possibly it is not implemented yet. Use std::result_of instead, while it is deprecated in C++17 and removed in C++20, because we are now using std::result_of in the other places.

[Yusuke Suzuki]

Committed r247844: <https://trac.webkit.org/changeset/247844>