[vm] Adjust concurrent marker to re-enable code W^X.
Take 2: account for encountering Instructions in the write barrier.
Bug: https://github.com/dart-lang/sdk/issues/34002
Change-Id: I31a8fc8965042678a861a05b09070d52fa07e4a5
Reviewed-on: https://dart-review.googlesource.com/c/87082
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
diff --git a/runtime/vm/code_patcher.cc b/runtime/vm/code_patcher.cc
index f0ec1db..3546b60 100644
--- a/runtime/vm/code_patcher.cc
+++ b/runtime/vm/code_patcher.cc
@@ -10,7 +10,7 @@
namespace dart {
-DEFINE_FLAG(bool, write_protect_code, false, "Write protect jitted code");
+DEFINE_FLAG(bool, write_protect_code, true, "Write protect jitted code");
WritableInstructionsScope::WritableInstructionsScope(uword address,
intptr_t size)
diff --git a/runtime/vm/dart.cc b/runtime/vm/dart.cc
index 3ea0f09..6f64118 100644
--- a/runtime/vm/dart.cc
+++ b/runtime/vm/dart.cc
@@ -101,7 +101,7 @@
// These offsets are embedded in precompiled instructions. We need simarm64
// (compiler) and arm64 (runtime) to agree.
CHECK_OFFSET(Thread::stack_limit_offset(), 8);
- CHECK_OFFSET(Thread::object_null_offset(), 112);
+ CHECK_OFFSET(Thread::object_null_offset(), 120);
CHECK_OFFSET(SingleTargetCache::upper_limit_offset(), 26);
CHECK_OFFSET(Isolate::object_store_offset(), 40);
NOT_IN_PRODUCT(CHECK_OFFSET(sizeof(ClassHeapStats), 288));
diff --git a/runtime/vm/heap/marker.cc b/runtime/vm/heap/marker.cc
index 3145090..1dcd875 100644
--- a/runtime/vm/heap/marker.cc
+++ b/runtime/vm/heap/marker.cc
@@ -172,6 +172,7 @@
MarkingVisitorBase(Isolate* isolate,
PageSpace* page_space,
MarkingStack* marking_stack,
+ MarkingStack* deferred_marking_stack,
SkippedCodeFunctions* skipped_code_functions)
: ObjectPointerVisitor(isolate),
thread_(Thread::Current()),
@@ -182,6 +183,7 @@
#endif // !PRODUCT
page_space_(page_space),
work_list_(marking_stack),
+ deferred_work_list_(deferred_marking_stack),
delayed_weak_properties_(NULL),
skipped_code_functions_(skipped_code_functions),
marked_bytes_(0),
@@ -260,8 +262,7 @@
if (class_id != kWeakPropertyCid) {
size = raw_obj->VisitPointersNonvirtual(this);
} else {
- RawWeakProperty* raw_weak =
- reinterpret_cast<RawWeakProperty*>(raw_obj);
+ RawWeakProperty* raw_weak = static_cast<RawWeakProperty*>(raw_obj);
size = ProcessWeakProperty(raw_weak);
}
marked_bytes_ += size;
@@ -315,6 +316,20 @@
return raw_weak->VisitPointersNonvirtual(this);
}
+ void FinalizeInstructions() {
+ RawObject* raw_obj;
+ while ((raw_obj = deferred_work_list_.Pop()) != NULL) {
+ ASSERT(raw_obj->IsInstructions());
+ RawInstructions* instr = static_cast<RawInstructions*>(raw_obj);
+ if (TryAcquireMarkBit(instr)) {
+ intptr_t size = instr->Size();
+ marked_bytes_ += size;
+ NOT_IN_PRODUCT(UpdateLiveOld(kInstructionsCid, size));
+ }
+ }
+ deferred_work_list_.Finalize();
+ }
+
// Called when all marking is complete.
void Finalize() {
work_list_.Finalize();
@@ -337,7 +352,10 @@
}
}
- void AbandonWork() { work_list_.AbandonWork(); }
+ void AbandonWork() {
+ work_list_.AbandonWork();
+ deferred_work_list_.AbandonWork();
+ }
private:
void PushMarked(RawObject* raw_obj) {
@@ -370,7 +388,15 @@
return;
}
- ASSERT(raw_obj->GetClassId() != kFreeListElement);
+ intptr_t class_id = raw_obj->GetClassId();
+ ASSERT(class_id != kFreeListElement);
+
+ if (sync && UNLIKELY(class_id == kInstructionsCid)) {
+ // If this is the concurrent marker, instruction pages may be
+ // non-writable.
+ deferred_work_list_.Push(raw_obj);
+ return;
+ }
if (!TryAcquireMarkBit(raw_obj)) {
// Already marked.
@@ -396,6 +422,7 @@
#endif // !PRODUCT
PageSpace* page_space_;
MarkerWorkList work_list_;
+ MarkerWorkList deferred_work_list_;
RawWeakProperty* delayed_weak_properties_;
SkippedCodeFunctions* skipped_code_functions_;
uintptr_t marked_bytes_;
@@ -636,6 +663,8 @@
barrier_->Sync();
} while (more_to_mark);
+ visitor_->FinalizeInstructions();
+
// Phase 2: Weak processing and follow-up marking on main thread.
barrier_->Sync();
@@ -815,7 +844,7 @@
}
void GCMarker::StartConcurrentMark(PageSpace* page_space, bool collect_code) {
- isolate_->EnableIncrementalBarrier(&marking_stack_);
+ isolate_->EnableIncrementalBarrier(&marking_stack_, &deferred_marking_stack_);
const intptr_t num_tasks = FLAG_marker_tasks;
@@ -838,6 +867,7 @@
SkippedCodeFunctions* skipped_code_functions =
collect_code ? new SkippedCodeFunctions() : NULL;
visitors_[i] = new SyncMarkingVisitor(isolate_, page_space, &marking_stack_,
+ &deferred_marking_stack_,
skipped_code_functions);
// Begin marking on a helper thread.
@@ -870,9 +900,11 @@
SkippedCodeFunctions* skipped_code_functions =
collect_code ? new SkippedCodeFunctions() : NULL;
UnsyncMarkingVisitor mark(isolate_, page_space, &marking_stack_,
+ &deferred_marking_stack_,
skipped_code_functions);
IterateRoots(&mark, 0, 1);
mark.DrainMarkingStack();
+ mark.FinalizeInstructions();
{
TIMELINE_FUNCTION_GC_DURATION(thread, "ProcessWeakHandles");
MarkingWeakVisitor mark_weak(thread);
@@ -897,7 +929,8 @@
SkippedCodeFunctions* skipped_code_functions =
collect_code ? new SkippedCodeFunctions() : NULL;
visitor = new SyncMarkingVisitor(
- isolate_, page_space, &marking_stack_, skipped_code_functions);
+ isolate_, page_space, &marking_stack_, &deferred_marking_stack_,
+ skipped_code_functions);
}
MarkTask* mark_task =
diff --git a/runtime/vm/heap/marker.h b/runtime/vm/heap/marker.h
index 10fde88..a4424cf 100644
--- a/runtime/vm/heap/marker.h
+++ b/runtime/vm/heap/marker.h
@@ -63,6 +63,7 @@
Isolate* const isolate_;
Heap* const heap_;
MarkingStack marking_stack_;
+ MarkingStack deferred_marking_stack_;
MarkingVisitorBase<true>** visitors_;
Mutex stats_mutex_;
diff --git a/runtime/vm/heap/pages.cc b/runtime/vm/heap/pages.cc
index 087e1a0..b6335d5 100644
--- a/runtime/vm/heap/pages.cc
+++ b/runtime/vm/heap/pages.cc
@@ -1007,7 +1007,6 @@
#else
if (!FLAG_concurrent_mark) return; // Disabled.
if (FLAG_marker_tasks == 0) return; // Disabled.
- if (FLAG_write_protect_code) return; // Not implemented.
#endif
}
@@ -1081,7 +1080,7 @@
}
// Make code pages writable.
- WriteProtectCode(false);
+ if (finalize) WriteProtectCode(false);
// Save old value before GCMarker visits the weak persistent handles.
SpaceUsage usage_before = GetCurrentUsage();
@@ -1186,7 +1185,7 @@
}
// Make code pages read-only.
- WriteProtectCode(true);
+ if (finalize) WriteProtectCode(true);
int64_t end = OS::GetCurrentMonotonicMicros();
diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc
index f3a9979..5b69675 100644
--- a/runtime/vm/isolate.cc
+++ b/runtime/vm/isolate.cc
@@ -2046,9 +2046,11 @@
thread_registry()->ReleaseStoreBuffers();
}
-void Isolate::EnableIncrementalBarrier(MarkingStack* marking_stack) {
+void Isolate::EnableIncrementalBarrier(MarkingStack* marking_stack,
+ MarkingStack* deferred_marking_stack) {
ASSERT(marking_stack_ == NULL);
marking_stack_ = marking_stack;
+ deferred_marking_stack_ = deferred_marking_stack;
thread_registry()->AcquireMarkingStacks();
ASSERT(Thread::Current()->is_marking());
}
@@ -2057,6 +2059,7 @@
thread_registry()->ReleaseMarkingStacks();
ASSERT(marking_stack_ != NULL);
marking_stack_ = NULL;
+ deferred_marking_stack_ = NULL;
ASSERT(!Thread::Current()->is_marking());
}
diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h
index c07bf00..55b4bad 100644
--- a/runtime/vm/isolate.h
+++ b/runtime/vm/isolate.h
@@ -196,11 +196,15 @@
// Prepares all threads in an isolate for Garbage Collection.
void ReleaseStoreBuffers();
- void EnableIncrementalBarrier(MarkingStack* marking_stack);
+ void EnableIncrementalBarrier(MarkingStack* marking_stack,
+ MarkingStack* deferred_marking_stack);
void DisableIncrementalBarrier();
StoreBuffer* store_buffer() const { return store_buffer_; }
MarkingStack* marking_stack() const { return marking_stack_; }
+ MarkingStack* deferred_marking_stack() const {
+ return deferred_marking_stack_;
+ }
ThreadRegistry* thread_registry() const { return thread_registry_; }
SafepointHandler* safepoint_handler() const { return safepoint_handler_; }
@@ -860,6 +864,7 @@
StoreBuffer* store_buffer_;
MarkingStack* marking_stack_;
+ MarkingStack* deferred_marking_stack_;
Heap* heap_;
#define ISOLATE_FLAG_BITS(V) \
diff --git a/runtime/vm/raw_object.h b/runtime/vm/raw_object.h
index 93cde94..891d7d9 100644
--- a/runtime/vm/raw_object.h
+++ b/runtime/vm/raw_object.h
@@ -747,6 +747,13 @@
// Incremental barrier: record when a store creates an
// old -> old-and-not-marked reference.
ASSERT(value->IsOldObject());
+#if !defined(TARGET_ARCH_IA32)
+ if (ClassIdTag::decode(target_tags) == kInstructionsCid) {
+ // Instruction pages may be non-writable. Defer marking.
+ thread->DeferredMarkingStackAddObject(value);
+ return;
+ }
+#endif
if (value->TryAcquireMarkBit()) {
thread->MarkingStackAddObject(value);
}
@@ -786,12 +793,19 @@
RememberCard(reinterpret_cast<RawObject* const*>(addr));
} else {
this->SetRememberedBit();
- Thread::Current()->StoreBufferAddObject(this);
+ thread->StoreBufferAddObject(this);
}
} else {
// Incremental barrier: record when a store creates an
// old -> old-and-not-marked reference.
ASSERT(value->IsOldObject());
+#if !defined(TARGET_ARCH_IA32)
+ if (ClassIdTag::decode(target_tags) == kInstructionsCid) {
+ // Instruction pages may be non-writable. Defer marking.
+ thread->DeferredMarkingStackAddObject(value);
+ return;
+ }
+#endif
if (value->TryAcquireMarkBit()) {
thread->MarkingStackAddObject(value);
}
diff --git a/runtime/vm/thread.cc b/runtime/vm/thread.cc
index 30c0f51..b7a8f14 100644
--- a/runtime/vm/thread.cc
+++ b/runtime/vm/thread.cc
@@ -334,6 +334,7 @@
if (isolate->marking_stack() != NULL) {
// Concurrent mark in progress. Enable barrier for this thread.
thread->MarkingStackAcquire();
+ thread->DeferredMarkingStackAcquire();
}
return true;
}
@@ -352,6 +353,7 @@
thread->ClearReusableHandles();
if (thread->is_marking()) {
thread->MarkingStackRelease();
+ thread->DeferredMarkingStackRelease();
}
thread->StoreBufferRelease();
if (isolate->is_runnable()) {
@@ -379,6 +381,7 @@
if (isolate->marking_stack() != NULL) {
// Concurrent mark in progress. Enable barrier for this thread.
thread->MarkingStackAcquire();
+ thread->DeferredMarkingStackAcquire();
}
// This thread should not be the main mutator.
thread->task_kind_ = kind;
@@ -398,6 +401,7 @@
thread->ClearReusableHandles();
if (thread->is_marking()) {
thread->MarkingStackRelease();
+ thread->DeferredMarkingStackRelease();
}
thread->StoreBufferRelease();
Isolate* isolate = thread->isolate();
@@ -601,6 +605,11 @@
MarkingStackAcquire();
}
+void Thread::DeferredMarkingStackBlockProcess() {
+ DeferredMarkingStackRelease();
+ DeferredMarkingStackAcquire();
+}
+
void Thread::MarkingStackAddObject(RawObject* obj) {
marking_stack_block_->Push(obj);
if (marking_stack_block_->IsFull()) {
@@ -608,6 +617,13 @@
}
}
+void Thread::DeferredMarkingStackAddObject(RawObject* obj) {
+ deferred_marking_stack_block_->Push(obj);
+ if (deferred_marking_stack_block_->IsFull()) {
+ DeferredMarkingStackBlockProcess();
+ }
+}
+
void Thread::MarkingStackRelease() {
MarkingStackBlock* block = marking_stack_block_;
marking_stack_block_ = NULL;
@@ -621,6 +637,17 @@
RawObject::kGenerationalBarrierMask | RawObject::kIncrementalBarrierMask;
}
+void Thread::DeferredMarkingStackRelease() {
+ MarkingStackBlock* block = deferred_marking_stack_block_;
+ deferred_marking_stack_block_ = NULL;
+ isolate()->deferred_marking_stack()->PushBlock(block);
+}
+
+void Thread::DeferredMarkingStackAcquire() {
+ deferred_marking_stack_block_ =
+ isolate()->deferred_marking_stack()->PopEmptyBlock();
+}
+
bool Thread::IsMutatorThread() const {
return ((isolate_ != NULL) && (isolate_->mutator_thread() == this));
}
diff --git a/runtime/vm/thread.h b/runtime/vm/thread.h
index bb6fc9f..514d4d1 100644
--- a/runtime/vm/thread.h
+++ b/runtime/vm/thread.h
@@ -437,7 +437,9 @@
bool is_marking() const { return marking_stack_block_ != NULL; }
void MarkingStackAddObject(RawObject* obj);
+ void DeferredMarkingStackAddObject(RawObject* obj);
void MarkingStackBlockProcess();
+ void DeferredMarkingStackBlockProcess();
static intptr_t marking_stack_block_offset() {
return OFFSET_OF(Thread, marking_stack_block_);
}
@@ -818,6 +820,7 @@
uword top_exit_frame_info_;
StoreBufferBlock* store_buffer_block_;
MarkingStackBlock* marking_stack_block_;
+ MarkingStackBlock* deferred_marking_stack_block_;
uword vm_tag_;
RawStackTrace* async_stack_trace_;
// Memory location dedicated for passing unboxed int64 values from
@@ -926,6 +929,8 @@
void MarkingStackRelease();
void MarkingStackAcquire();
+ void DeferredMarkingStackRelease();
+ void DeferredMarkingStackAcquire();
void set_zone(Zone* zone) { zone_ = zone; }
diff --git a/runtime/vm/thread_registry.cc b/runtime/vm/thread_registry.cc
index bb02973..a390528 100644
--- a/runtime/vm/thread_registry.cc
+++ b/runtime/vm/thread_registry.cc
@@ -99,6 +99,7 @@
while (thread != NULL) {
if (!thread->BypassSafepoints()) {
thread->MarkingStackAcquire();
+ thread->DeferredMarkingStackAcquire();
}
thread = thread->next_;
}
@@ -110,6 +111,7 @@
while (thread != NULL) {
if (!thread->BypassSafepoints()) {
thread->MarkingStackRelease();
+ thread->DeferredMarkingStackRelease();
}
thread = thread->next_;
}
