[heap] Forces CodeSpaceMemoryModificationScope only in safepoints

CodeSpaceMemoryModificationScope should only be used by the main
thread and during a safepoint. This adds a check in
CodeSpaceMemoryModificationScope.

The reason for this is that CodeSpaceMemoryModificationScope is not
thread-safe. It assumes that no other thread is modifying code space
(either by setting memory permission or adding a new page).

This CL also replaces CodeSpaceMemoryModificationScope to
CodePageCollectionMemoryModificationScope in a few occurrences, where
the former is not needed. This should not hurt performance.

Bug: v8:12054
Change-Id: I2675e667782c6ad8410877a4e64374899066bcd1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3263890
Commit-Queue: Victor Gomes <victorgomes@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77732}

src/builtins/setup-builtins-internal.cc

@@ -223,7 +223,7 @@ void SetupIsolateDelegate::ReplacePlaceholders(Isolate* isolate) {
   // Replace references from all builtin code objects to placeholders.
   Builtins* builtins = isolate->builtins();
   DisallowGarbageCollection no_gc;
-  CodeSpaceMemoryModificationScope modification_scope(isolate->heap());
+  CodePageCollectionMemoryModificationScope modification_scope(isolate->heap());
   static const int kRelocMask =
       RelocInfo::ModeMask(RelocInfo::CODE_TARGET) |
       RelocInfo::ModeMask(RelocInfo::FULL_EMBEDDED_OBJECT) |
@@ -233,6 +233,8 @@ void SetupIsolateDelegate::ReplacePlaceholders(Isolate* isolate) {
   for (Builtin builtin = Builtins::kFirst; builtin <= Builtins::kLast;
        ++builtin) {
     Code code = builtins->code(builtin);
+    isolate->heap()->UnprotectAndRegisterMemoryChunk(
+        code, UnprotectMemoryOrigin::kMainThread);
     bool flush_icache = false;
     for (RelocIterator it(code, kRelocMask); !it.done(); it.next()) {
       RelocInfo* rinfo = it.rinfo();

src/execution/isolate.cc

@@ -3831,7 +3831,7 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
 
   // If we are deserializing, read the state into the now-empty heap.
   {
-    CodeSpaceMemoryModificationScope modification_scope(heap());
+    CodePageCollectionMemoryModificationScope modification_scope(heap());
 
     if (create_heap_objects) {
       heap_.read_only_space()->ClearStringPaddingIfNeeded();

src/heap/heap-inl.h

@@ -30,6 +30,7 @@
 #include "src/heap/paged-spaces-inl.h"
 #include "src/heap/read-only-heap.h"
 #include "src/heap/read-only-spaces.h"
+#include "src/heap/safepoint.h"
 #include "src/heap/spaces-inl.h"
 #include "src/heap/third-party/heap-api.h"
 #include "src/objects/allocation-site-inl.h"
@@ -791,6 +792,8 @@ AlwaysAllocateScopeForTesting::AlwaysAllocateScopeForTesting(Heap* heap)
 
 CodeSpaceMemoryModificationScope::CodeSpaceMemoryModificationScope(Heap* heap)
     : heap_(heap) {
+  DCHECK_EQ(ThreadId::Current(), heap_->isolate()->thread_id());
+  heap_->safepoint()->AssertActive();
   if (heap_->write_protect_code_memory()) {
     heap_->increment_code_space_memory_modification_scope_depth();
     heap_->code_space()->SetCodeModificationPermissions();

src/heap/heap.cc

@@ -5420,7 +5420,7 @@ void Heap::DisableInlineAllocation() {
 
   // Update inline allocation limit for old spaces.
   PagedSpaceIterator spaces(this);
-  CodeSpaceMemoryModificationScope modification_scope(this);
+  CodePageCollectionMemoryModificationScope modification_scope(this);
   for (PagedSpace* space = spaces.Next(); space != nullptr;
        space = spaces.Next()) {
     base::MutexGuard guard(space->mutex());

src/wasm/module-compiler.cc

@@ -17,7 +17,7 @@
 #include "src/base/utils/random-number-generator.h"
 #include "src/compiler/wasm-compiler.h"
 #include "src/handles/global-handles-inl.h"
-#include "src/heap/heap-inl.h"  // For CodeSpaceMemoryModificationScope.
+#include "src/heap/heap-inl.h"  // For CodePageCollectionMemoryModificationScope.
 #include "src/logging/counters-scopes.h"
 #include "src/logging/metrics.h"
 #include "src/objects/property-descriptor.h"
@@ -3360,12 +3360,13 @@ void CompilationStateImpl::FinalizeJSToWasmWrappers(
   *export_wrappers_out = isolate->factory()->NewFixedArray(
       MaxNumExportWrappers(module), AllocationType::kOld);
   // TODO(6792): Wrappers below are allocated with {Factory::NewCode}. As an
-  // optimization we keep the code space unlocked to avoid repeated unlocking
-  // because many such wrapper are allocated in sequence below.
+  // optimization we create a code memory modification scope that avoids
+  // changing the page permissions back-and-forth between RWX and RX, because
+  // many such wrapper are allocated in sequence below.
   TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("v8.wasm.detailed"),
                "wasm.FinalizeJSToWasmWrappers", "wrappers",
                js_to_wasm_wrapper_units_.size());
-  CodeSpaceMemoryModificationScope modification_scope(isolate->heap());
+  CodePageCollectionMemoryModificationScope modification_scope(isolate->heap());
   for (auto& unit : js_to_wasm_wrapper_units_) {
     DCHECK_EQ(isolate, unit->isolate());
     Handle<Code> code = unit->Finalize();
@@ -3798,9 +3799,10 @@ void CompileJsToWasmWrappers(Isolate* isolate, const WasmModule* module,
 
   // Finalize compilation jobs in the main thread.
   // TODO(6792): Wrappers below are allocated with {Factory::NewCode}. As an
-  // optimization we keep the code space unlocked to avoid repeated unlocking
-  // because many such wrapper are allocated in sequence below.
-  CodeSpaceMemoryModificationScope modification_scope(isolate->heap());
+  // optimization we create a code memory modification scope that avoids
+  // changing the page permissions back-and-forth between RWX and RX, because
+  // many such wrapper are allocated in sequence below.
+  CodePageCollectionMemoryModificationScope modification_scope(isolate->heap());
   for (auto& pair : compilation_units) {
     JSToWasmWrapperKey key = pair.first;
     JSToWasmWrapperCompilationUnit* unit = pair.second.get();

test/cctest/heap/heap-utils.cc

@@ -198,7 +198,7 @@ void SimulateFullSpace(v8::internal::PagedSpace* space) {
   // FLAG_stress_concurrent_allocation = false;
   // Background thread allocating concurrently interferes with this function.
   CHECK(!FLAG_stress_concurrent_allocation);
-  CodeSpaceMemoryModificationScope modification_scope(space->heap());
+  CodePageCollectionMemoryModificationScope modification_scope(space->heap());
   i::MarkCompactCollector* collector = space->heap()->mark_compact_collector();
   if (collector->sweeping_in_progress()) {
     collector->EnsureSweepingCompleted();

test/cctest/heap/test-concurrent-allocation.cc

@@ -478,50 +478,50 @@ UNINITIALIZED_TEST(ConcurrentRecordRelocSlot) {
   v8::Isolate* isolate = v8::Isolate::New(create_params);
   Isolate* i_isolate = reinterpret_cast<Isolate*>(isolate);
   Heap* heap = i_isolate->heap();
-
-  Code code;
-  HeapObject value;
   {
-    HandleScope handle_scope(i_isolate);
-    i::byte buffer[i::Assembler::kDefaultBufferSize];
-    MacroAssembler masm(i_isolate, v8::internal::CodeObjectRequired::kYes,
-                        ExternalAssemblerBuffer(buffer, sizeof(buffer)));
+    Code code;
+    HeapObject value;
+    CodePageCollectionMemoryModificationScope modification_scope(heap);
+    {
+      HandleScope handle_scope(i_isolate);
+      i::byte buffer[i::Assembler::kDefaultBufferSize];
+      MacroAssembler masm(i_isolate, v8::internal::CodeObjectRequired::kYes,
+                          ExternalAssemblerBuffer(buffer, sizeof(buffer)));
 #if V8_TARGET_ARCH_ARM64
-    // Arm64 requires stack alignment.
-    UseScratchRegisterScope temps(&masm);
-    Register tmp = temps.AcquireX();
-    masm.Mov(tmp, Operand(ReadOnlyRoots(heap).undefined_value_handle()));
-    masm.Push(tmp, padreg);
+      // Arm64 requires stack alignment.
+      UseScratchRegisterScope temps(&masm);
+      Register tmp = temps.AcquireX();
+      masm.Mov(tmp, Operand(ReadOnlyRoots(heap).undefined_value_handle()));
+      masm.Push(tmp, padreg);
 #else
-    masm.Push(ReadOnlyRoots(heap).undefined_value_handle());
+      masm.Push(ReadOnlyRoots(heap).undefined_value_handle());
 #endif
-    CodeDesc desc;
-    masm.GetCode(i_isolate, &desc);
-    Handle<Code> code_handle =
-        Factory::CodeBuilder(i_isolate, desc, CodeKind::FOR_TESTING).Build();
-    heap::AbandonCurrentlyFreeMemory(heap->old_space());
-    Handle<HeapNumber> value_handle(
-        i_isolate->factory()->NewHeapNumber<AllocationType::kOld>(1.1));
-    heap::ForceEvacuationCandidate(Page::FromHeapObject(*value_handle));
-    code = *code_handle;
-    value = *value_handle;
+      CodeDesc desc;
+      masm.GetCode(i_isolate, &desc);
+      Handle<Code> code_handle =
+          Factory::CodeBuilder(i_isolate, desc, CodeKind::FOR_TESTING).Build();
+      heap::AbandonCurrentlyFreeMemory(heap->old_space());
+      Handle<HeapNumber> value_handle(
+          i_isolate->factory()->NewHeapNumber<AllocationType::kOld>(1.1));
+      heap::ForceEvacuationCandidate(Page::FromHeapObject(*value_handle));
+      code = *code_handle;
+      value = *value_handle;
+    }
+    heap->StartIncrementalMarking(i::Heap::kNoGCFlags,
+                                  i::GarbageCollectionReason::kTesting);
+    CHECK(heap->incremental_marking()->marking_state()->IsWhite(value));
+
+    {
+      auto thread =
+          std::make_unique<ConcurrentRecordRelocSlotThread>(heap, code, value);
+      CHECK(thread->Start());
+
+      thread->Join();
+    }
+
+    CHECK(heap->incremental_marking()->marking_state()->IsBlackOrGrey(value));
+    heap::InvokeMarkSweep(i_isolate);
   }
-  heap->StartIncrementalMarking(i::Heap::kNoGCFlags,
-                                i::GarbageCollectionReason::kTesting);
-  CHECK(heap->incremental_marking()->marking_state()->IsWhite(value));
-
-  {
-    CodeSpaceMemoryModificationScope modification_scope(heap);
-    auto thread =
-        std::make_unique<ConcurrentRecordRelocSlotThread>(heap, code, value);
-    CHECK(thread->Start());
-
-    thread->Join();
-  }
-
-  CHECK(heap->incremental_marking()->marking_state()->IsBlackOrGrey(value));
-  heap::InvokeMarkSweep(i_isolate);
-
   isolate->Dispose();
 }
 

test/cctest/heap/test-heap.cc

@@ -224,12 +224,7 @@ HEAP_TEST(TestNewSpaceRefsInCopiedCode) {
   masm.GetCode(isolate, &desc);
   Handle<Code> code =
       Factory::CodeBuilder(isolate, desc, CodeKind::FOR_TESTING).Build();
-
-  Handle<Code> copy;
-  {
-    CodeSpaceMemoryModificationScope modification_scope(isolate->heap());
-    copy = factory->CopyCode(code);
-  }
+  Handle<Code> copy = factory->CopyCode(code);
 
   CheckEmbeddedObjectsAreEqual(isolate, code, copy);
   CcTest::CollectAllAvailableGarbage();
@@ -7289,9 +7284,10 @@ TEST(Regress10900) {
   Handle<Code> code =
       Factory::CodeBuilder(isolate, desc, CodeKind::FOR_TESTING).Build();
   {
-    // Generate multiple code pages.
-    CodeSpaceMemoryModificationScope modification_scope(isolate->heap());
     for (int i = 0; i < 100; i++) {
+      // Generate multiple code pages.
+      CodePageCollectionMemoryModificationScope modification_scope(
+          isolate->heap());
       factory->CopyCode(code);
     }
   }