Bug 1575153 part 1 - Flush ICache when making code executable. r=tcampbell,lth
authorJan de Mooij <jdemooij@mozilla.com>
Mon, 16 Sep 2019 13:18:34 +0000
changeset 493548 90aeb73dadaf90adec6fce1d02fd1520b1d73704
parent 493547 840c4edafa021eeac6a7e6ae0e828d0adcfea92e
child 493549 f5bc71d6ee11838d067fd128a4436153378d7c7f
push id36585
push usermalexandru@mozilla.com
push dateWed, 18 Sep 2019 09:56:40 +0000
treeherdermozilla-central@e71921b729c6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcampbell, lth
bugs1575153
milestone71.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1575153 part 1 - Flush ICache when making code executable. r=tcampbell,lth Flushing on mprotect is a lot simpler and it's easier to reason about correctness. The next patch will remove the AutoFlushICache infrastructure. Because mprotect is pretty expensive we've been moving away from code patching in perf-sensitive cases so I don't expect any serious regressions from this. (This patch could be optimized more, if needed, but it'd add more complexity.) Differential Revision: https://phabricator.services.mozilla.com/D45996
js/src/jit/ExecutableAllocator.cpp
js/src/jit/ExecutableAllocator.h
js/src/jit/JitRealm.h
js/src/jit/ProcessExecutableMemory.cpp
js/src/jit/ProcessExecutableMemory.h
js/src/jit/shared/AtomicOperations-shared-jit.cpp
js/src/jsapi-tests/testJitMacroAssembler.cpp
js/src/wasm/WasmBuiltins.cpp
js/src/wasm/WasmCode.cpp
--- a/js/src/jit/ExecutableAllocator.cpp
+++ b/js/src/jit/ExecutableAllocator.cpp
@@ -253,19 +253,21 @@ void ExecutableAllocator::addSizeOfCode(
     sizes->regexp += pool->m_codeBytes[CodeKind::RegExp];
     sizes->other += pool->m_codeBytes[CodeKind::Other];
     sizes->unused += pool->m_allocation.size - pool->usedCodeBytes();
   }
 }
 
 /* static */
 void ExecutableAllocator::reprotectPool(JSRuntime* rt, ExecutablePool* pool,
-                                        ProtectionSetting protection) {
+                                        ProtectionSetting protection,
+                                        MustFlushICache flushICache) {
   char* start = pool->m_allocation.pages;
-  if (!ReprotectRegion(start, pool->m_freePtr - start, protection)) {
+  if (!ReprotectRegion(start, pool->m_freePtr - start, protection,
+                       flushICache)) {
     MOZ_CRASH();
   }
 }
 
 /* static */
 void ExecutableAllocator::poisonCode(JSRuntime* rt,
                                      JitPoisonRangeVector& ranges) {
   MOZ_ASSERT(CurrentThreadCanAccessRuntime(rt));
@@ -285,32 +287,34 @@ void ExecutableAllocator::poisonCode(JSR
       continue;
     }
 
     MOZ_ASSERT(pool->m_refCount > 1);
 
     // Use the pool's mark bit to indicate we made the pool writable.
     // This avoids reprotecting a pool multiple times.
     if (!pool->isMarked()) {
-      reprotectPool(rt, pool, ProtectionSetting::Writable);
+      reprotectPool(rt, pool, ProtectionSetting::Writable, MustFlushICache::No);
       pool->mark();
     }
 
     // Note: we use memset instead of js::Poison because we want to poison
     // JIT code in release builds too. Furthermore, we don't want the
     // invalid-ObjectValue poisoning js::Poison does in debug builds.
     memset(ranges[i].start, JS_SWEPT_CODE_PATTERN, ranges[i].size);
     MOZ_MAKE_MEM_NOACCESS(ranges[i].start, ranges[i].size);
   }
 
-  // Make the pools executable again and drop references.
+  // Make the pools executable again and drop references. We don't flush the
+  // ICache here to not add extra overhead.
   for (size_t i = 0; i < ranges.length(); i++) {
     ExecutablePool* pool = ranges[i].pool;
     if (pool->isMarked()) {
-      reprotectPool(rt, pool, ProtectionSetting::Executable);
+      reprotectPool(rt, pool, ProtectionSetting::Executable,
+                    MustFlushICache::No);
       pool->unmark();
     }
     pool->release();
   }
 }
 
 ExecutablePool::Allocation ExecutableAllocator::systemAlloc(size_t n) {
   void* allocation = AllocateExecutableMemory(n, ProtectionSetting::Executable,
--- a/js/src/jit/ExecutableAllocator.h
+++ b/js/src/jit/ExecutableAllocator.h
@@ -161,27 +161,30 @@ class ExecutableAllocator {
   // On OOM, this will return an Allocation where pages is nullptr.
   ExecutablePool::Allocation systemAlloc(size_t n);
   static void systemRelease(const ExecutablePool::Allocation& alloc);
 
   ExecutablePool* createPool(size_t n);
   ExecutablePool* poolForSize(size_t n);
 
   static void reprotectPool(JSRuntime* rt, ExecutablePool* pool,
-                            ProtectionSetting protection);
+                            ProtectionSetting protection,
+                            MustFlushICache flushICache);
 
  public:
   MOZ_MUST_USE
   static bool makeWritable(void* start, size_t size) {
-    return ReprotectRegion(start, size, ProtectionSetting::Writable);
+    return ReprotectRegion(start, size, ProtectionSetting::Writable,
+                           MustFlushICache::No);
   }
 
   MOZ_MUST_USE
-  static bool makeExecutable(void* start, size_t size) {
-    return ReprotectRegion(start, size, ProtectionSetting::Executable);
+  static bool makeExecutableAndFlushICache(void* start, size_t size) {
+    return ReprotectRegion(start, size, ProtectionSetting::Executable,
+                           MustFlushICache::Yes);
   }
 
   static void poisonCode(JSRuntime* rt, JitPoisonRangeVector& ranges);
 
  private:
   ExecutableAllocator(const ExecutableAllocator&) = delete;
   void operator=(const ExecutableAllocator&) = delete;
 
--- a/js/src/jit/JitRealm.h
+++ b/js/src/jit/JitRealm.h
@@ -711,17 +711,17 @@ class MOZ_RAII AutoWritableJitCodeFallib
       : AutoWritableJitCodeFallible(code->runtimeFromMainThread(), code->raw(),
                                     code->bufferSize()) {}
 
   MOZ_MUST_USE bool makeWritable() {
     return ExecutableAllocator::makeWritable(addr_, size_);
   }
 
   ~AutoWritableJitCodeFallible() {
-    if (!ExecutableAllocator::makeExecutable(addr_, size_)) {
+    if (!ExecutableAllocator::makeExecutableAndFlushICache(addr_, size_)) {
       MOZ_CRASH();
     }
     rt_->toggleAutoWritableJitCodeActive(false);
   }
 };
 
 // Infallible variant of AutoWritableJitCodeFallible, ensures writable during
 // construction
--- a/js/src/jit/ProcessExecutableMemory.cpp
+++ b/js/src/jit/ProcessExecutableMemory.cpp
@@ -704,17 +704,24 @@ bool js::jit::CanLikelyAllocateMoreExecu
   static const size_t BufferSize = 8 * 1024 * 1024;
 
   MOZ_ASSERT(execMemory.bytesAllocated() <= MaxCodeBytesPerProcess);
 
   return execMemory.bytesAllocated() + BufferSize <= MaxCodeBytesPerProcess;
 }
 
 bool js::jit::ReprotectRegion(void* start, size_t size,
-                              ProtectionSetting protection) {
+                              ProtectionSetting protection,
+                              MustFlushICache flushICache) {
+  // Flush ICache when making code executable, before we modify |size|.
+  if (flushICache == MustFlushICache::Yes) {
+    MOZ_ASSERT(protection == ProtectionSetting::Executable);
+    jit::FlushICache(start, size);
+  }
+
   // Calculate the start of the page containing this region,
   // and account for this extra memory within size.
   size_t pageSize = gc::SystemPageSize();
   intptr_t startPtr = reinterpret_cast<intptr_t>(start);
   intptr_t pageStartPtr = startPtr & ~(pageSize - 1);
   void* pageStart = reinterpret_cast<void*>(pageStartPtr);
   size += (startPtr - pageStartPtr);
 
--- a/js/src/jit/ProcessExecutableMemory.h
+++ b/js/src/jit/ProcessExecutableMemory.h
@@ -65,18 +65,21 @@ static const size_t MaxCodeBytesPerBuffe
 static const size_t ExecutableCodePageSize = 64 * 1024;
 
 enum class ProtectionSetting {
   Protected,  // Not readable, writable, or executable.
   Writable,
   Executable,
 };
 
+enum class MustFlushICache { No, Yes };
+
 extern MOZ_MUST_USE bool ReprotectRegion(void* start, size_t size,
-                                         ProtectionSetting protection);
+                                         ProtectionSetting protection,
+                                         MustFlushICache flushICache);
 
 // Functions called at process start-up/shutdown to initialize/release the
 // executable memory region.
 extern MOZ_MUST_USE bool InitProcessExecutableMemory();
 extern void ReleaseProcessExecutableMemory();
 
 // Allocate/deallocate executable pages.
 extern void* AllocateExecutableMemory(size_t bytes,
--- a/js/src/jit/shared/AtomicOperations-shared-jit.cpp
+++ b/js/src/jit/shared/AtomicOperations-shared-jit.cpp
@@ -871,21 +871,19 @@ bool InitializeJittedAtomics() {
 
   // Zero the padding.
   memset(code + codeLength, 0, roundedCodeLength - codeLength);
 
   // Copy the code into place but do not flush, as the flush path requires a
   // JSContext* we do not have.
   masm.executableCopy(code, /* flushICache = */ false);
 
-  // Flush the icache using a primitive method.
-  jit::FlushICache(code, roundedCodeLength);
-
   // Reprotect the whole region to avoid having separate RW and RX mappings.
-  if (!ExecutableAllocator::makeExecutable(code, roundedCodeLength)) {
+  if (!ExecutableAllocator::makeExecutableAndFlushICache(code,
+                                                         roundedCodeLength)) {
     DeallocateExecutableMemory(code, roundedCodeLength);
     return false;
   }
 
   // Create the function pointers.
 
   AtomicFenceSeqCst = (void (*)())(code + fenceSeqCst);
 
--- a/js/src/jsapi-tests/testJitMacroAssembler.cpp
+++ b/js/src/jsapi-tests/testJitMacroAssembler.cpp
@@ -44,17 +44,18 @@ static bool Execute(JSContext* cx, Macro
     return false;
   }
 
   Linker linker(masm, "Test");
   JitCode* code = linker.newCode(cx, CodeKind::Other);
   if (!code) {
     return false;
   }
-  if (!ExecutableAllocator::makeExecutable(code->raw(), code->bufferSize())) {
+  if (!ExecutableAllocator::makeExecutableAndFlushICache(code->raw(),
+                                                         code->bufferSize())) {
     return false;
   }
 
   JS::AutoSuppressGCAnalysis suppress;
   EnterTest test = code->as<EnterTest>();
   test();
   return true;
 }
--- a/js/src/wasm/WasmBuiltins.cpp
+++ b/js/src/wasm/WasmBuiltins.cpp
@@ -1210,19 +1210,18 @@ bool wasm::EnsureBuiltinThunksInitialize
 
   masm.processCodeLabels(thunks->codeBase);
   PatchDebugSymbolicAccesses(thunks->codeBase, masm);
 
   MOZ_ASSERT(masm.callSites().empty());
   MOZ_ASSERT(masm.callSiteTargets().empty());
   MOZ_ASSERT(masm.trapSites().empty());
 
-  jit::FlushICache(thunks->codeBase, thunks->codeSize);
-  if (!ExecutableAllocator::makeExecutable(thunks->codeBase,
-                                           thunks->codeSize)) {
+  if (!ExecutableAllocator::makeExecutableAndFlushICache(thunks->codeBase,
+                                                         thunks->codeSize)) {
     return false;
   }
 
   builtinThunks = thunks.release();
   return true;
 }
 
 void wasm::ReleaseBuiltinThunks() {
--- a/js/src/wasm/WasmCode.cpp
+++ b/js/src/wasm/WasmCode.cpp
@@ -373,21 +373,19 @@ UniqueModuleSegment ModuleSegment::creat
 bool ModuleSegment::initialize(const CodeTier& codeTier,
                                const LinkData& linkData,
                                const Metadata& metadata,
                                const MetadataTier& metadataTier) {
   if (!StaticallyLink(*this, linkData)) {
     return false;
   }
 
-  jit::FlushICache(base(), RoundupCodeLength(length()));
-
   // Reprotect the whole region to avoid having separate RW and RX mappings.
-  if (!ExecutableAllocator::makeExecutable(base(),
-                                           RoundupCodeLength(length()))) {
+  if (!ExecutableAllocator::makeExecutableAndFlushICache(
+          base(), RoundupCodeLength(length()))) {
     return false;
   }
 
   SendCodeRangesToProfiler(*this, metadata, metadataTier.codeRanges);
 
   // See comments in CodeSegment::initialize() for why this must be last.
   return CodeSegment::initialize(codeTier);
 }
@@ -719,18 +717,17 @@ bool LazyStubTier::createMany(const Uint
   masm.executableCopy(codePtr, /* flushICache = */ false);
   PatchDebugSymbolicAccesses(codePtr, masm);
   memset(codePtr + masm.bytesNeeded(), 0, codeLength - masm.bytesNeeded());
 
   for (const CodeLabel& label : masm.codeLabels()) {
     Assembler::Bind(codePtr, label);
   }
 
-  jit::FlushICache(codePtr, codeLength);
-  if (!ExecutableAllocator::makeExecutable(codePtr, codeLength)) {
+  if (!ExecutableAllocator::makeExecutableAndFlushICache(codePtr, codeLength)) {
     return false;
   }
 
   // Create lazy function exports for funcIndex -> entry lookup.
   if (!exports_.reserve(exports_.length() + funcExportIndices.length())) {
     return false;
   }