[vm] Fix racy initialization of lazily populated ObjectStore members
If two threads concurrently trigger initialization of the lazily
populated ObjectStore members, they have to coordinate.
We ensure mutual exclusion by holding a writer lock, to ensure only one
thread can perform the lazy initialization. Furthermore we use
store-release/load-acquire for those ObjectStore fields to ensure that
once an object is made available on ObjectStore all of it's initializing
stores are visible at that point.
We no longer make public setters for those fields, since they shouldn't
be used (only the getters + lazy initialization should be used)
Fixes https://github.com/dart-lang/sdk/issues/46611
TEST=Fixes TSAN races reported on iso-stres builder.
Change-Id: Icaf54bb224e74baf1bfbb4a9bfc386ebfbd52755
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206782
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Tess Strickland <sstrickl@google.com>
diff --git a/runtime/vm/object_store.cc b/runtime/vm/object_store.cc
index 2ffb6ce..8b84d23 100644
--- a/runtime/vm/object_store.cc
+++ b/runtime/vm/object_store.cc
@@ -334,40 +334,46 @@
}
void ObjectStore::LazyInitCoreTypes() {
- if (list_class_ == Type::null()) {
- ASSERT(non_nullable_list_rare_type_ == Type::null());
- ASSERT(non_nullable_map_rare_type_ == Type::null());
- Thread* thread = Thread::Current();
- Zone* zone = thread->zone();
- const Library& core_lib = Library::Handle(zone, Library::CoreLibrary());
- Class& cls = Class::Handle(zone, core_lib.LookupClass(Symbols::List()));
+ auto* const thread = Thread::Current();
+ SafepointWriteRwLocker locker(thread,
+ thread->isolate_group()->program_lock());
+ if (list_class_.load() == Type::null()) {
+ ASSERT(non_nullable_list_rare_type_.load() == Type::null());
+ ASSERT(non_nullable_map_rare_type_.load() == Type::null());
+
+ auto* const zone = thread->zone();
+ const auto& core_lib = Library::Handle(zone, Library::CoreLibrary());
+ auto& cls = Class::Handle(zone);
+
+ cls = core_lib.LookupClass(Symbols::List());
ASSERT(!cls.IsNull());
- set_list_class(cls);
- Type& type = Type::Handle(zone);
+ list_class_.store(cls.ptr());
+
+ auto& type = Type::Handle(zone);
type ^= cls.RareType();
- set_non_nullable_list_rare_type(type);
+ non_nullable_list_rare_type_.store(type.ptr());
+
cls = core_lib.LookupClass(Symbols::Map());
ASSERT(!cls.IsNull());
type ^= cls.RareType();
- set_non_nullable_map_rare_type(type);
+ non_nullable_map_rare_type_.store(type.ptr());
}
}
void ObjectStore::LazyInitFutureTypes() {
- if (non_nullable_future_rare_type_ == Type::null()) {
- ASSERT(non_nullable_future_never_type_ == Type::null() &&
- nullable_future_null_type_ == Type::null());
- Thread* thread = Thread::Current();
- Zone* zone = thread->zone();
- Class& cls = Class::Handle(zone, future_class());
- if (cls.IsNull()) {
- const Library& async_lib = Library::Handle(zone, async_library());
- ASSERT(!async_lib.IsNull());
- cls = async_lib.LookupClass(Symbols::Future());
- ASSERT(!cls.IsNull());
- }
- TypeArguments& type_args = TypeArguments::Handle(zone);
- Type& type = Type::Handle(zone);
+ auto* const thread = Thread::Current();
+ SafepointWriteRwLocker locker(thread,
+ thread->isolate_group()->program_lock());
+ if (non_nullable_future_rare_type_.load() == Type::null()) {
+ ASSERT(non_nullable_future_never_type_.load() == Type::null());
+ ASSERT(nullable_future_null_type_.load() == Type::null());
+
+ auto* const zone = thread->zone();
+ const auto& cls = Class::Handle(zone, future_class());
+ ASSERT(!cls.IsNull());
+
+ auto& type_args = TypeArguments::Handle(zone);
+ auto& type = Type::Handle(zone);
type = never_type();
ASSERT(!type.IsNull());
type_args = TypeArguments::New(1);
@@ -375,7 +381,8 @@
type = Type::New(cls, type_args, Nullability::kNonNullable);
type.SetIsFinalized();
type ^= type.Canonicalize(thread, nullptr);
- set_non_nullable_future_never_type(type);
+ non_nullable_future_never_type_.store(type.ptr());
+
type = null_type();
ASSERT(!type.IsNull());
type_args = TypeArguments::New(1);
@@ -383,9 +390,10 @@
type = Type::New(cls, type_args, Nullability::kNullable);
type.SetIsFinalized();
type ^= type.Canonicalize(thread, nullptr);
- set_nullable_future_null_type(type);
+ nullable_future_null_type_.store(type.ptr());
+
type ^= cls.RareType();
- set_non_nullable_future_rare_type(type);
+ non_nullable_future_rare_type_.store(type.ptr());
}
}
diff --git a/runtime/vm/object_store.h b/runtime/vm/object_store.h
index eec9974e..9e5606a 100644
--- a/runtime/vm/object_store.h
+++ b/runtime/vm/object_store.h
@@ -36,9 +36,9 @@
//
// R_ - needs getter only
// RW - needs getter and setter
-// CW - needs lazy Core init getter and setter
-// FW - needs lazy Future init getter and setter
-#define OBJECT_STORE_FIELD_LIST(R_, RW, CW, FW) \
+// CR - needs lazy Core init getter
+// FR - needs lazy Future init getter
+#define OBJECT_STORE_FIELD_LIST(R_, RW, CR, FR) \
RW(Class, object_class) \
RW(Type, object_type) \
RW(Type, legacy_object_type) \
@@ -81,12 +81,12 @@
RW(Type, string_type) \
RW(Type, legacy_string_type) \
RW(Type, non_nullable_string_type) \
- CW(Class, list_class) /* maybe be null, lazily built */ \
- CW(Type, non_nullable_list_rare_type) /* maybe be null, lazily built */ \
- CW(Type, non_nullable_map_rare_type) /* maybe be null, lazily built */ \
- FW(Type, non_nullable_future_rare_type) /* maybe be null, lazily built */ \
- FW(Type, non_nullable_future_never_type) /* maybe be null, lazily built */ \
- FW(Type, nullable_future_null_type) /* maybe be null, lazily built */ \
+ CR(Class, list_class) /* maybe be null, lazily built */ \
+ CR(Type, non_nullable_list_rare_type) /* maybe be null, lazily built */ \
+ CR(Type, non_nullable_map_rare_type) /* maybe be null, lazily built */ \
+ FR(Type, non_nullable_future_rare_type) /* maybe be null, lazily built */ \
+ FR(Type, non_nullable_future_never_type) /* maybe be null, lazily built */ \
+ FR(Type, nullable_future_null_type) /* maybe be null, lazily built */ \
RW(TypeArguments, type_argument_int) \
RW(TypeArguments, type_argument_legacy_int) \
RW(TypeArguments, type_argument_non_nullable_int) \
@@ -395,27 +395,25 @@
void set_##name(const Type& value) { name##_ = value.ptr(); }
#define DECLARE_LAZY_INIT_GETTER(Type, name, init) \
Type##Ptr name() { \
- if (name##_ == Type::null()) { \
+ if (name##_.load() == Type::null()) { \
init(); \
} \
- return name##_; \
+ return name##_.load(); \
} \
static intptr_t name##_offset() { return OFFSET_OF(ObjectStore, name##_); }
-#define DECLARE_LAZY_INIT_CORE_GETTER_AND_SETTER(Type, name) \
- DECLARE_LAZY_INIT_GETTER(Type, name, LazyInitCoreTypes) \
- void set_##name(const Type& value) { name##_ = value.ptr(); }
-#define DECLARE_LAZY_INIT_FUTURE_GETTER_AND_SETTER(Type, name) \
- DECLARE_LAZY_INIT_GETTER(Type, name, LazyInitFutureTypes) \
- void set_##name(const Type& value) { name##_ = value.ptr(); }
+#define DECLARE_LAZY_INIT_CORE_GETTER(Type, name) \
+ DECLARE_LAZY_INIT_GETTER(Type, name, LazyInitCoreTypes)
+#define DECLARE_LAZY_INIT_FUTURE_GETTER(Type, name) \
+ DECLARE_LAZY_INIT_GETTER(Type, name, LazyInitFutureTypes)
OBJECT_STORE_FIELD_LIST(DECLARE_GETTER,
DECLARE_GETTER_AND_SETTER,
- DECLARE_LAZY_INIT_CORE_GETTER_AND_SETTER,
- DECLARE_LAZY_INIT_FUTURE_GETTER_AND_SETTER)
+ DECLARE_LAZY_INIT_CORE_GETTER,
+ DECLARE_LAZY_INIT_FUTURE_GETTER)
#undef DECLARE_GETTER
#undef DECLARE_GETTER_AND_SETTER
#undef DECLARE_LAZY_INIT_GETTER
-#undef DECLARE_LAZY_INIT_CORE_GETTER_AND_SETTER
-#undef DECLARE_LAZY_INIT_FUTURE_GETTER_AND_SETTER
+#undef DECLARE_LAZY_INIT_CORE_GETTER
+#undef DECLARE_LAZY_INIT_FUTURE_GETTER
LibraryPtr bootstrap_library(BootstrapLibraryId index) {
switch (index) {
@@ -471,10 +469,13 @@
ObjectPtr* from() { return reinterpret_cast<ObjectPtr*>(&object_class_); }
#define DECLARE_OBJECT_STORE_FIELD(type, name) type##Ptr name##_;
+#define DECLARE_LAZY_OBJECT_STORE_FIELD(type, name) \
+ AcqRelAtomic<type##Ptr> name##_;
OBJECT_STORE_FIELD_LIST(DECLARE_OBJECT_STORE_FIELD,
DECLARE_OBJECT_STORE_FIELD,
- DECLARE_OBJECT_STORE_FIELD,
- DECLARE_OBJECT_STORE_FIELD)
+ DECLARE_LAZY_OBJECT_STORE_FIELD,
+ DECLARE_LAZY_OBJECT_STORE_FIELD)
+#undef DECLARE_LAZY_OBJECT_STORE_FIELD
#undef DECLARE_OBJECT_STORE_FIELD
ObjectPtr* to() {
return reinterpret_cast<ObjectPtr*>(&ffi_as_function_internal_);
diff --git a/runtime/vm/version_in.cc b/runtime/vm/version_in.cc
index 206cd1f..4fe06a5 100644
--- a/runtime/vm/version_in.cc
+++ b/runtime/vm/version_in.cc
@@ -10,7 +10,7 @@
namespace dart {
-// We use require-release semantics to ensure initializing stores to the string
+// We use acquire-release semantics to ensure initializing stores to the string
// are visible when the string becomes visible.
static AcqRelAtomic<const char*> formatted_version = nullptr;
