Bug 214914

Summary: Update some JSArrayBufferView comments and add some assertions.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, keith_miller, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch. darin: review+

Description Mark Lam 2020-07-29 01:49:43 PDT
...
Comment 1 Mark Lam 2020-07-29 01:53:09 PDT
Created attachment 405444 [details]
proposed patch.
Comment 2 Darin Adler 2020-07-29 09:39:44 PDT
Comment on attachment 405444 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/ArrayBuffer.cpp:213
> +    ASSERT(!Gigacage::isEnabled() || (Gigacage::contains(data) && Gigacage::contains(bitwise_cast<const uint8_t*>(data) + byteLength - 1)));

Converting from const void* to const uint8_t* requires only a static_cast, not a bitwise_cast. We should reserve bitwise_cast for the cases where it’s needed.

> Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:51
> +    ASSERT(!Gigacage::isEnabled() || (Gigacage::contains(vector) && Gigacage::contains(bitwise_cast<const uint8_t*>(vector) + length - 1)));

Ditto. Also, I suggest casting to uint8_t* for brevity rather than adding const. That can still be passed to a function that expects const uint8_t.
Comment 3 Mark Lam 2020-07-29 09:50:54 PDT
Comment on attachment 405444 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/runtime/ArrayBuffer.cpp:213
>> +    ASSERT(!Gigacage::isEnabled() || (Gigacage::contains(data) && Gigacage::contains(bitwise_cast<const uint8_t*>(data) + byteLength - 1)));
> 
> Converting from const void* to const uint8_t* requires only a static_cast, not a bitwise_cast. We should reserve bitwise_cast for the cases where it’s needed.

I'll switch static_cast.  const is still needed.  Otherwise Clang will complain about: error: static_cast from 'const void *' to 'uint8_t *' (aka 'unsigned char *') casts away qualifiers.

>> Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:51
>> +    ASSERT(!Gigacage::isEnabled() || (Gigacage::contains(vector) && Gigacage::contains(bitwise_cast<const uint8_t*>(vector) + length - 1)));
> 
> Ditto. Also, I suggest casting to uint8_t* for brevity rather than adding const. That can still be passed to a function that expects const uint8_t.

Ditto.
Comment 4 Mark Lam 2020-07-29 09:55:37 PDT
Landed in r265045: <http://trac.webkit.org/r265045>.
Comment 5 Radar WebKit Bug Importer 2020-07-29 09:56:19 PDT
<rdar://problem/66278689>