[vm/concurrency] Introduce program_lock to guard program structure changes.

aam · commit-bot@chromium.org · commit 37abbb2e3d14 · 2020-10-22T13:08:10.000Z
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: dart-lang/sdk#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
@@ -51,7 +51,7 @@ static void Finish(Thread* thread) {
   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 @@ static void Finish(Thread* thread) {
 
   // 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
@@ -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
@@ -1724,6 +1724,7 @@ void Precompiler::DropFunctions() {
     }
   };
 
+  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
@@ -2472,7 +2472,11 @@ void BytecodeReaderHelper::ReadFunctionDeclarations(const Class& cls) {
     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
@@ -1919,7 +1919,8 @@ void Debugger::DeoptimizeWorld() {
   // 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 @@ void Debugger::DeoptimizeWorld() {
 
   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
@@ -374,6 +374,7 @@ IsolateGroup::IsolateGroup(std::shared_ptr<IsolateGroupSource> source,
           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
@@ -435,6 +435,8 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
   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 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
   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
@@ -734,6 +734,7 @@ void KernelLoader::LoadNativeExtension(const Library& library,
 }
 
 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
@@ -1116,7 +1116,11 @@ void Object::Init(Isolate* isolate) {
   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 @@ ErrorPtr Object::Init(Isolate* isolate,
     // 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 @@ class ClassFunctionsTraits {
 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::SetFunctions(const Array& value) const {
 }
 
 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 @@ FunctionPtr Function::GetMethodExtractor(const String& getter_name) const {
   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 @@ ErrorPtr Class::EnsureIsFinalized(Thread* thread) const {
     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 @@ static ObjectPtr EvaluateCompiledExpressionHelper(
     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
@@ -121,7 +121,10 @@ ISOLATE_UNIT_TEST_CASE(Class) {
   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 @@ ISOLATE_UNIT_TEST_CASE(SixtyThousandDartClasses) {
     }
 
     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 @@ ISOLATE_UNIT_TEST_CASE(Closure) {
       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 @@ ISOLATE_UNIT_TEST_CASE(FindClosureIndex) {
       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 @@ ISOLATE_UNIT_TEST_CASE(FindInvocationDispatcherFunctionIndex) {
       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
@@ -57,6 +57,7 @@ static FunctionPtr ResolveDynamicAnyArgsWithCustomLookup(
 
   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,19 +83,22 @@ static FunctionPtr ResolveDynamicAnyArgsWithCustomLookup(
       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();
         }
       }
     }
     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
@@ -109,27 +109,6 @@ void VerifyOnTransition() {
 }
 #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 @@ static FunctionPtr Resolve(Thread* thread,
                            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
@@ -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
@@ -436,6 +436,7 @@ vm_sources_tests = [
   "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",

Original file line number	Diff line number	Diff line change
`@@ -1724,6 +1724,7 @@ void Precompiler::DropFunctions() {`
`1724`	`1724`	`}`
`1725`	`1725`	`};`
`1726`	`1726`
	`1727`	`+ SafepointWriteRwLocker ml(T, T->isolate_group()->program_lock());`
`1727`	`1728`	`auto& dispatchers_array = Array::Handle(Z);`
`1728`	`1729`	`auto& name = String::Handle(Z);`
`1729`	`1730`	`auto& desc = Array::Handle(Z);`
Original file line number	Diff line number	Diff line change
`@@ -734,6 +734,7 @@ void KernelLoader::LoadNativeExtension(const Library& library,`
`734`	`734`	`}`
`735`	`735`
`736`	`736`	`ObjectPtr KernelLoader::LoadProgram(bool process_pending_classes) {`
	`737`	`+ SafepointWriteRwLocker ml(thread_, thread_->isolate_group()->program_lock());`
`737`	`738`	`ASSERT(kernel_program_info_.constants() == Array::null());`
`738`	`739`
`739`	`740`	`if (!program_->is_single_program()) {`