Summary: | [JSC] Enable JITCage on macOS | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, bfulgham, cdumez, cmarcelo, commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, pvollan, saam, thorton, tzagallo, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 221907 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2021-02-11 20:59:53 PST
Created attachment 420090 [details]
Patch
Comment on attachment 420090 [details]
Patch
r=me
Created attachment 420208 [details]
Patch
Comment on attachment 420208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420208&action=review LGTM but it would be good to get more reviewers to take a look at this. > Source/JavaScriptCore/ChangeLog:10 > + We need to add this entitlement only when building it on macOS 120000 or upper version. /upper/higher/ > Source/JavaScriptCore/ChangeLog:14 > + This patch follows to r248164's way: we must not use CODE_SIGN_ENTITLEMENTS because XCode inserts implicit code-signing /follows to r248164/follows r248164/ > Source/WTF/ChangeLog:9 > + Enable JIT_CAGE when macOS is 120000 or upper with ARM64E. /upper/higher/ > Source/WebKit/ChangeLog:9 > + We need to add this entitlement only when building it on macOS 120000 or upper version. /upper/higher/ > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:11345 > + name = "Generate Entitlements"; Can this have a different name? Maybe "Generate jsc Entitlements"? Or does it have to be this exact string? > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:11365 > + name = "Generate Entitlements"; Ditto. Maybe "Generate testapi entitlements"? Comment on attachment 420208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420208&action=review >> Source/JavaScriptCore/ChangeLog:10 >> + We need to add this entitlement only when building it on macOS 120000 or upper version. > > /upper/higher/ Fixed. >> Source/JavaScriptCore/ChangeLog:14 >> + This patch follows to r248164's way: we must not use CODE_SIGN_ENTITLEMENTS because XCode inserts implicit code-signing > > /follows to r248164/follows r248164/ Fixed. >> Source/WTF/ChangeLog:9 >> + Enable JIT_CAGE when macOS is 120000 or upper with ARM64E. > > /upper/higher/ Fixed. >> Source/WebKit/ChangeLog:9 >> + We need to add this entitlement only when building it on macOS 120000 or upper version. > > /upper/higher/ Fixed. >> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:11345 >> + name = "Generate Entitlements"; > > Can this have a different name? Maybe "Generate jsc Entitlements"? Or does it have to be this exact string? I think this is ok since this is a build phase inside "jsc" target. When building, we can see that this "Generate Entitlements" is running inside "jsc" build target. And the other build phases (compile, linking etc.) does not include "jsc" name too. >> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:11365 >> + name = "Generate Entitlements"; > > Ditto. Maybe "Generate testapi entitlements"? Ditto. > Source/JavaScriptCore/Scripts/process-entitlements.sh:27 > + plistbuddy Add :com.apple.rootless.storage.JavaScriptCore bool YES > + mac_process_jsc_entitlements This needs to be inside `if [[ "${WK_USE_RESTRICTED_ENTITLEMENTS}" == YES ]]` fixed. > Source/JavaScriptCore/Scripts/process-entitlements.sh:61 > + plistbuddy Add :com.apple.rootless.storage.JavaScriptCore bool YES This is not necessary. fixed. Created attachment 420212 [details]
Patch
Landing Committed r272831 (234067@main): <https://commits.webkit.org/234067@main> Comment on attachment 420212 [details]
Patch
For what it's worth, I think this looks correct. It would be good to have someone like Mitz look at the xcconf magic, but I can't see anything concerning.
Re-opened since this is blocked by bug 221907 Created attachment 420444 [details]
Patch
Comment on attachment 420444 [details]
Patch
LGTM.
Committed r272922: <https://commits.webkit.org/r272922> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420444 [details]. |