[vm/concurrency] Guard updates of Field::guarded_cid and is_nullable.
This disables field unboxing in JIT with enabled lightweight isolates because currently unboxed generated code won't behave correctly if another isolate updates field unboxed state: for that guarded_cid updates would require stopping all isolates, getting to a safepoint.
Use of field guards(guarded_cid, is_nullable) in optimized compiler is concurrent-safe because each optimized compiler operates on its own clones of fields, which are checked for consistency with latest(mutator) state just before optimized code is installed.
Use of field guards in optimized generated code is safe because vm is brought to a safepoint and optimized code is deoptimized as soon as field guards are invalidated.
Use of field guards in generated unoptimized code outside of unboxed fields(transition to boxed from previously unboxed field is not safe which is why field unboxing is disabled) and use of field guards in unoptimized compiler is safe because transitions through guarded_cid and is_nullable makes less and less optimistic assumptions about the field state.
Issue https://github.com/dart-lang/sdk/issues/36097.
TEST=existing test suite
Change-Id: I00d37a49df483814c846b17c9c49e0dc6ce701e1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/172860
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Alexander Aprelev <aam@google.com>
diff --git a/runtime/vm/clustered_snapshot.cc b/runtime/vm/clustered_snapshot.cc
index 906d5f6..3caa919 100644
--- a/runtime/vm/clustered_snapshot.cc
+++ b/runtime/vm/clustered_snapshot.cc
@@ -1165,8 +1165,8 @@
if (!Isolate::Current()->use_field_guards()) {
for (intptr_t i = start_index_; i < stop_index_; i++) {
field ^= refs.At(i);
- field.set_guarded_cid(kDynamicCid);
- field.set_is_nullable(true);
+ field.set_guarded_cid_unsafe(kDynamicCid);
+ field.set_is_nullable_unsafe(true);
field.set_guarded_list_length(Field::kNoFixedLength);
field.set_guarded_list_length_in_object_offset(
Field::kUnknownLengthOffset);
diff --git a/runtime/vm/compiler/backend/flow_graph_compiler.cc b/runtime/vm/compiler/backend/flow_graph_compiler.cc
index 20428a1..b8b18c7 100644
--- a/runtime/vm/compiler/backend/flow_graph_compiler.cc
+++ b/runtime/vm/compiler/backend/flow_graph_compiler.cc
@@ -174,11 +174,13 @@
// The `field.is_non_nullable_integer()` is set in the kernel loader and can
// only be set if we consume a AOT kernel (annotated with inferred types).
ASSERT(!field.is_non_nullable_integer() || FLAG_precompiled_mode);
+ // Unboxed fields in JIT lightweight isolates mode are not supported yet.
const bool valid_class =
- (SupportsUnboxedDoubles() && (field.guarded_cid() == kDoubleCid)) ||
- (SupportsUnboxedSimd128() && (field.guarded_cid() == kFloat32x4Cid)) ||
- (SupportsUnboxedSimd128() && (field.guarded_cid() == kFloat64x2Cid)) ||
- field.is_non_nullable_integer();
+ (FLAG_precompiled_mode || !FLAG_enable_isolate_groups) &&
+ ((SupportsUnboxedDoubles() && (field.guarded_cid() == kDoubleCid)) ||
+ (SupportsUnboxedSimd128() && (field.guarded_cid() == kFloat32x4Cid)) ||
+ (SupportsUnboxedSimd128() && (field.guarded_cid() == kFloat64x2Cid)) ||
+ field.is_non_nullable_integer());
return field.is_unboxing_candidate() && !field.is_nullable() && valid_class;
}
@@ -189,7 +191,8 @@
// proven to be correct.
return IsUnboxedField(field);
}
- return field.is_unboxing_candidate() &&
+ // Unboxed fields in JIT lightweight isolates mode are not supported yet.
+ return !FLAG_enable_isolate_groups && field.is_unboxing_candidate() &&
(FlowGraphCompiler::IsUnboxedField(field) ||
(field.guarded_cid() == kIllegalCid));
}
diff --git a/runtime/vm/compiler/backend/slot_test.cc b/runtime/vm/compiler/backend/slot_test.cc
index a506698..8c4b620 100644
--- a/runtime/vm/compiler/backend/slot_test.cc
+++ b/runtime/vm/compiler/backend/slot_test.cc
@@ -59,8 +59,8 @@
TokenPosition::kMinSource));
// Set non-trivial guarded state on the field.
- field.set_guarded_cid(kSmiCid);
- field.set_is_nullable(false);
+ field.set_guarded_cid_unsafe(kSmiCid);
+ field.set_is_nullable_unsafe(false);
// Enter compiler state.
CompilerState compiler_state(thread, /*is_aot=*/false);
@@ -87,8 +87,8 @@
// Change the guarded state of the field to "unknown" - emulating concurrent
// modification of the guarded state in mutator) and create a new clone of
// the field.
- field.set_guarded_cid(kDynamicCid);
- field.set_is_nullable(true);
+ field.set_guarded_cid_unsafe(kDynamicCid);
+ field.set_is_nullable_unsafe(true);
const Field& field_clone_3 = Field::ZoneHandle(field.CloneFromOriginal());
// Slot::Get must return the same slot and add the field from which it
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index 2e907af..aff7bc5 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -10128,8 +10128,8 @@
FLAG_precompiled_mode ||
(isolate->use_field_guards() && !isolate->HasAttemptedReload());
#endif // !defined(PRODUCT)
- result.set_guarded_cid(use_guarded_cid ? kIllegalCid : kDynamicCid);
- result.set_is_nullable(use_guarded_cid ? false : true);
+ result.set_guarded_cid_unsafe(use_guarded_cid ? kIllegalCid : kDynamicCid);
+ result.set_is_nullable_unsafe(use_guarded_cid ? false : true);
result.set_guarded_list_length_in_object_offset(Field::kUnknownLengthOffset);
// Presently, we only attempt to remember the list length for final fields.
if (is_final && use_guarded_cid) {
@@ -10961,6 +10961,8 @@
// We should never try to record a sentinel.
ASSERT(value.raw() != Object::sentinel().raw());
+ Thread* const thread = Thread::Current();
+ SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
if ((guarded_cid() == kDynamicCid) ||
(is_nullable() && value.raw() == Object::null())) {
// Nothing to do: the field is not guarded or we are storing null into
@@ -10986,8 +10988,6 @@
THR_Print(" => %s\n", GuardedPropertiesAsCString());
}
- Thread* const thread = Thread::Current();
- SafepointWriteRwLocker ml(thread, thread->isolate_group()->program_lock());
DeoptimizeDependentCode();
}
}
diff --git a/runtime/vm/object.h b/runtime/vm/object.h
index 6538625..a950136 100644
--- a/runtime/vm/object.h
+++ b/runtime/vm/object.h
@@ -4162,6 +4162,11 @@
}
void set_guarded_cid(intptr_t cid) const {
+ DEBUG_ASSERT(
+ IsolateGroup::Current()->program_lock()->IsCurrentThreadWriter());
+ set_guarded_cid_unsafe(cid);
+ }
+ void set_guarded_cid_unsafe(intptr_t cid) const {
#if defined(DEBUG)
Thread* thread = Thread::Current();
ASSERT(!IsOriginal() || is_static() || thread->IsMutatorThread() ||
@@ -4246,6 +4251,11 @@
return raw_ptr()->is_nullable_ == kNullCid;
}
void set_is_nullable(bool val) const {
+ DEBUG_ASSERT(
+ IsolateGroup::Current()->program_lock()->IsCurrentThreadWriter());
+ set_is_nullable_unsafe(val);
+ }
+ void set_is_nullable_unsafe(bool val) const {
ASSERT(Thread::Current()->IsMutatorThread());
StoreNonPointer(&raw_ptr()->is_nullable_, val ? kNullCid : kIllegalCid);
}
