Bug 169155

Summary: Take advantage of fast permissions switching of JIT memory for devices that support it
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, mitz, saam
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
saam: review+
Updated Patch - Addressed the concerns with the prior patch. saam: review+

Description Michael Saboff 2017-03-03 16:39:50 PST 
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.
Comment 1 Michael Saboff 2017-03-03 16:40:40 PST 
<rdar://problem/30845044>
Comment 2 Michael Saboff 2017-03-03 16:55:18 PST 
Created attachment 303359 [details]
Patch
Comment 3 WebKit Commit Bot 2017-03-03 16:58:04 PST 
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.
Comment 4 Saam Barati 2017-03-03 17:01:12 PST 
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.
Comment 5 Michael Saboff 2017-03-03 17:03:54 PST 
I fixed the omission of the bug number in Source/WebKit2/ChangeLog locally.
Comment 6 Michael Saboff 2017-03-03 17:04:49 PST 
(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 7 mitz 2017-03-03 17:09:47 PST 
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.
Comment 8 Michael Saboff 2017-03-03 17:18:39 PST 
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.
Comment 9 Saam Barati 2017-03-03 21:04:14 PST 
(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 <=
Comment 10 Michael Saboff 2017-03-06 11:32:15 PST 
Created attachment 303530 [details]
Updated Patch - Addressed the concerns with the prior patch.
Comment 11 Saam Barati 2017-03-06 11:44:27 PST 
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.
Comment 12 BJ Burg 2017-03-06 15:40:32 PST 
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.
Comment 13 Michael Saboff 2017-03-06 17:51:22 PST 
Committed r213483: <http://trac.webkit.org/changeset/213483>