| Summary: | [JSC] Disable JITCage compile time in old iOS | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Yusuke Suzuki
2021-01-08 12:59:19 PST
Created attachment 417292 [details]
Patch
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 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 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 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). Created attachment 417295 [details]
Patch
Comment on attachment 417295 [details]
Patch
Oops, I cleared the old patch accidentally.
Created attachment 417296 [details]
Patch
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 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. Landing since the remaining EWS are not on ARM => (ENABLE(JIT_CAGE) was disabled before). Committed r271332: <https://trac.webkit.org/changeset/271332> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417296 [details]. |