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+

Mark Lam
Reported 2020-07-29 01:49:43 PDT
...
Attachments
proposed patch. (5.75 KB, patch)
2020-07-29 01:53 PDT, Mark Lam 		darin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2020-07-29 01:53:09 PDT
Created attachment 405444 [details] proposed patch.
Darin Adler
Comment 2 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.
Mark Lam
Comment 3 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.
Mark Lam
Comment 4 2020-07-29 09:55:37 PDT
Landed in r265045: <http://trac.webkit.org/r265045>.
Radar WebKit Bug Importer
Comment 5 2020-07-29 09:56:19 PDT
<rdar://problem/66278689>
Note You need to log in before you can comment on or make changes to this bug.