[turboprop] Don't use weak pointers across TryMigrateInstance calls
We shouldn't spill weak pointers onto the stack when calling functions
that can trigger GC. DynamicMapChecks operator was using feedback loaded
from the feedback vector across the TryMigrateInstance function call.
The feedback can be a weak pointer to receiver map for monomorphic cases
and TryMigrateInstance can trigger a GC. This cl fixes it by holding
a holding a strong reference to the feedback.
Bug: v8:10774,v8:10582,v8:9684
Change-Id: Ia36f4d8ad46421ae570f41439bc1f0875081deee
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2336804
Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Commit-Queue: Mythri Alle <mythria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69338}
diff --git a/src/compiler/effect-control-linearizer.cc b/src/compiler/effect-control-linearizer.cc
index dcf6832..14dd26b 100644
--- a/src/compiler/effect-control-linearizer.cc
+++ b/src/compiler/effect-control-linearizer.cc
@@ -266,7 +266,9 @@
Node* LoadFromSeqString(Node* receiver, Node* position, Node* is_one_byte);
Node* TruncateWordToInt32(Node* value);
Node* BuildIsWeakReferenceTo(Node* maybe_object, Node* value);
+ Node* BuildIsClearedWeakReference(Node* maybe_object);
Node* BuildIsStrongReference(Node* value);
+ Node* BuildStrongReferenceFromWeakReference(Node* value);
Node* SmiMaxValueConstant();
Node* SmiShiftBitsConstant();
void TransitionElementsTo(Node* node, Node* array, ElementsKind from,
@@ -281,12 +283,14 @@
// Helper functions used in LowerDynamicCheckMaps
void CheckPolymorphic(Node* feedback, Node* value_map, Node* handler,
- GraphAssemblerLabel<0>* migrate,
GraphAssemblerLabel<0>* done, Node* frame_state);
- void CheckMonomorphic(Node* feedback, Node* value_map, Node* handler,
- GraphAssemblerLabel<0>* done,
- GraphAssemblerLabel<0>* map_check_failed,
- Node* frame_state, int slot, Node* vector);
+ void ProcessMonomorphic(Node* handler, GraphAssemblerLabel<0>* done,
+ Node* frame_state, int slot, Node* vector);
+ void BranchOnICState(int slot_index, Node* vector, Node* value_map,
+ Node* frame_state, GraphAssemblerLabel<0>* monomorphic,
+ GraphAssemblerLabel<0>* maybe_poly,
+ GraphAssemblerLabel<0>* migrate, Node** strong_feedback,
+ Node** poly_array);
bool should_maintain_schedule() const {
return maintain_schedule_ == MaintainSchedule::kMaintain;
@@ -1879,7 +1883,6 @@
void EffectControlLinearizer::CheckPolymorphic(Node* feedback_slot,
Node* value_map, Node* handler,
- GraphAssemblerLabel<0>* migrate,
GraphAssemblerLabel<0>* done,
Node* frame_state) {
Node* feedback_slot_map =
@@ -1898,13 +1901,9 @@
{
Node* index = loop.PhiAt(0);
Node* check = __ Int32LessThan(index, length);
- if (migrate != nullptr) {
- __ GotoIfNot(check, migrate);
- } else {
- __ DeoptimizeIfNot(DeoptimizeKind::kBailout,
- DeoptimizeReason::kMissingMap, FeedbackSource(), check,
- frame_state, IsSafetyCheck::kCriticalSafetyCheck);
- }
+ __ DeoptimizeIfNot(DeoptimizeKind::kBailout, DeoptimizeReason::kMissingMap,
+ FeedbackSource(), check, frame_state,
+ IsSafetyCheck::kCriticalSafetyCheck);
Node* maybe_map = __ LoadElement(AccessBuilder::ForWeakFixedArrayElement(),
feedback_slot, index);
@@ -1929,19 +1928,10 @@
}
}
-void EffectControlLinearizer::CheckMonomorphic(
- Node* feedback, Node* value_map, Node* handler,
- GraphAssemblerLabel<0>* done, GraphAssemblerLabel<0>* map_check_failed,
- Node* frame_state, int slot, Node* vector) {
- Node* mono_check = BuildIsWeakReferenceTo(feedback, value_map);
- if (map_check_failed != nullptr) {
- __ GotoIfNot(mono_check, map_check_failed);
- } else {
- __ DeoptimizeIfNot(DeoptimizeKind::kBailout, DeoptimizeReason::kMissingMap,
- FeedbackSource(), mono_check, frame_state,
- IsSafetyCheck::kCriticalSafetyCheck);
- }
-
+void EffectControlLinearizer::ProcessMonomorphic(Node* handler,
+ GraphAssemblerLabel<0>* done,
+ Node* frame_state, int slot,
+ Node* vector) {
Node* feedback_slot_handler =
__ LoadField(AccessBuilder::ForFeedbackVectorSlot(slot + 1), vector);
Node* handler_check = __ TaggedEqual(handler, feedback_slot_handler);
@@ -1951,6 +1941,40 @@
__ Goto(done);
}
+void EffectControlLinearizer::BranchOnICState(
+ int slot_index, Node* vector, Node* value_map, Node* frame_state,
+ GraphAssemblerLabel<0>* monomorphic, GraphAssemblerLabel<0>* maybe_poly,
+ GraphAssemblerLabel<0>* migrate, Node** strong_feedback,
+ Node** poly_array) {
+ Node* feedback =
+ __ LoadField(AccessBuilder::ForFeedbackVectorSlot(slot_index), vector);
+
+ Node* mono_check = BuildIsWeakReferenceTo(feedback, value_map);
+ __ GotoIf(mono_check, monomorphic);
+
+ Node* is_strong_ref = BuildIsStrongReference(feedback);
+ if (migrate != nullptr) {
+ auto check_poly = __ MakeLabel();
+
+ __ GotoIf(is_strong_ref, &check_poly);
+ Node* is_cleared = BuildIsClearedWeakReference(feedback);
+ __ DeoptimizeIf(DeoptimizeKind::kBailout, DeoptimizeReason::kMissingMap,
+ FeedbackSource(), is_cleared, frame_state,
+ IsSafetyCheck::kCriticalSafetyCheck);
+ *strong_feedback = BuildStrongReferenceFromWeakReference(feedback);
+ __ Goto(migrate);
+
+ __ Bind(&check_poly);
+ } else {
+ __ DeoptimizeIfNot(DeoptimizeKind::kBailout, DeoptimizeReason::kMissingMap,
+ FeedbackSource(), is_strong_ref, frame_state,
+ IsSafetyCheck::kCriticalSafetyCheck);
+ }
+
+ *poly_array = feedback;
+ __ Goto(maybe_poly);
+}
+
void EffectControlLinearizer::LowerDynamicCheckMaps(Node* node,
Node* frame_state) {
DynamicCheckMapsParameters const& p =
@@ -1959,69 +1983,71 @@
FeedbackSource const& feedback = p.feedback();
Node* vector = __ HeapConstant(feedback.vector);
- Node* feedback_slot = __ LoadField(
- AccessBuilder::ForFeedbackVectorSlot(feedback.index()), vector);
Node* value_map = __ LoadField(AccessBuilder::ForMap(), value);
Node* handler = p.handler()->IsSmi()
? __ SmiConstant(Smi::ToInt(*p.handler()))
: __ HeapConstant(Handle<HeapObject>::cast(p.handler()));
auto done = __ MakeLabel();
- auto maybe_poly = __ MakeLabel();
// Emit monomorphic checks only if current state is monomorphic. In
// case the current state is polymorphic, and if we ever go back to
// monomorphic start, we will deopt and reoptimize the code.
if (p.state() == DynamicCheckMapsParameters::kMonomorphic) {
- CheckMonomorphic(feedback_slot, value_map, handler, &done, &maybe_poly,
- frame_state, feedback.index(), vector);
- } else {
- DCHECK(p.state() == DynamicCheckMapsParameters::kPolymorphic);
- __ Goto(&maybe_poly);
- }
+ auto monomorphic_map_match = __ MakeLabel();
+ auto maybe_poly = __ MakeLabel();
+ Node* strong_feedback;
+ Node* poly_array;
- __ Bind(&maybe_poly);
- {
- Node* is_poly_or_megamorphic = BuildIsStrongReference(feedback_slot);
- // If the IC state at code generation time is not monomorphic, we don't
- // handle monomorphic states and just deoptimize if IC transitions to
- // monomorphic. For polymorphic ICs it is not required to migrate deprecated
- // maps since ICs don't discard deprecated maps from feedback. Only generate
- // codeneed to migrate maps for Monomoprhic state.
- if (p.flags() & CheckMapsFlag::kTryMigrateInstance &&
- p.state() == DynamicCheckMapsParameters::kMonomorphic) {
- auto migrate = __ MakeDeferredLabel();
+ if (p.flags() & CheckMapsFlag::kTryMigrateInstance) {
+ auto map_check_failed = __ MakeDeferredLabel();
+ BranchOnICState(feedback.index(), vector, value_map, frame_state,
+ &monomorphic_map_match, &maybe_poly, &map_check_failed,
+ &strong_feedback, &poly_array);
- __ GotoIfNot(is_poly_or_megamorphic, &migrate);
- // TODO(mythria): ICs don't drop deprecated maps from feedback vector.
- // So it is not equired to migrate the instance for polymorphic case.
- // When we change dynamic map checks to check only four maps re-evaluate
- // if this is required.
- CheckPolymorphic(feedback_slot, value_map, handler, nullptr, &done,
- frame_state);
-
- __ Bind(&migrate);
+ __ Bind(&map_check_failed);
{
MigrateInstanceOrDeopt(value, value_map, frame_state, FeedbackSource(),
DeoptimizeReason::kMissingMap);
- Node* new_value_map = __ LoadField(AccessBuilder::ForMap(), value);
// Check if new map matches.
- CheckMonomorphic(feedback_slot, new_value_map, handler, &done, nullptr,
- frame_state, feedback.index(), vector);
+ Node* new_value_map = __ LoadField(AccessBuilder::ForMap(), value);
+ Node* mono_check = __ TaggedEqual(strong_feedback, new_value_map);
+ __ DeoptimizeIfNot(DeoptimizeKind::kBailout,
+ DeoptimizeReason::kMissingMap, FeedbackSource(),
+ mono_check, frame_state,
+ IsSafetyCheck::kCriticalSafetyCheck);
+ ProcessMonomorphic(handler, &done, frame_state, feedback.index(),
+ vector);
}
} else {
- DeoptimizeReason reason = DeoptimizeReason::kMissingMap;
- DeoptimizeKind kind = DeoptimizeKind::kBailout;
- if (p.state() != DynamicCheckMapsParameters::kMonomorphic) {
- reason = DeoptimizeReason::kTransitionedToMonomorphicIC;
- kind = DeoptimizeKind::kEager;
- }
- __ DeoptimizeIfNot(kind, reason, FeedbackSource(), is_poly_or_megamorphic,
- frame_state, IsSafetyCheck::kCriticalSafetyCheck);
- CheckPolymorphic(feedback_slot, value_map, handler, nullptr, &done,
- frame_state);
+ BranchOnICState(feedback.index(), vector, value_map, frame_state,
+ &monomorphic_map_match, &maybe_poly, nullptr,
+ &strong_feedback, &poly_array);
}
+
+ __ Bind(&monomorphic_map_match);
+ ProcessMonomorphic(handler, &done, frame_state, feedback.index(), vector);
+
+ __ Bind(&maybe_poly);
+ // TODO(mythria): ICs don't drop deprecated maps from feedback vector.
+ // So it is not equired to migrate the instance for polymorphic case.
+ // When we change dynamic map checks to check only four maps re-evaluate
+ // if this is required.
+ CheckPolymorphic(poly_array, value_map, handler, &done, frame_state);
+ } else {
+ DCHECK_EQ(p.state(), DynamicCheckMapsParameters::kPolymorphic);
+ Node* feedback_slot = __ LoadField(
+ AccessBuilder::ForFeedbackVectorSlot(feedback.index()), vector);
+ // If the IC state at code generation time is not monomorphic, we don't
+ // handle monomorphic states and just deoptimize if IC transitions to
+ // monomorphic. For polymorphic ICs it is not required to migrate deprecated
+ // maps since ICs don't discard deprecated maps from feedback.
+ Node* is_poly_or_megamorphic = BuildIsStrongReference(feedback_slot);
+ __ DeoptimizeIfNot(DeoptimizeReason::kTransitionedToMonomorphicIC,
+ FeedbackSource(), is_poly_or_megamorphic, frame_state,
+ IsSafetyCheck::kCriticalSafetyCheck);
+ CheckPolymorphic(feedback_slot, value_map, handler, &done, frame_state);
}
__ Bind(&done);
}
@@ -6412,6 +6438,13 @@
__ Int32Constant(kHeapObjectTag));
}
+Node* EffectControlLinearizer::BuildStrongReferenceFromWeakReference(
+ Node* maybe_object) {
+ return __ BitcastWordToTagged(
+ __ WordAnd(__ BitcastMaybeObjectToWord(maybe_object),
+ __ IntPtrConstant(~kWeakHeapObjectMask)));
+}
+
Node* EffectControlLinearizer::BuildIsWeakReferenceTo(Node* maybe_object,
Node* value) {
if (COMPRESS_POINTERS_BOOL) {
@@ -6427,6 +6460,12 @@
}
}
+Node* EffectControlLinearizer::BuildIsClearedWeakReference(Node* maybe_object) {
+ return __ Word32Equal(
+ TruncateWordToInt32(__ BitcastMaybeObjectToWord(maybe_object)),
+ __ Int32Constant(kClearedWeakHeapObjectLower32));
+}
+
#undef __
void LinearizeEffectControl(JSGraph* graph, Schedule* schedule, Zone* temp_zone,
diff --git a/src/compiler/graph-assembler.cc b/src/compiler/graph-assembler.cc
index a3da27a..ae999b6 100644
--- a/src/compiler/graph-assembler.cc
+++ b/src/compiler/graph-assembler.cc
@@ -730,6 +730,15 @@
condition, frame_state, effect(), control()));
}
+Node* GraphAssembler::DeoptimizeIf(DeoptimizeKind kind, DeoptimizeReason reason,
+ FeedbackSource const& feedback,
+ Node* condition, Node* frame_state,
+ IsSafetyCheck is_safety_check) {
+ return AddNode(graph()->NewNode(
+ common()->DeoptimizeIf(kind, reason, feedback, is_safety_check),
+ condition, frame_state, effect(), control()));
+}
+
Node* GraphAssembler::DeoptimizeIfNot(DeoptimizeKind kind,
DeoptimizeReason reason,
FeedbackSource const& feedback,
diff --git a/src/compiler/graph-assembler.h b/src/compiler/graph-assembler.h
index 1a33c9e..2b2dbb5 100644
--- a/src/compiler/graph-assembler.h
+++ b/src/compiler/graph-assembler.h
@@ -295,6 +295,10 @@
DeoptimizeReason reason, FeedbackSource const& feedback, Node* condition,
Node* frame_state,
IsSafetyCheck is_safety_check = IsSafetyCheck::kSafetyCheck);
+ Node* DeoptimizeIf(
+ DeoptimizeKind kind, DeoptimizeReason reason,
+ FeedbackSource const& feedback, Node* condition, Node* frame_state,
+ IsSafetyCheck is_safety_check = IsSafetyCheck::kSafetyCheck);
Node* DeoptimizeIfNot(
DeoptimizeKind kind, DeoptimizeReason reason,
FeedbackSource const& feedback, Node* condition, Node* frame_state,
diff --git a/test/cctest/heap/test-heap.cc b/test/cctest/heap/test-heap.cc
index 19652e6..70b8d4c 100644
--- a/test/cctest/heap/test-heap.cc
+++ b/test/cctest/heap/test-heap.cc
@@ -1263,6 +1263,63 @@
}
}
+// Tests that spill slots from optimized code don't have weak pointers.
+TEST(Regress10774) {
+ i::FLAG_allow_natives_syntax = true;
+ i::FLAG_dynamic_map_checks = true;
+#ifdef VERIFY_HEAP
+ i::FLAG_verify_heap = true;
+#endif
+
+ ManualGCScope manual_gc_scope;
+ CcTest::InitializeVM();
+ v8::Isolate* isolate = CcTest::isolate();
+ Isolate* i_isolate = CcTest::i_isolate();
+ Factory* factory = i_isolate->factory();
+ Heap* heap = i_isolate->heap();
+
+ {
+ v8::HandleScope scope(isolate);
+ // We want to generate optimized code with dynamic map check operator that
+ // migrates deprecated maps. To force this, we want the IC state to be
+ // monomorphic and the map in the feedback should be a migration target.
+ const char* source =
+ "function f(o) {"
+ " return o.b;"
+ "}"
+ "var o = {a:10, b:20};"
+ "var o1 = {a:10, b:20};"
+ "var o2 = {a:10, b:20};"
+ "%PrepareFunctionForOptimization(f);"
+ "f(o);"
+ "o1.b = 10.23;" // Deprecate O's map.
+ "f(o1);" // Install new map in IC
+ "f(o);" // Mark o's map as migration target
+ "%OptimizeFunctionOnNextCall(f);"
+ "f(o);";
+ CompileRun(source);
+
+ Handle<String> foo_name = factory->InternalizeUtf8String("f");
+ Handle<Object> func_value =
+ Object::GetProperty(i_isolate, i_isolate->global_object(), foo_name)
+ .ToHandleChecked();
+ CHECK(func_value->IsJSFunction());
+ Handle<JSFunction> fun = Handle<JSFunction>::cast(func_value);
+
+ Handle<String> obj_name = factory->InternalizeUtf8String("o2");
+ Handle<Object> obj_value =
+ Object::GetProperty(i_isolate, i_isolate->global_object(), obj_name)
+ .ToHandleChecked();
+
+ heap::SimulateFullSpace(heap->new_space());
+
+ Handle<JSObject> global(i_isolate->context().global_object(), i_isolate);
+ // O2 still has the deprecated map and the optimized code should migrate O2
+ // successfully. This shouldn't crash.
+ Execution::Call(i_isolate, fun, global, 1, &obj_value).ToHandleChecked();
+ }
+}
+
#ifndef V8_LITE_MODE
TEST(TestOptimizeAfterBytecodeFlushingCandidate) {
