[vm/concurrency] Introduce program_lock to guard program structure changes.
This CL adds acquisiton of write lock and check for whether write lock is held when updating class functions, primarily populated during class finalization. Read locks will be added in successive CLs, so is extending of the locks coverage to all aspects of program structure changes.
Bug: https://github.com/dart-lang/sdk/issues/36097
Change-Id: I3dba6bc23db4e45599a20717226210f12e7fd2b3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/168140
Commit-Queue: Alexander Aprelev <aam@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
diff --git a/runtime/vm/bootstrap.cc b/runtime/vm/bootstrap.cc
index 774a398..f0c2d76 100644
--- a/runtime/vm/bootstrap.cc
+++ b/runtime/vm/bootstrap.cc
@@ -51,7 +51,7 @@
ObjectStore* object_store = thread->isolate()->object_store();
Zone* zone = thread->zone();
Class& cls = Class::Handle(zone, object_store->closure_class());
- ClassFinalizer::LoadClassMembers(cls);
+ cls.EnsureIsFinalized(thread);
// Make sure _Closure fields are not marked as unboxing candidates
// as they are accessed with plain loads.
@@ -88,7 +88,7 @@
// Eagerly compile Bool class, bool constants are used from within compiler.
cls = object_store->bool_class();
- ClassFinalizer::LoadClassMembers(cls);
+ cls.EnsureIsFinalized(thread);
}
static ErrorPtr BootstrapFromKernel(Thread* thread,
diff --git a/runtime/vm/class_finalizer_test.cc b/runtime/vm/class_finalizer_test.cc
index 29a708a..7f91581 100644
--- a/runtime/vm/class_finalizer_test.cc
+++ b/runtime/vm/class_finalizer_test.cc
@@ -10,13 +10,14 @@
namespace dart {
static ClassPtr CreateTestClass(const char* name) {
- const String& class_name =
- String::Handle(Symbols::New(Thread::Current(), name));
+ Thread* thread = Thread::Current();
+ const String& class_name = String::Handle(Symbols::New(thread, name));
const Script& script = Script::Handle();
const Class& cls = Class::Handle(Class::New(
Library::Handle(), class_name, script, TokenPosition::kNoSource));
cls.set_interfaces(Object::empty_array());
cls.set_is_declaration_loaded();
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
cls.SetFunctions(Object::empty_array());
cls.SetFields(Object::empty_array());
return cls.raw();
diff --git a/runtime/vm/compiler/aot/precompiler.cc b/runtime/vm/compiler/aot/precompiler.cc
index 62c6dd0..41933d8 100644
--- a/runtime/vm/compiler/aot/precompiler.cc
+++ b/runtime/vm/compiler/aot/precompiler.cc
@@ -1724,6 +1724,7 @@
}
};
+ SafepointWriteRwLocker ml(T, T->isolate_group()->program_lock());
auto& dispatchers_array = Array::Handle(Z);
auto& name = String::Handle(Z);
auto& desc = Array::Handle(Z);
diff --git a/runtime/vm/compiler/frontend/bytecode_reader.cc b/runtime/vm/compiler/frontend/bytecode_reader.cc
index 2ddd261..ef3a137 100644
--- a/runtime/vm/compiler/frontend/bytecode_reader.cc
+++ b/runtime/vm/compiler/frontend/bytecode_reader.cc
@@ -2472,7 +2472,11 @@
functions_->SetAt(function_index_++, function);
}
- cls.SetFunctions(*functions_);
+ {
+ Thread* thread = Thread::Current();
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
+ cls.SetFunctions(*functions_);
+ }
if (cls.IsTopLevel()) {
const Library& library = Library::Handle(Z, cls.library());
diff --git a/runtime/vm/debugger.cc b/runtime/vm/debugger.cc
index cd1a70c..8ad542d 100644
--- a/runtime/vm/debugger.cc
+++ b/runtime/vm/debugger.cc
@@ -1919,7 +1919,8 @@
// Iterate over all classes, deoptimize functions.
// TODO(hausner): Could possibly be combined with RemoveOptimizedCode()
const ClassTable& class_table = *isolate_->class_table();
- Zone* zone = Thread::Current()->zone();
+ Thread* thread = Thread::Current();
+ Zone* zone = thread->zone();
CallSiteResetter resetter(zone);
Class& cls = Class::Handle(zone);
Array& functions = Array::Handle(zone);
@@ -1929,6 +1930,9 @@
const intptr_t num_classes = class_table.NumCids();
const intptr_t num_tlc_classes = class_table.NumTopLevelCids();
+ // TODO(dartbug.com/36097): Need to stop other mutators running in same IG
+ // before deoptimizing the world.
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
for (intptr_t i = 1; i < num_classes + num_tlc_classes; i++) {
const classid_t cid =
i < num_classes ? i : ClassTable::CidFromTopLevelIndex(i - num_classes);
diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc
index 5aa297b..3f7e002 100644
--- a/runtime/vm/isolate.cc
+++ b/runtime/vm/isolate.cc
@@ -374,6 +374,7 @@
NOT_IN_PRODUCT("IsolateGroup::type_canonicalization_mutex_")),
type_arguments_canonicalization_mutex_(NOT_IN_PRODUCT(
"IsolateGroup::type_arguments_canonicalization_mutex_")),
+ program_lock_(new SafepointRwLock()),
active_mutators_monitor_(new Monitor()),
max_active_mutators_(Scavenger::MaxMutatorThreadCount()) {
const bool is_vm_isolate = Dart::VmIsolateNameEquals(source_->name);
diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h
index 51d50d91..68c72ed 100644
--- a/runtime/vm/isolate.h
+++ b/runtime/vm/isolate.h
@@ -435,6 +435,8 @@
Mutex* initializer_functions_mutex() { return &initializer_functions_mutex_; }
#endif // !defined(DART_PRECOMPILED_RUNTIME)
+ SafepointRwLock* program_lock() { return program_lock_.get(); }
+
static inline IsolateGroup* Current() {
Thread* thread = Thread::Current();
return thread == nullptr ? nullptr : thread->isolate_group();
@@ -725,6 +727,11 @@
Mutex initializer_functions_mutex_;
#endif // !defined(DART_PRECOMPILED_RUNTIME)
+ // Ensures synchronized access to classes functions, fields and other
+ // program structure elements to accommodate concurrent modification done
+ // by multiple isolates and background compiler.
+ std::unique_ptr<SafepointRwLock> program_lock_;
+
// Allow us to ensure the number of active mutators is limited by a maximum.
std::unique_ptr<Monitor> active_mutators_monitor_;
intptr_t active_mutators_ = 0;
diff --git a/runtime/vm/kernel_loader.cc b/runtime/vm/kernel_loader.cc
index 567f4e1..7a7f4f6 100644
--- a/runtime/vm/kernel_loader.cc
+++ b/runtime/vm/kernel_loader.cc
@@ -734,6 +734,7 @@
}
ObjectPtr KernelLoader::LoadProgram(bool process_pending_classes) {
+ SafepointWriteRwLocker ml(thread_, thread_->isolate_group()->program_lock());
ASSERT(kernel_program_info_.constants() == Array::null());
if (!program_->is_single_program()) {
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index b45e2f1..ba6f92c 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -1116,7 +1116,11 @@
cls = type_arguments_class_;
cls.set_interfaces(Object::empty_array());
cls.SetFields(Object::empty_array());
- cls.SetFunctions(Object::empty_array());
+ {
+ Thread* thread = Thread::Current();
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
+ cls.SetFunctions(Object::empty_array());
+ }
cls = Class::New<Bool, RTN::Bool>(isolate);
isolate->object_store()->set_bool_class(cls);
@@ -1654,6 +1658,7 @@
// This will initialize isolate group object_store, shared by all isolates
// running in the isolate group.
ObjectStore* object_store = isolate->object_store();
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
Class& cls = Class::Handle(zone);
Type& type = Type::Handle(zone);
@@ -3000,7 +3005,11 @@
typedef UnorderedHashSet<ClassFunctionsTraits> ClassFunctionsSet;
void Class::SetFunctions(const Array& value) const {
- ASSERT(Thread::Current()->IsMutatorThread());
+#if defined(DEBUG)
+ Thread* thread = Thread::Current();
+ ASSERT(thread->IsMutatorThread());
+ ASSERT(thread->isolate_group()->program_lock()->IsCurrentThreadWriter());
+#endif
ASSERT(!value.IsNull());
set_functions(value);
const intptr_t len = value.Length();
@@ -3020,7 +3029,11 @@
}
void Class::AddFunction(const Function& function) const {
- ASSERT(Thread::Current()->IsMutatorThread());
+#if defined(DEBUG)
+ Thread* thread = Thread::Current();
+ ASSERT(thread->IsMutatorThread());
+ ASSERT(thread->isolate_group()->program_lock()->IsCurrentThreadWriter());
+#endif
const Array& arr = Array::Handle(functions());
const Array& new_array =
Array::Handle(Array::Grow(arr, arr.Length() + 1, Heap::kOld));
@@ -3616,13 +3629,19 @@
const Function& closure_function =
Function::Handle(ImplicitClosureFunction());
const Class& owner = Class::Handle(closure_function.Owner());
- if (owner.EnsureIsFinalized(Thread::Current()) != Error::null()) {
+ Thread* thread = Thread::Current();
+ if (owner.EnsureIsFinalized(thread) != Error::null()) {
return Function::null();
}
+ IsolateGroup* group = thread->isolate_group();
Function& result =
Function::Handle(owner.LookupDynamicFunctionUnsafe(getter_name));
if (result.IsNull()) {
- result = CreateMethodExtractor(getter_name);
+ SafepointWriteRwLocker ml(thread, group->program_lock());
+ result = owner.LookupDynamicFunctionUnsafe(getter_name);
+ if (result.IsNull()) {
+ result = CreateMethodExtractor(getter_name);
+ }
}
ASSERT(result.kind() == FunctionLayout::kMethodExtractor);
return result.raw();
@@ -4288,6 +4307,10 @@
Compiler::AbortBackgroundCompilation(DeoptId::kNone,
"Class finalization while compiling");
}
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
+ if (is_finalized()) {
+ return Error::null();
+ }
ASSERT(thread->IsMutatorThread());
ASSERT(thread != NULL);
const Error& error =
@@ -13126,7 +13149,8 @@
const String& klass,
const Array& arguments,
const TypeArguments& type_arguments) {
- Zone* zone = Thread::Current()->zone();
+ Thread* thread = Thread::Current();
+ Zone* zone = thread->zone();
#if defined(DART_PRECOMPILED_RUNTIME)
const String& error_str = String::Handle(
zone,
diff --git a/runtime/vm/object_test.cc b/runtime/vm/object_test.cc
index d46aae1..a9c2fcb 100644
--- a/runtime/vm/object_test.cc
+++ b/runtime/vm/object_test.cc
@@ -121,7 +121,10 @@
functions.SetAt(5, function);
// Setup the functions in the class.
- cls.SetFunctions(functions);
+ {
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
+ cls.SetFunctions(functions);
+ }
// The class can now be finalized.
cls.Finalize();
@@ -200,7 +203,11 @@
}
cls.set_interfaces(Array::empty_array());
- cls.SetFunctions(Array::empty_array());
+ {
+ SafepointWriteRwLocker ml(thread,
+ thread->isolate_group()->program_lock());
+ cls.SetFunctions(Array::empty_array());
+ }
cls.SetFields(fields);
cls.Finalize();
@@ -2516,7 +2523,10 @@
Function::New(parent_name, FunctionLayout::kRegularFunction, false, false,
false, false, false, cls, TokenPosition::kMinSource);
functions.SetAt(0, parent);
- cls.SetFunctions(functions);
+ {
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
+ cls.SetFunctions(functions);
+ }
Function& function = Function::Handle();
const String& function_name = String::Handle(Symbols::New(thread, "foo"));
@@ -3766,7 +3776,10 @@
Function::New(parent_name, FunctionLayout::kRegularFunction, false, false,
false, false, false, cls, TokenPosition::kMinSource);
functions.SetAt(0, parent);
- cls.SetFunctions(functions);
+ {
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
+ cls.SetFunctions(functions);
+ }
Function& function = Function::Handle();
const String& function_name = String::Handle(Symbols::New(thread, "foo"));
@@ -3802,7 +3815,10 @@
Function::New(parent_name, FunctionLayout::kRegularFunction, false, false,
false, false, false, cls, TokenPosition::kMinSource);
functions.SetAt(0, parent);
- cls.SetFunctions(functions);
+ {
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
+ cls.SetFunctions(functions);
+ }
cls.Finalize();
// Add invocation dispatcher.
diff --git a/runtime/vm/resolver.cc b/runtime/vm/resolver.cc
index 57b2051..cdf4fac 100644
--- a/runtime/vm/resolver.cc
+++ b/runtime/vm/resolver.cc
@@ -57,6 +57,7 @@
const bool is_dyn_call = demangled.raw() != function_name.raw();
+ bool need_to_create_method_extractor = false;
while (!cls.IsNull()) {
if (is_dyn_call) {
// Try to find a dyn:* forwarder & return it.
@@ -82,12 +83,8 @@
function = lookup(cls, demangled_getter_name);
if (!function.IsNull()) {
if (allow_add && FLAG_lazy_dispatchers) {
- // We were looking for the getter but found a method with the same
- // name. Create a method extractor and return it.
- // The extractor does not exist yet, so using GetMethodExtractor is
- // not necessary here.
- function = function.CreateMethodExtractor(demangled);
- return function.raw();
+ need_to_create_method_extractor = true;
+ break;
} else {
return Function::null();
}
@@ -95,6 +92,13 @@
}
cls = cls.SuperClass();
}
+ if (need_to_create_method_extractor) {
+ // We were looking for the getter but found a method with the same
+ // name. Create a method extractor and return it.
+ // Use GetMethodExtractor instead of CreateMethodExtractor to ensure
+ // nobody created method extractor since we last checked under ReadRwLocker.
+ function = function.GetMethodExtractor(demangled);
+ }
return function.raw();
}
diff --git a/runtime/vm/runtime_entry.cc b/runtime/vm/runtime_entry.cc
index 3435afe..601befe 100644
--- a/runtime/vm/runtime_entry.cc
+++ b/runtime/vm/runtime_entry.cc
@@ -109,27 +109,6 @@
}
#endif
-// Add function to a class and that class to the class dictionary so that
-// frame walking can be used.
-const Function& RegisterFakeFunction(const char* name, const Code& code) {
- Thread* thread = Thread::Current();
- const String& class_name = String::Handle(Symbols::New(thread, "ownerClass"));
- const Script& script = Script::Handle();
- const Library& lib = Library::Handle(Library::CoreLibrary());
- const Class& owner_class = Class::Handle(
- Class::New(lib, class_name, script, TokenPosition::kNoSource));
- const String& function_name = String::ZoneHandle(Symbols::New(thread, name));
- const Function& function = Function::ZoneHandle(Function::New(
- function_name, FunctionLayout::kRegularFunction, true, false, false,
- false, false, owner_class, TokenPosition::kMinSource));
- const Array& functions = Array::Handle(Array::New(1));
- functions.SetAt(0, function);
- owner_class.SetFunctions(functions);
- lib.AddClass(owner_class);
- function.AttachCode(code);
- return function;
-}
-
DEFINE_RUNTIME_ENTRY(RangeError, 2) {
const Instance& length = Instance::CheckedHandle(zone, arguments.ArgAt(0));
const Instance& index = Instance::CheckedHandle(zone, arguments.ArgAt(1));
@@ -1776,10 +1755,10 @@
const String& name,
const Array& descriptor) {
ASSERT(name.IsSymbol());
-
- ArgumentsDescriptor args_desc(descriptor);
Function& target_function = Function::Handle(zone);
+
if (receiver_class.EnsureIsFinalized(thread) == Error::null()) {
+ ArgumentsDescriptor args_desc(descriptor);
target_function = Resolver::ResolveDynamicForReceiverClass(receiver_class,
name, args_desc);
}
diff --git a/runtime/vm/stub_code_test.cc b/runtime/vm/stub_code_test.cc
new file mode 100644
index 0000000..4b8649b
--- /dev/null
+++ b/runtime/vm/stub_code_test.cc
@@ -0,0 +1,35 @@
+// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+#include "vm/globals.h"
+#include "vm/object.h"
+#include "vm/symbols.h"
+
+namespace dart {
+
+// Add function to a class and that class to the class dictionary so that
+// frame walking can be used.
+const Function& RegisterFakeFunction(const char* name, const Code& code) {
+ Thread* thread = Thread::Current();
+ const String& class_name = String::Handle(Symbols::New(thread, "ownerClass"));
+ const Script& script = Script::Handle();
+ const Library& lib = Library::Handle(Library::CoreLibrary());
+ const Class& owner_class = Class::Handle(
+ Class::New(lib, class_name, script, TokenPosition::kNoSource));
+ const String& function_name = String::ZoneHandle(Symbols::New(thread, name));
+ const Function& function = Function::ZoneHandle(Function::New(
+ function_name, FunctionLayout::kRegularFunction, true, false, false,
+ false, false, owner_class, TokenPosition::kMinSource));
+ const Array& functions = Array::Handle(Array::New(1));
+ functions.SetAt(0, function);
+ {
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
+ owner_class.SetFunctions(functions);
+ }
+ lib.AddClass(owner_class);
+ function.AttachCode(code);
+ return function;
+}
+
+} // namespace dart
diff --git a/runtime/vm/vm_sources.gni b/runtime/vm/vm_sources.gni
index f70a389..83569d7 100644
--- a/runtime/vm/vm_sources.gni
+++ b/runtime/vm/vm_sources.gni
@@ -436,6 +436,7 @@
"snapshot_test.cc",
"source_report_test.cc",
"stack_frame_test.cc",
+ "stub_code_test.cc",
"stub_code_arm64_test.cc",
"stub_code_arm_test.cc",
"stub_code_ia32_test.cc",
