Bug 220477 - [JSC] Disable JITCage compile time in old iOS
Summary: [JSC] Disable JITCage compile time in old iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-08 12:59 PST by Yusuke Suzuki
Modified: 2021-01-08 16:02 PST (History)
11 users (show)

See Also:


Attachments
Patch (2.57 KB, patch)
2021-01-08 12:59 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (3.08 KB, patch)
2021-01-08 13:06 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (3.08 KB, patch)
2021-01-08 13:08 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2021-01-08 12:59:19 PST
[JSC] Disable JITCage compile time in old iOS
Comment 1 Yusuke Suzuki 2021-01-08 12:59:46 PST
Created attachment 417292 [details]
Patch
Comment 2 Darin Adler 2021-01-08 13:00:59 PST
Comment on attachment 417292 [details]
Patch

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

> Source/WTF/wtf/PlatformEnable.h:883
> -#if OS(DARWIN) && ENABLE(JIT) && ((USE(APPLE_INTERNAL_SDK) && CPU(ARM64E)))
> +#if OS(DARWIN) && ENABLE(JIT) && ((USE(APPLE_INTERNAL_SDK) && CPU(ARM64E))) && defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 150000

This also disables JITCage on macOS. Is that correct?
Comment 3 Darin Adler 2021-01-08 13:01:41 PST
Comment on attachment 417292 [details]
Patch

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

>> Source/WTF/wtf/PlatformEnable.h:883
>> +#if OS(DARWIN) && ENABLE(JIT) && ((USE(APPLE_INTERNAL_SDK) && CPU(ARM64E))) && defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 150000
> 
> This also disables JITCage on macOS. Is that correct?

Also, I suggest we remove the extra parentheses that make this expression harder to read.
Comment 4 Yusuke Suzuki 2021-01-08 13:02:28 PST
Comment on attachment 417292 [details]
Patch

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

>> Source/WTF/wtf/PlatformEnable.h:883
>> +#if OS(DARWIN) && ENABLE(JIT) && ((USE(APPLE_INTERNAL_SDK) && CPU(ARM64E))) && defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 150000
> 
> This also disables JITCage on macOS. Is that correct?

Yes. And we are not enabling it in macOS (even if ENABLE(JIT_CAGE) is true, anyway, we will not use JITCage because Options::useJITCage becomes false for macOS). And our plan is not using it on macOS for now.
Comment 5 Yusuke Suzuki 2021-01-08 13:06:01 PST
Comment on attachment 417292 [details]
Patch

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

>>>> Source/WTF/wtf/PlatformEnable.h:883
>>>> +#if OS(DARWIN) && ENABLE(JIT) && ((USE(APPLE_INTERNAL_SDK) && CPU(ARM64E))) && defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 150000
>>> 
>>> This also disables JITCage on macOS. Is that correct?
>> 
>> Also, I suggest we remove the extra parentheses that make this expression harder to read.
> 
> Yes. And we are not enabling it in macOS (even if ENABLE(JIT_CAGE) is true, anyway, we will not use JITCage because Options::useJITCage becomes false for macOS). And our plan is not using it on macOS for now.

And in macOS, it is not enabled even without this patch as expected (Options.cpp).
Comment 6 Yusuke Suzuki 2021-01-08 13:06:23 PST
Created attachment 417295 [details]
Patch
Comment 7 Yusuke Suzuki 2021-01-08 13:06:40 PST
Comment on attachment 417295 [details]
Patch

Oops, I cleared the old patch accidentally.
Comment 8 Yusuke Suzuki 2021-01-08 13:08:04 PST
Created attachment 417296 [details]
Patch
Comment 9 Darin Adler 2021-01-08 13:21:52 PST
Comment on attachment 417296 [details]
Patch

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

> Source/WTF/wtf/PlatformEnable.h:883
> +#if OS(DARWIN) && ENABLE(JIT) && USE(APPLE_INTERNAL_SDK) && CPU(ARM64E) && defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 150000

We should probably move this to PlatformEnableCocoa.h and take out redundant checks like OS(DARWIN). iOS-specific flags are mostly in there. But maybe it needs to be here so it’s after ENABLE(JIT) is set?
Comment 10 Yusuke Suzuki 2021-01-08 13:58:32 PST
Comment on attachment 417296 [details]
Patch

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

>> Source/WTF/wtf/PlatformEnable.h:883
>> +#if OS(DARWIN) && ENABLE(JIT) && USE(APPLE_INTERNAL_SDK) && CPU(ARM64E) && defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 150000
> 
> We should probably move this to PlatformEnableCocoa.h and take out redundant checks like OS(DARWIN). iOS-specific flags are mostly in there. But maybe it needs to be here so it’s after ENABLE(JIT) is set?

Yes, we need ENABLE(JIT) to determine this status, so we cannot put it in EnableCocoa.h.
Comment 11 Yusuke Suzuki 2021-01-08 15:08:32 PST
Landing since the remaining EWS are not on ARM => (ENABLE(JIT_CAGE) was disabled before).
Comment 12 EWS 2021-01-08 16:01:03 PST
Committed r271332: <https://trac.webkit.org/changeset/271332>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417296 [details].
Comment 13 Radar WebKit Bug Importer 2021-01-08 16:02:19 PST
<rdar://problem/72947770>