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.
<rdar://problem/30845044>
Created attachment 303359 [details] Patch
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 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 the omission of the bug number in Source/WebKit2/ChangeLog locally.
(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.
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.
(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 <=
Created attachment 303530 [details] Updated Patch - Addressed the concerns with the prior patch.
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 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.
Committed r213483: <http://trac.webkit.org/changeset/213483>
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>