Increase the alignment requirement of WKObject's wrapped C++ object
Created attachment 326529 [details] Patch
Comment on attachment 326529 [details] Patch Attachment 326529 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5172330 Number of test failures exceeded the failure limit.
Created attachment 326535 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 326529 [details] Patch Attachment 326529 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5172450 Number of test failures exceeded the failure limit.
Created attachment 326537 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 326643 [details] Patch
Comment on attachment 326643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326643&action=review > Source/WebKit/Shared/Cocoa/APIObject.mm:86 > +static const size_t minimumObjectAlignment = 8; > +static const size_t maximumExtraSpaceForAlignment = minimumObjectAlignment - alignof(void*); > +static_assert(minimumObjectAlignment >= alignof(void*), "Objects should always be at least pointer-aligned."); Do we really always want to do this alignment? Or just in some platform?
Comment on attachment 326643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326643&action=review > Source/WebKit/Shared/Cocoa/APIObject.mm:87 > + I'd put the assertion before the subtraction it's guarding against underflow. > Source/WebKit/Shared/Cocoa/APIObject.mm:111 > + void* api = object_getIndexedIvars(obj); > + api = std::align(minimumObjectAlignment, size, api, spaceAvailable); > + return *static_cast<API::Object*>(api); I’d have probably written this entire function as just two statements. Can you at least combine 109 with 110, or is there something about std::align that makes that impossible?
Comment on attachment 326643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326643&action=review >> Source/WebKit/Shared/Cocoa/APIObject.mm:86 >> +static_assert(minimumObjectAlignment >= alignof(void*), "Objects should always be at least pointer-aligned."); > > Do we really always want to do this alignment? Or just in some platform? We do, as mentioned on IRC. >> Source/WebKit/Shared/Cocoa/APIObject.mm:87 >> + > > I'd put the assertion before the subtraction it's guarding against underflow. Seems reasonable. >> Source/WebKit/Shared/Cocoa/APIObject.mm:111 >> + return *static_cast<API::Object*>(api); > > I’d have probably written this entire function as just two statements. Can you at least combine 109 with 110, or is there something about std::align that makes that impossible? For whatever reason, std::align is void* align( std::size_t, std::size_t, void*&, std::size_t&); so I /think/ the best I can do is merge the last two lines.
(In reply to Tim Horton from comment #9) > Comment on attachment 326643 [details] > Patch > >> Source/WebKit/Shared/Cocoa/APIObject.mm:111 > >> + return *static_cast<API::Object*>(api); > > > > I’d have probably written this entire function as just two statements. Can you at least combine 109 with 110, or is there something about std::align that makes that impossible? > > For whatever reason, std::align is void* align( std::size_t, std::size_t, > void*&, std::size_t&); so I /think/ the best I can do is merge the last two > lines. Otherwise, lots of "non-const lvalue reference cannot bind to a temporary..."
Created attachment 326686 [details] Patch
Comment on attachment 326686 [details] Patch Clearing flags on attachment: 326686 Committed r224729: <https://trac.webkit.org/changeset/224729>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35562125>