[vm/compiler] Avoid load forwarding for weak fields
TEST=runtime/tests/vm/dart/weak_field_load_forwarding_il_test.dart
Fixes https://github.com/dart-lang/sdk/issues/55518
Change-Id: I4576b25ba8f88fe5bb1bd319baa5980fe22cccce
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/364201
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Slava Egorov <vegorov@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
diff --git a/runtime/tests/vm/dart/weak_field_load_forwarding_il_test.dart b/runtime/tests/vm/dart/weak_field_load_forwarding_il_test.dart
new file mode 100644
index 0000000..a4bee40
--- /dev/null
+++ b/runtime/tests/vm/dart/weak_field_load_forwarding_il_test.dart
@@ -0,0 +1,105 @@
+// Copyright (c) 2024, 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.
+
+// Verify that load forwarding doesn't happen for weak fields
+// (values of weak fields can be set to null by the GC).
+// Regression test for https://github.com/dart-lang/sdk/issues/55518.
+
+import 'package:expect/expect.dart';
+import 'package:vm/testing/il_matchers.dart';
+
+class StrongRef {
+ Object? target;
+ StrongRef(this.target);
+}
+
+@pragma('vm:never-inline')
+@pragma('vm:testing:print-flow-graph')
+bool loadLoadStrongRef(StrongRef obj) {
+ var v1 = obj.target;
+ var v2 = obj.target;
+ return identical(v1, v2);
+}
+
+void matchIL$loadLoadStrongRef(FlowGraph graph) {
+ graph.match([
+ match.block('Graph', [
+ 'true' << match.Constant(value: true),
+ ]),
+ match.block('Function', [
+ 'obj' << match.Parameter(index: 0),
+ match.DartReturn('true'),
+ ]),
+ ]);
+}
+
+@pragma('vm:never-inline')
+@pragma('vm:testing:print-flow-graph')
+bool loadLoadWeakReference(WeakReference obj) {
+ var v1 = obj.target;
+ var v2 = obj.target;
+ return identical(v1, v2);
+}
+
+void matchIL$loadLoadWeakReference(FlowGraph graph) {
+ graph.match([
+ match.block('Graph'),
+ match.block('Function', [
+ 'obj' << match.Parameter(index: 0),
+ 'v1' << match.LoadField('obj', slot: 'WeakReference.target'),
+ 'v2' << match.LoadField('obj', slot: 'WeakReference.target'),
+ 'res' << match.StrictCompare('v1', 'v2', kind: '==='),
+ match.DartReturn('res'),
+ ]),
+ ]);
+}
+
+@pragma('vm:never-inline')
+@pragma('vm:testing:print-flow-graph')
+bool storeLoadStrongRef(Object v1) {
+ final obj = StrongRef(v1);
+ var v2 = obj.target;
+ return identical(v1, v2);
+}
+
+void matchIL$storeLoadStrongRef(FlowGraph graph) {
+ graph.match([
+ match.block('Graph', [
+ 'true' << match.Constant(value: true),
+ ]),
+ match.block('Function', [
+ 'obj' << match.Parameter(index: 0),
+ match.DartReturn('true'),
+ ]),
+ ]);
+}
+
+@pragma('vm:never-inline')
+@pragma('vm:testing:print-flow-graph')
+bool storeLoadWeakReference(Object v1) {
+ final obj = WeakReference(v1);
+ var v2 = obj.target;
+ return identical(v1, v2);
+}
+
+void matchIL$storeLoadWeakReference(FlowGraph graph) {
+ graph.match([
+ match.block('Graph'),
+ match.block('Function', [
+ 'v1' << match.Parameter(index: 0),
+ 'obj' << match.AllocateObject(),
+ match.StoreField('obj', 'v1', slot: 'WeakReference.target'),
+ 'v2' << match.LoadField('obj', slot: 'WeakReference.target'),
+ 'res' << match.StrictCompare('v1', 'v2', kind: '==='),
+ match.DartReturn('res'),
+ ]),
+ ]);
+}
+
+void main(List<String> args) {
+ loadLoadStrongRef(StrongRef(Object()));
+ loadLoadWeakReference(WeakReference(Object()));
+ storeLoadStrongRef(Object());
+ storeLoadWeakReference(Object());
+}
diff --git a/runtime/vm/compiler/backend/redundancy_elimination.cc b/runtime/vm/compiler/backend/redundancy_elimination.cc
index 851635c..d43ab2f 100644
--- a/runtime/vm/compiler/backend/redundancy_elimination.cc
+++ b/runtime/vm/compiler/backend/redundancy_elimination.cc
@@ -1427,6 +1427,11 @@
instr->AsDefinition()->MayCreateUnsafeUntaggedPointer()) {
return false;
}
+ if (auto* load = instr->AsLoadField()) {
+ if (load->slot().is_weak()) {
+ return false;
+ }
+ }
return instr->IsLoadField() || instr->IsLoadIndexed() ||
instr->IsLoadStaticField();
}
diff --git a/runtime/vm/compiler/backend/slot.cc b/runtime/vm/compiler/backend/slot.cc
index cd2e0ea..92b6aa2 100644
--- a/runtime/vm/compiler/backend/slot.cc
+++ b/runtime/vm/compiler/backend/slot.cc
@@ -68,14 +68,18 @@
Slot* SlotCache::CreateNativeSlot(Slot::Kind kind) {
switch (kind) {
-#define FIELD_FINAL true
-#define FIELD_VAR false
+#define FIELD_FLAGS_FINAL \
+ (Slot::IsImmutableBit::encode(true) | Slot::IsWeakBit::encode(false))
+#define FIELD_FLAGS_VAR \
+ (Slot::IsImmutableBit::encode(false) | Slot::IsWeakBit::encode(false))
+#define FIELD_FLAGS_WEAK \
+ (Slot::IsImmutableBit::encode(false) | Slot::IsWeakBit::encode(true))
#define DEFINE_NULLABLE_TAGGED_NATIVE_DART_FIELD(ClassName, UnderlyingType, \
FieldName, cid, mutability) \
case Slot::Kind::k##ClassName##_##FieldName: \
return new (zone_) Slot( \
Slot::Kind::k##ClassName##_##FieldName, \
- (Slot::IsImmutableBit::encode(FIELD_##mutability) | \
+ (FIELD_FLAGS_##mutability | \
Slot::IsCompressedBit::encode( \
ClassName::ContainsCompressedPointers())), \
compiler::target::ClassName::FieldName##_offset(), \
@@ -94,7 +98,7 @@
case Slot::Kind::k##ClassName##_##FieldName: \
return new (zone_) Slot( \
Slot::Kind::k##ClassName##_##FieldName, \
- (Slot::IsImmutableBit::encode(FIELD_##mutability) | \
+ (FIELD_FLAGS_##mutability | \
Slot::IsCompressedBit::encode( \
ClassName::ContainsCompressedPointers())), \
compiler::target::ClassName::FieldName##_offset(), \
@@ -115,8 +119,7 @@
case Slot::Kind::k##ClassName##_##FieldName: \
return new (zone_) \
Slot(Slot::Kind::k##ClassName##_##FieldName, \
- Slot::IsImmutableBit::encode(FIELD_##mutability) | \
- Slot::IsNonTaggedBit::encode(true), \
+ FIELD_FLAGS_##mutability | Slot::IsNonTaggedBit::encode(true), \
compiler::target::ClassName::FieldName##_offset(), \
#ClassName "." #FieldName, \
CompileType::FromUnboxedRepresentation(kUnboxed##representation), \
@@ -131,7 +134,7 @@
case Slot::Kind::k##ClassName##_##FieldName: \
return new (zone_) \
Slot(Slot::Kind::k##ClassName##_##FieldName, \
- Slot::IsImmutableBit::encode(FIELD_##mutability) | \
+ FIELD_FLAGS_##mutability | \
Slot::MayContainInnerPointerBit::encode(GcMayMove) | \
Slot::IsNonTaggedBit::encode(true), \
compiler::target::ClassName::FieldName##_offset(), \
@@ -146,8 +149,7 @@
case Slot::Kind::k##ClassName##_##FieldName: \
return new (zone_) Slot( \
Slot::Kind::k##ClassName##_##FieldName, \
- Slot::IsImmutableBit::encode(FIELD_##mutability) | \
- Slot::HasUntaggedInstanceBit::encode(true), \
+ FIELD_FLAGS_##mutability | Slot::HasUntaggedInstanceBit::encode(true), \
compiler::target::ClassName::FieldName##_offset(), \
#ClassName "." #FieldName, \
CompileType(CompileType::kCanBeNull, CompileType::kCannotBeSentinel, \
@@ -164,8 +166,7 @@
case Slot::Kind::k##ClassName##_##FieldName: \
return new (zone_) \
Slot(Slot::Kind::k##ClassName##_##FieldName, \
- Slot::IsImmutableBit::encode(FIELD_##mutability) | \
- Slot::IsNonTaggedBit::encode(true) | \
+ FIELD_FLAGS_##mutability | Slot::IsNonTaggedBit::encode(true) | \
Slot::HasUntaggedInstanceBit::encode(true), \
compiler::target::ClassName::FieldName##_offset(), \
#ClassName "." #FieldName, \
@@ -181,7 +182,7 @@
case Slot::Kind::k##ClassName##_##FieldName: \
return new (zone_) \
Slot(Slot::Kind::k##ClassName##_##FieldName, \
- Slot::IsImmutableBit::encode(FIELD_##mutability) | \
+ FIELD_FLAGS_##mutability | \
Slot::MayContainInnerPointerBit::encode(gc_may_move) | \
Slot::IsNonTaggedBit::encode(true) | \
Slot::HasUntaggedInstanceBit::encode(true), \
@@ -192,8 +193,9 @@
#undef DEFINE_UNTAGGED_NATIVE_NONDART_FIELD
-#undef FIELD_VAR
-#undef FIELD_FINAL
+#undef FIELD_FLAGS_FINAL
+#undef FIELD_FLAGS_VAR
+#undef FIELD_FLAGS_WEAK
default:
UNREACHABLE();
}
diff --git a/runtime/vm/compiler/backend/slot.h b/runtime/vm/compiler/backend/slot.h
index 89a5735..c6bb983 100644
--- a/runtime/vm/compiler/backend/slot.h
+++ b/runtime/vm/compiler/backend/slot.h
@@ -41,7 +41,7 @@
// The list of slots that correspond to nullable boxed fields of native
// Dart objects in the following format:
//
-// V(class_name, underlying_type, field_name, exact_type, FINAL|VAR)
+// V(class_name, underlying_type, field_name, exact_type, FINAL|VAR|WEAK)
//
// - class_name and field_name specify the name of the host class and the name
// of the field respectively;
@@ -50,17 +50,17 @@
// would only yield instances of this type);
// - the last component specifies whether field behaves like a final field
// (i.e. initialized once at construction time and does not change after
-// that) or like a non-final field.
+// that), ordinary mutable field or a weak field (can be modified by GC).
#define NULLABLE_TAGGED_NATIVE_DART_SLOTS_LIST(V) \
V(Array, UntaggedArray, type_arguments, TypeArguments, FINAL) \
V(Finalizer, UntaggedFinalizer, type_arguments, TypeArguments, FINAL) \
V(FinalizerBase, UntaggedFinalizerBase, all_entries, Set, VAR) \
V(FinalizerBase, UntaggedFinalizerBase, detachments, Dynamic, VAR) \
V(FinalizerBase, UntaggedFinalizer, entries_collected, FinalizerEntry, VAR) \
- V(FinalizerEntry, UntaggedFinalizerEntry, value, Dynamic, VAR) \
- V(FinalizerEntry, UntaggedFinalizerEntry, detach, Dynamic, VAR) \
+ V(FinalizerEntry, UntaggedFinalizerEntry, value, Dynamic, WEAK) \
+ V(FinalizerEntry, UntaggedFinalizerEntry, detach, Dynamic, WEAK) \
V(FinalizerEntry, UntaggedFinalizerEntry, token, Dynamic, VAR) \
- V(FinalizerEntry, UntaggedFinalizerEntry, finalizer, FinalizerBase, VAR) \
+ V(FinalizerEntry, UntaggedFinalizerEntry, finalizer, FinalizerBase, WEAK) \
V(FinalizerEntry, UntaggedFinalizerEntry, next, FinalizerEntry, VAR) \
V(Function, UntaggedFunction, signature, FunctionType, FINAL) \
V(Context, UntaggedContext, parent, Context, FINAL) \
@@ -81,9 +81,9 @@
V(TypeParameters, UntaggedTypeParameters, flags, Array, FINAL) \
V(TypeParameters, UntaggedTypeParameters, bounds, TypeArguments, FINAL) \
V(TypeParameters, UntaggedTypeParameters, defaults, TypeArguments, FINAL) \
- V(WeakProperty, UntaggedWeakProperty, key, Dynamic, VAR) \
- V(WeakProperty, UntaggedWeakProperty, value, Dynamic, VAR) \
- V(WeakReference, UntaggedWeakReference, target, Dynamic, VAR) \
+ V(WeakProperty, UntaggedWeakProperty, key, Dynamic, WEAK) \
+ V(WeakProperty, UntaggedWeakProperty, value, Dynamic, WEAK) \
+ V(WeakReference, UntaggedWeakReference, target, Dynamic, WEAK) \
V(WeakReference, UntaggedWeakReference, type_arguments, TypeArguments, FINAL)
// The list of slots that correspond to non-nullable boxed fields of native
@@ -322,7 +322,7 @@
// guaranteed to be a integer. This includes nullable integer slots, since
// those slots may return a non-integer value (null). Such uses can
// only use the following arguments for each entry:
-// V(class_name, _, field_name, exact_type, FINAL|VAR)
+// V(class_name, _, field_name, exact_type, FINAL|VAR|WEAK)
#define TAGGED_NONINT_NATIVE_SLOTS_LIST(V) \
NULLABLE_TAGGED_NATIVE_DART_SLOTS_LIST(V) \
NONNULLABLE_NONINT_TAGGED_NATIVE_DART_SLOTS_LIST(V) \
@@ -331,7 +331,7 @@
// For uses that need any native slot that is not guaranteed to contain an
// integer, whether a Dart object or unboxed. Such uses can only use the
// following arguments for each entry:
-// V(class_name, _, field_name, _, FINAL|VAR)
+// V(class_name, _, field_name, _, FINAL|VAR|WEAK)
#define NOT_INT_NATIVE_SLOTS_LIST(V) \
TAGGED_NONINT_NATIVE_SLOTS_LIST(V) \
UNTAGGED_NATIVE_SLOTS_LIST(V)
@@ -339,7 +339,7 @@
// For uses that need any native slot on Dart objects that contains a Dart
// object (e.g., for write barrier purposes). Such uses can use the following
// arguments for each entry:
-// V(class_name, underlying_class, field_name, exact_type, FINAL|VAR)
+// V(class_name, underlying_class, field_name, exact_type, FINAL|VAR|WEAK)
#define TAGGED_NATIVE_DART_SLOTS_LIST(V) \
NULLABLE_TAGGED_NATIVE_DART_SLOTS_LIST(V) \
NONNULLABLE_INT_TAGGED_NATIVE_DART_SLOTS_LIST(V) \
@@ -355,14 +355,14 @@
// For uses that need any native slot that contains a Dart object. Such uses can
// only use the following arguments for each entry:
-// V(class_name, _, field_name, exact_type, FINAL|VAR)
+// V(class_name, _, field_name, exact_type, FINAL|VAR|WEAK)
#define TAGGED_NATIVE_SLOTS_LIST(V) \
TAGGED_INT_NATIVE_SLOTS_LIST(V) \
TAGGED_NONINT_NATIVE_SLOTS_LIST(V)
// For uses that need all native slots. Such uses can only use the following
// arguments for each entry:
-// V(class_name, _, field_name, _, FINAL|VAR)
+// V(class_name, _, field_name, _, FINAL|VAR|WEAK)
#define NATIVE_SLOTS_LIST(V) \
TAGGED_NATIVE_SLOTS_LIST(V) \
NOT_TAGGED_NATIVE_SLOTS_LIST(V)
@@ -520,6 +520,8 @@
bool is_immutable() const { return IsImmutableBit::decode(flags_); }
+ bool is_weak() const { return IsWeakBit::decode(flags_); }
+
// Returns true if properties of this slot were based on the guarded state
// of the corresponding Dart field.
bool is_guarded_field() const { return IsGuardedBit::decode(flags_); }
@@ -620,8 +622,9 @@
CompileType type_;
using IsImmutableBit = BitField<decltype(flags_), bool, 0, 1>;
- using IsGuardedBit =
+ using IsWeakBit =
BitField<decltype(flags_), bool, IsImmutableBit::kNextBit, 1>;
+ using IsGuardedBit = BitField<decltype(flags_), bool, IsWeakBit::kNextBit, 1>;
using IsCompressedBit =
BitField<decltype(flags_), bool, IsGuardedBit::kNextBit, 1>;
// Stores whether a field isn't tagged so that tagged is the default value
