[vm/compiler] Ensure that field is added to guarded fields when Slot::Get uses its guarded properties.
There might be a race between background compiler and mutator where
mutator changes guarded state of the field after Slot was created from
it.
A situation is possible where we have a clone of a field with its
guarded state set to unknown, however Slot::Get for this field returns
a Slot created from the previous clone of the same field with a known
guarded state. In this case we must add *old* clone from which the Slot
was created to guarded fields and not the new clone, because new clone
has no guarded state to begin with and thus
ParsedFunction::AddToGuardedFields(...) would simply ignore it.
Such slots with inconsistent guarded state that are not in the current
list of guarded fields arise due to unsuccessful inlining attempts.
If we built and discard the graph, then guarded fields associated with
that graph are also discarded. However the slot itself stays behind in
the compilation global cache.
TEST=vm/cc/SlotFromGuardedField
Bug: b/121271056
Change-Id: Ib6ee5ec4922c033c3a71bdc46da74ead47b1edd0
Reviewed-on: https://dart-review.googlesource.com/c/88575
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
diff --git a/runtime/vm/compiler/backend/slot.cc b/runtime/vm/compiler/backend/slot.cc
index 79574ea..f1c01f9 100644
--- a/runtime/vm/compiler/backend/slot.cc
+++ b/runtime/vm/compiler/backend/slot.cc
@@ -136,15 +136,46 @@
}
}
+ bool used_guarded_state = false;
if (field.guarded_cid() != kIllegalCid &&
field.guarded_cid() != kDynamicCid) {
- nullable_cid =
- nullable_cid != kDynamicCid ? nullable_cid : field.guarded_cid();
- is_nullable = is_nullable && field.is_nullable();
+ // Use guarded state if it is more precise then what we already have.
+ if (nullable_cid == kDynamicCid) {
+ nullable_cid = field.guarded_cid();
+ used_guarded_state = true;
+ }
+ if (is_nullable && !field.is_nullable()) {
+ is_nullable = false;
+ used_guarded_state = true;
+ }
+ }
+
+ const Slot& slot = SlotCache::Instance(thread).Canonicalize(
+ Slot(Kind::kDartField,
+ IsImmutableBit::encode(field.is_final() || field.is_const()) |
+ IsNullableBit::encode(is_nullable) |
+ IsGuardedBit::encode(used_guarded_state),
+ nullable_cid, field.Offset(), &field,
+ &AbstractType::ZoneHandle(zone, field.type())));
+
+ // If properties of this slot were based on the guarded state make sure
+ // to add the field to the list of guarded fields. Note that during background
+ // compilation we might have two field clones that have incompatible guarded
+ // state - however both of these clones would correspond to the same slot.
+ // That is why we check the is_guarded_field() property of the slot rather
+ // than look at the current guarded state of the field, because current
+ // guarded state of the field might be set to kDynamicCid, while it was
+ // set to something more concrete when the slot was created.
+ // Note that we could have created this slot during an unsuccessful inlining
+ // attempt where we built and discarded the graph, in this case guarded
+ // fields associated with that graph are also discarded. However the slot
+ // itself stays behind in the compilation global cache. Thus we must always
+ // try to add it to the list of guarded fields of the current function.
+ if (slot.is_guarded_field()) {
if (thread->isolate()->use_field_guards()) {
ASSERT(parsed_function != nullptr);
- parsed_function->AddToGuardedFields(&field);
+ parsed_function->AddToGuardedFields(&slot.field());
} else {
// In precompiled mode we use guarded_cid field for type information
// inferred by TFA.
@@ -152,12 +183,7 @@
}
}
- return SlotCache::Instance(thread).Canonicalize(
- Slot(Kind::kDartField,
- IsImmutableBit::encode(field.is_final() || field.is_const()) |
- IsNullableBit::encode(is_nullable),
- nullable_cid, field.Offset(), &field,
- &AbstractType::ZoneHandle(zone, field.type())));
+ return slot;
}
CompileType Slot::ComputeCompileType() const {
diff --git a/runtime/vm/compiler/backend/slot.h b/runtime/vm/compiler/backend/slot.h
index aef5482..d448e40 100644
--- a/runtime/vm/compiler/backend/slot.h
+++ b/runtime/vm/compiler/backend/slot.h
@@ -142,6 +142,10 @@
intptr_t nullable_cid() const { return cid_; }
intptr_t is_nullable() const { return IsNullableBit::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_); }
+
// Static type of the slots if any.
//
// A value that is read from the slot is guaranteed to be assignable to its
@@ -183,7 +187,8 @@
other.static_type_) {}
using IsImmutableBit = BitField<int8_t, bool, 0, 1>;
- using IsNullableBit = BitField<int8_t, bool, 1, 1>;
+ using IsNullableBit = BitField<int8_t, bool, IsImmutableBit::kNextBit, 1>;
+ using IsGuardedBit = BitField<int8_t, bool, IsNullableBit::kNextBit, 1>;
template <typename T>
const T* DataAs() const {
diff --git a/runtime/vm/compiler/backend/slot_test.cc b/runtime/vm/compiler/backend/slot_test.cc
new file mode 100644
index 0000000..9e81ee5
--- /dev/null
+++ b/runtime/vm/compiler/backend/slot_test.cc
@@ -0,0 +1,104 @@
+// Copyright (c) 2019, 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 "include/dart_api.h"
+
+#include "platform/globals.h"
+
+#include "vm/compiler/backend/slot.h"
+#include "vm/compiler/compiler_state.h"
+#include "vm/object.h"
+#include "vm/parser.h"
+#include "vm/symbols.h"
+#include "vm/unit_test.h"
+
+namespace dart {
+
+// This is a regression test for b/121271056: there might be a race between
+// background compiler and mutator where mutator changes guarded state of
+// the field after Slot was created from it. A situation is possible where we
+// have a clone of a field with its guarded state set to unknown, however
+// Slot::Get for this field returns a Slot created from the previous clone of
+// the same field with a known guarded state. In this case we must add *old*
+// clone from which the Slot was created to guarded fields and not the new
+// clone, because new clone has no guarded state to begin with and thus
+// ParsedFunction::AddToGuardedFields(...) would simply ignore it.
+// Such slots with inconsistent guarded state that are not in the current
+// list of guarded fields arise due to unsuccessful inlining attempts.
+// If we built and discard the graph, then guarded fields associated with
+// that graph are also discarded. However the slot itself stays behind in
+// the global cache.
+// Adding old clone would lead to correct rejection of the compilation
+// attempt because Slot type information is different from the current guarded
+// state of the field.
+TEST_CASE(SlotFromGuardedField) {
+ if (!FLAG_use_field_guards) {
+ return;
+ }
+
+ TransitionNativeToVM transition(thread);
+ Zone* zone = thread->zone();
+
+ // Setup: create dummy class, function and a field.
+ const Class& dummy_class = Class::Handle(Class::New(
+ Library::Handle(), String::Handle(Symbols::New(thread, "DummyClass")),
+ Script::Handle(), TokenPosition::kNoSource));
+ dummy_class.set_is_synthesized_class();
+
+ const Function& dummy_function = Function::ZoneHandle(
+ Function::New(String::Handle(Symbols::New(thread, "foo")),
+ RawFunction::kRegularFunction, false, false, false, false,
+ false, dummy_class, TokenPosition::kMinSource));
+
+ const Field& field = Field::Handle(
+ Field::New(String::Handle(Symbols::New(thread, "field")),
+ /*is_static=*/false, /*is_final=*/false, /*is_const=*/false,
+ /*is_reflectable=*/true, dummy_class, Object::dynamic_type(),
+ TokenPosition::kMinSource, TokenPosition::kMinSource));
+
+ // Set non-trivial guarded state on the field.
+ field.set_guarded_cid(kSmiCid);
+ field.set_is_nullable(false);
+
+ // Enter compiler state.
+ CompilerState compiler_state(thread);
+
+ const Field& field_clone_1 = Field::ZoneHandle(field.CloneFromOriginal());
+ const Field& field_clone_2 = Field::ZoneHandle(field.CloneFromOriginal());
+
+ // Check that Slot::Get() returns correctly canonicalized and configured
+ // slot that matches properties of the field.
+ ParsedFunction* parsed_function =
+ new (zone) ParsedFunction(thread, dummy_function);
+ const Slot& slot1 = Slot::Get(field_clone_1, parsed_function);
+ const Slot& slot2 = Slot::Get(field_clone_2, parsed_function);
+ EXPECT_EQ(&slot1, &slot2);
+ EXPECT(slot1.is_guarded_field());
+ EXPECT(!slot1.is_nullable());
+ EXPECT_EQ(kSmiCid, slot1.nullable_cid());
+
+ // Check that the field was added (once) to the list of guarded fields.
+ EXPECT_EQ(1, parsed_function->guarded_fields()->length());
+ EXPECT_EQ(parsed_function->guarded_fields()->At(0)->raw(),
+ field_clone_1.raw());
+
+ // 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);
+ const Field& field_clone_3 = Field::ZoneHandle(field.CloneFromOriginal());
+
+ // Slot::Get must return the same slot and add the field from which it
+ // was created to the guarded fields list.
+ ParsedFunction* parsed_function2 =
+ new (zone) ParsedFunction(thread, dummy_function);
+ const Slot& slot3 = Slot::Get(field_clone_3, parsed_function2);
+ EXPECT_EQ(&slot1, &slot3);
+ EXPECT_EQ(1, parsed_function2->guarded_fields()->length());
+ EXPECT_EQ(parsed_function2->guarded_fields()->At(0)->raw(),
+ field_clone_1.raw());
+}
+
+} // namespace dart
diff --git a/runtime/vm/compiler/compiler_sources.gni b/runtime/vm/compiler/compiler_sources.gni
index eee9017..1b6e95f 100644
--- a/runtime/vm/compiler/compiler_sources.gni
+++ b/runtime/vm/compiler/compiler_sources.gni
@@ -133,5 +133,6 @@
"backend/locations_helpers_test.cc",
"backend/loops_test.cc",
"backend/range_analysis_test.cc",
+ "backend/slot_test.cc",
"cha_test.cc",
]
