Bug 239054 – Adopt "version set"-based linked-on-or-after checks instead of platform-specific ones

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

[Tim Horton]

Adopt "version set"-based linked-on-or-after checks instead of platform-specific ones

[Tim Horton]

Created attachment 457212 [details]
Patch

[Tim Horton]

<rdar://problem/83436715>

[Tim Horton]

Comment on attachment 457212 [details]
Patch

Oh dear, I forgot about the SPI headers.

[Tim Horton]

Actually, not sure we can make SPI headers because of how dyld_build_version_t values are constructed. We might just have to always-be-linked-on-or-after-everything in the open source build? Though I can imagine folks objecting to that. Will ask around!

[Tim Horton]

Or we could fall back to the old mechanism if the SDK we're building against doesn't have the requisite dyld_build_version_t defines... that would solve both the open source build and back-deployment problems. I will try to hack that up.

[Tim Horton]

Created attachment 457220 [details]
Patch

[Tim Horton]

Created attachment 457221 [details]
Patch

[Tim Horton]

Created attachment 457222 [details]
Patch

[Tim Horton]

Created attachment 457223 [details]
Patch

[Tim Horton]

Created attachment 457224 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 457224 [details]
Patch

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

> Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.cpp:75
> +    if (linkedBefore(dyld_fall_2015_os_versions, DYLD_IOS_VERSION_9_0, DYLD_MACOSX_VERSION_10_11))
> +        disableBehavior(SDKAlignedBehavior::PictureInPictureMediaPlayback);
> +
> +    if (linkedBefore(dyld_fall_2016_os_versions, DYLD_IOS_VERSION_10_0, DYLD_MACOSX_VERSION_10_12)) {

This code could make use of the fact that if 'linkedBefore(dyld_fall_2015_os_versions ' is true, then 'linkedBefore(dyld_fall_2016_os_versions' must also be true.

[Wenson Hsieh]

Comment on attachment 457224 [details]
Patch

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

> Source/WTF/wtf/Bitmap.h:249
> +    memset(bits.data(), 0xFF, sizeof(bits));

We chatted about this on Slack — it seems this might not work well with the implementation of `Bitmap::count()`, since it would end up counting bits after the last (valid) bit in the map.

(Though, that shouldn't trigger any bugs, since we only use `setAll()` for the linked-on behaviors map below)

[Simon Fraser (smfr)]

Can this use a big OptionSet<> instead?

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #13)
> Can this use a big OptionSet<> instead?

We're already at 60 bits, IDK if uint128_t is a thing I can use, and since we almost never remove things from this list and add them at a greater frequency now, I think using something flexible makes the most sense.

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 457224 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457224&action=review
> 
> > Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.cpp:75
> > +    if (linkedBefore(dyld_fall_2015_os_versions, DYLD_IOS_VERSION_9_0, DYLD_MACOSX_VERSION_10_11))
> > +        disableBehavior(SDKAlignedBehavior::PictureInPictureMediaPlayback);
> > +
> > +    if (linkedBefore(dyld_fall_2016_os_versions, DYLD_IOS_VERSION_10_0, DYLD_MACOSX_VERSION_10_12)) {
> 
> This code could make use of the fact that if
> 'linkedBefore(dyld_fall_2015_os_versions ' is true, then
> 'linkedBefore(dyld_fall_2016_os_versions' must also be true.

It is true, though dyld_program_sdk_at_least is very cheap, and it's likely not worth the decreased ergonomics within this function that people have to frequently add to (and also might confuse the weird one-off one where the versions are different).

[Tim Horton]

(In reply to Wenson Hsieh from comment #12)
> Comment on attachment 457224 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457224&action=review
> 
> > Source/WTF/wtf/Bitmap.h:249
> > +    memset(bits.data(), 0xFF, sizeof(bits));
> 
> We chatted about this on Slack — it seems this might not work well with the
> implementation of `Bitmap::count()`, since it would end up counting bits
> after the last (valid) bit in the map.
> 
> (Though, that shouldn't trigger any bugs, since we only use `setAll()` for
> the linked-on behaviors map below)

I will fix this.

[Tim Horton]

Created attachment 457312 [details]
Patch

[Tim Horton]

Created attachment 457313 [details]
Patch

[Myles C. Maxfield]

Holy moly we have a lot of linkedOnOrAfter checks!

[Wenson Hsieh]

Comment on attachment 457313 [details]
Patch

r=mews

[EWS]

Committed r292793 (249576@main): <https://commits.webkit.org/249576@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457313 [details].