Summary: | Increase the alignment requirement of WKObject's wrapped C++ object | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||||
Component: | New Bugs | Assignee: | Tim Horton <thorton> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aestes, andersca, buildbot, commit-queue, mitz, rniwa, sam, simon.fraser, webkit-bug-importer, wenson_hsieh | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 199523 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Tim Horton
2017-11-09 18:41:21 PST
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. |