Bug 179518 – Increase the alignment requirement of WKObject's wrapped C++ object

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

[Tim Horton]

Increase the alignment requirement of WKObject's wrapped C++ object

[Tim Horton]

Created attachment 326529 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Tim Horton]

Created attachment 326643 [details]
Patch

[Ryosuke Niwa]

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?

[Tim Horton]

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.

[Tim Horton]

(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..."

[Tim Horton]

Created attachment 326686 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 326686 [details]
Patch

Clearing flags on attachment: 326686

Committed r224729: <https://trac.webkit.org/changeset/224729>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35562125>