Bug 169155 – Take advantage of fast permissions switching of JIT memory for devices that support it

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

The iOS OS team is providing new SPIs to allow for quick switching of the JIT memory region mapped RWX.  These SPIs allow switching all RWX mappings from RX to RW and back on a per thread basis with a single call.

[Michael Saboff]

<rdar://problem/30845044>

[Michael Saboff]

Created attachment 303359 [details]
Patch

[WebKit Commit Bot]

Attachment 303359 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 303359 [details]
Patch

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

r=me

> Source/JavaScriptCore/jit/ExecutableAllocator.h:93
> +    if ((uintptr_t)dst >= startOfFixedExecutableMemoryPool && (uintptr_t)dst <= endOfFixedExecutableMemoryPool) {

This shouldn't be < for the end?
I know the code before says <= too, but it looks a bit weird to me.

[Michael Saboff]

I fixed the omission of the bug number in Source/WebKit2/ChangeLog locally.

[Michael Saboff]

(In reply to comment #4)
> Comment on attachment 303359 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303359&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/jit/ExecutableAllocator.h:93
> > +    if ((uintptr_t)dst >= startOfFixedExecutableMemoryPool && (uintptr_t)dst <= endOfFixedExecutableMemoryPool) {
> 
> This shouldn't be < for the end?
> I know the code before says <= too, but it looks a bit weird to me.

I fixed this locally.

Comment on attachment 303359 [details]
Patch

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

> Source/JavaScriptCore/Configurations/Base.xcconfig:146
> +SYSTEM_PRIVATE_IOS_HEADERS_ios[arch=arm64] = $(SYSTEM_PRIVATE_IOS_HEADERS_ios_$(IPHONEOS_DEPLOYMENT_TARGET:base)_$(HAVE_INTERNAL_SDK));
> +SYSTEM_PRIVATE_IOS_HEADERS_ios_11_YES = -isystem $(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders;

Seems unnecessary to limit this to a specific version of iOS and a specific architecture. It is harmless to include this system header search path when building for other versions of iOS and other architectures.

> Source/JavaScriptCore/Configurations/Base.xcconfig:148
> +OTHER_CPLUSPLUSFLAGS[sdk=macosx*] = $(ASAN_OTHER_CPLUSPLUSFLAGS) -isystem icu;

Seems like you can add $(SYSTEM_PRIVATE_IOS_HEADERS) unconditionally to OTHER_CPLUSPLUSFLAGS, because it’s already defined to be empty when not targeting iOS.

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:218
> +ENABLE_FAST_JIT_PERMISSIONS_ios_11_YES = ENABLE_FAST_JIT_PERMISSIONS;

Seems bad to make enabling the feature in iOS 11 the exception rather than the rule. A more future-proof definition would be one that enables it as a rule and disables it for the legacy case, iOS 10.x, of which we’ll eventually be able to get rid.

> Source/JavaScriptCore/Configurations/ToolExecutable.xcconfig:53
> +OTHER_CPLUSPLUSFLAGS = $(ASAN_OTHER_CPLUSPLUSFLAGS) $(OTHER_CPLUSPLUSFLAGS);

We normally use $(inherited) instead of spelling out the setting name again. But what makes this definition necessary at all?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:8873
> -		417DA6D013734E02007C57FB /* libWebCoreTestSupport.dylib */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.dylib"; includeInIndex = 0; path = libWebCoreTestSupport.dylib; sourceTree = BUILT_PRODUCTS_DIR; };
> +		417DA6D013734E02007C57FB /* .dylib */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.dylib"; includeInIndex = 0; name = .dylib; path = libWebCoreTestSupport.dylib; sourceTree = BUILT_PRODUCTS_DIR; };

Seems wrong to change this.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:15352
> -				417DA6D013734E02007C57FB /* libWebCoreTestSupport.dylib */,
> +				417DA6D013734E02007C57FB /* .dylib */,

and this.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:29089
> -			productReference = 417DA6D013734E02007C57FB /* libWebCoreTestSupport.dylib */;
> +			productReference = 417DA6D013734E02007C57FB /* .dylib */;

and this.

[Michael Saboff]

Thanks for the comments.

(In reply to comment #7)
> Comment on attachment 303359 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303359&action=review
> 
> > Source/JavaScriptCore/Configurations/Base.xcconfig:146
> > +SYSTEM_PRIVATE_IOS_HEADERS_ios[arch=arm64] = $(SYSTEM_PRIVATE_IOS_HEADERS_ios_$(IPHONEOS_DEPLOYMENT_TARGET:base)_$(HAVE_INTERNAL_SDK));
> > +SYSTEM_PRIVATE_IOS_HEADERS_ios_11_YES = -isystem $(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders;
> 
> Seems unnecessary to limit this to a specific version of iOS and a specific
> architecture. It is harmless to include this system header search path when
> building for other versions of iOS and other architectures.
> 
> > Source/JavaScriptCore/Configurations/Base.xcconfig:148
> > +OTHER_CPLUSPLUSFLAGS[sdk=macosx*] = $(ASAN_OTHER_CPLUSPLUSFLAGS) -isystem icu;
> 
> Seems like you can add $(SYSTEM_PRIVATE_IOS_HEADERS) unconditionally to
> OTHER_CPLUSPLUSFLAGS, because it’s already defined to be empty when not
> targeting iOS.
> 
> > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:218
> > +ENABLE_FAST_JIT_PERMISSIONS_ios_11_YES = ENABLE_FAST_JIT_PERMISSIONS;
> 
> Seems bad to make enabling the feature in iOS 11 the exception rather than
> the rule. A more future-proof definition would be one that enables it as a
> rule and disables it for the legacy case, iOS 10.x, of which we’ll
> eventually be able to get rid.
> 
> > Source/JavaScriptCore/Configurations/ToolExecutable.xcconfig:53
> > +OTHER_CPLUSPLUSFLAGS = $(ASAN_OTHER_CPLUSPLUSFLAGS) $(OTHER_CPLUSPLUSFLAGS);
> 
> We normally use $(inherited) instead of spelling out the setting name again.
> But what makes this definition necessary at all?
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:8873
> > -		417DA6D013734E02007C57FB /* libWebCoreTestSupport.dylib */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.dylib"; includeInIndex = 0; path = libWebCoreTestSupport.dylib; sourceTree = BUILT_PRODUCTS_DIR; };
> > +		417DA6D013734E02007C57FB /* .dylib */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.dylib"; includeInIndex = 0; name = .dylib; path = libWebCoreTestSupport.dylib; sourceTree = BUILT_PRODUCTS_DIR; };

I'll make your suggested changes and then repost.

> Seems wrong to change this.
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:15352
> > -				417DA6D013734E02007C57FB /* libWebCoreTestSupport.dylib */,
> > +				417DA6D013734E02007C57FB /* .dylib */,
> 
> and this.
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:29089
> > -			productReference = 417DA6D013734E02007C57FB /* libWebCoreTestSupport.dylib */;
> > +			productReference = 417DA6D013734E02007C57FB /* .dylib */;
> 
> and this.

These were are result of changing via Xcode at some point.  I'll revert them.

[Saam Barati]

(In reply to comment #4)
> Comment on attachment 303359 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303359&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/jit/ExecutableAllocator.h:93
> > +    if ((uintptr_t)dst >= startOfFixedExecutableMemoryPool && (uintptr_t)dst <= endOfFixedExecutableMemoryPool) {
> 
> This shouldn't be < for the end?
> I know the code before says <= too, but it looks a bit weird to me.

Oops this is not proper English. I meant to say:
I think it should be <, not <=

[Michael Saboff]

Created attachment 303530 [details]
Updated Patch - Addressed the concerns with the prior patch.

[Saam Barati]

Comment on attachment 303530 [details]
Updated Patch - Addressed the concerns with the prior patch.

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

r=me on the JSC part. I'll let others approve the various build-related changes.

> Source/JavaScriptCore/jit/ExecutableAllocator.h:93
> +    if ((uintptr_t)dst >= startOfFixedExecutableMemoryPool && (uintptr_t)dst < endOfFixedExecutableMemoryPool) {

Style nit: I know this is what the code used to do, but I think we should use static_cast instead of C-style casts.

[Blaze Burg]

Comment on attachment 303530 [details]
Updated Patch - Addressed the concerns with the prior patch.

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

> Source/JavaScriptCore/Configurations/ToolExecutable.xcconfig:53
> +OTHER_CPLUSPLUSFLAGS = $(ASAN_OTHER_CPLUSPLUSFLAGS) $(inherited);

This definition is redundant with that in Base.xcconfig. You can remove this one.

[Michael Saboff]

Committed r213483: <http://trac.webkit.org/changeset/213483>

[Michael Saboff]

Follow up build fixes in change sets
  r213492: <http://trac.webkit.org/changeset/213492>
  r213496: <http://trac.webkit.org/changeset/213496>
  r213500: <http://trac.webkit.org/changeset/213500>