Bug 221805 - [JSC] Enable JITCage on macOS
Summary: [JSC] Enable JITCage on macOS
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: 221907
Blocks:
  Show dependency treegraph
 
Reported: 2021-02-11 20:59 PST by Yusuke Suzuki
Modified: 2021-02-16 12:51 PST (History)
14 users (show)

See Also:


Attachments
Patch (5.74 KB, patch)
2021-02-11 21:00 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.59 KB, patch)
2021-02-12 21:44 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.96 KB, patch)
2021-02-12 22:50 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (8.99 KB, patch)
2021-02-16 03:01 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-02-11 20:59:53 PST
[JSC] Enable JITCage on macOS
Comment 1 Yusuke Suzuki 2021-02-11 21:00:49 PST
Created attachment 420090 [details]
Patch
Comment 2 Yusuke Suzuki 2021-02-11 21:00:52 PST
<rdar://problem/74153806>
Comment 3 Mark Lam 2021-02-11 21:11:30 PST
Comment on attachment 420090 [details]
Patch

r=me
Comment 4 Yusuke Suzuki 2021-02-12 21:44:19 PST
Created attachment 420208 [details]
Patch
Comment 5 Mark Lam 2021-02-12 22:09:11 PST
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 6 Yusuke Suzuki 2021-02-12 22:50:15 PST
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.
Comment 7 Yusuke Suzuki 2021-02-12 22:50:25 PST
Created attachment 420212 [details]
Patch
Comment 8 Yusuke Suzuki 2021-02-13 02:12:09 PST
Landing
Comment 9 Yusuke Suzuki 2021-02-13 02:14:03 PST
Committed r272831 (234067@main): <https://commits.webkit.org/234067@main>
Comment 10 Brent Fulgham 2021-02-15 09:52:08 PST
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.
Comment 11 WebKit Commit Bot 2021-02-15 10:13:20 PST
Re-opened since this is blocked by bug 221907
Comment 12 Yusuke Suzuki 2021-02-16 03:01:19 PST
Created attachment 420444 [details]
Patch
Comment 13 Mark Lam 2021-02-16 09:52:32 PST
Comment on attachment 420444 [details]
Patch

LGTM.
Comment 14 EWS 2021-02-16 12:51:12 PST
Committed r272922: <https://commits.webkit.org/r272922>

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