Bug 221805 – [JSC] Enable JITCage on macOS

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Enable JITCage on macOS

[Yusuke Suzuki]

Created attachment 420090 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/74153806>

[Mark Lam]

Comment on attachment 420090 [details]
Patch

r=me

[Yusuke Suzuki]

Created attachment 420208 [details]
Patch

[Mark Lam]

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"?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 420212 [details]
Patch

[Yusuke Suzuki]

Landing

[Yusuke Suzuki]

Committed r272831 (234067@main): <https://commits.webkit.org/234067@main>

[Brent Fulgham]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 221907

[Yusuke Suzuki]

Created attachment 420444 [details]
Patch

[Mark Lam]

Comment on attachment 420444 [details]
Patch

LGTM.

[EWS]

Committed r272922: <https://commits.webkit.org/r272922>

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