[compiler] Fix inconsistency between JSFunctionRef and FeedbackCellRef

It could happen that the information about the feedback vector cached in
a JSFunctionData disagreed with the current value of the function's
feedback cell. The inlining code wasn't prepared for that and a CHECK
could fail.

The CL fixes this by removing the caching of
has_feedback_vector and feedback_vector and by getting hold of the
bytecode array before fetching the feedback vector in inlining.

Bug: v8:12172, v8:7790
Change-Id: Ife3ab8872085d9496e6d1f34514114a086f653ad
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3148010
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76751}

src/compiler/access-builder.cc

@@ -1217,15 +1217,6 @@ FieldAccess AccessBuilder::ForDictionaryObjectHashIndex() {
   return access;
 }
 
-// static
-FieldAccess AccessBuilder::ForFeedbackCellValue() {
-  FieldAccess access = {kTaggedBase,      FeedbackCell::kValueOffset,
-                        Handle<Name>(),   MaybeHandle<Map>(),
-                        Type::Any(),      MachineType::TaggedPointer(),
-                        kFullWriteBarrier};
-  return access;
-}
-
 // static
 FieldAccess AccessBuilder::ForFeedbackCellInterruptBudget() {
   FieldAccess access = {kTaggedBase,

src/compiler/access-builder.h

@@ -339,7 +339,6 @@ class V8_EXPORT_PRIVATE AccessBuilder final
   static FieldAccess ForDictionaryObjectHashIndex();
 
   // Provides access to FeedbackCell fields.
-  static FieldAccess ForFeedbackCellValue();
   static FieldAccess ForFeedbackCellInterruptBudget();
 
   // Provides access to a FeedbackVector fields.

src/compiler/bytecode-graph-builder.cc

@@ -1052,7 +1052,7 @@ BytecodeGraphBuilder::BytecodeGraphBuilder(
       shared_info_(shared_info),
       bytecode_array_(shared_info.GetBytecodeArray()),
       feedback_cell_(feedback_cell),
-      feedback_vector_(feedback_cell.value().value()),
+      feedback_vector_(feedback_cell.feedback_vector().value()),
       invocation_frequency_(invocation_frequency),
       type_hint_lowering_(
           broker, jsgraph, feedback_vector_,

src/compiler/heap-refs.cc

@@ -40,7 +40,7 @@ namespace compiler {
 //
 // kBackgroundSerializedHeapObject: The underlying V8 object is a HeapObject
 //   and the data is an instance of the corresponding (most-specific) subclass,
-//   e.g.  JSFunctionData, which provides serialized information about the
+//   e.g. JSFunctionData, which provides serialized information about the
 //   object. Allows serialization from the background thread.
 //
 // kUnserializedHeapObject: The underlying V8 object is a HeapObject and the
@@ -534,10 +534,6 @@ class JSFunctionData : public JSObjectData {
 
   bool IsConsistentWithHeapState(JSHeapBroker* broker) const;
 
-  bool has_feedback_vector() const {
-    DCHECK(serialized_);
-    return has_feedback_vector_;
-  }
   bool has_initial_map() const {
     DCHECK(serialized_);
     return has_initial_map_;
@@ -575,10 +571,6 @@ class JSFunctionData : public JSObjectData {
     DCHECK(serialized_);
     return feedback_cell_;
   }
-  ObjectData* feedback_vector() const {
-    DCHECK(serialized_);
-    return feedback_vector_;
-  }
   int initial_map_instance_size_with_min_slack() const {
     DCHECK(serialized_);
     return initial_map_instance_size_with_min_slack_;
@@ -615,7 +607,6 @@ class JSFunctionData : public JSObjectData {
   using UsedFields = base::Flags<UsedField>;
   UsedFields used_fields_;
 
-  bool has_feedback_vector_ = false;
   ObjectData* prototype_or_initial_map_ = nullptr;
   bool has_initial_map_ = false;
   bool has_instance_prototype_ = false;
@@ -627,7 +618,6 @@ class JSFunctionData : public JSObjectData {
   ObjectData* instance_prototype_ =
       nullptr;  // Derives from prototype_or_initial_map_.
   ObjectData* shared_ = nullptr;
-  ObjectData* feedback_vector_ = nullptr;  // Derives from feedback_cell.
   ObjectData* feedback_cell_ = nullptr;
   int initial_map_instance_size_with_min_slack_;  // Derives from
                                                   // prototype_or_initial_map_.
@@ -864,13 +854,6 @@ void JSFunctionData::Cache(JSHeapBroker* broker) {
   FeedbackCell feedback_cell = function->raw_feedback_cell(kAcquireLoad);
   feedback_cell_ = broker->GetOrCreateData(feedback_cell, kAssumeMemoryFence);
 
-  ObjectData* maybe_feedback_vector = broker->GetOrCreateData(
-      feedback_cell.value(kAcquireLoad), kAssumeMemoryFence);
-  if (shared.is_compiled() && maybe_feedback_vector->IsFeedbackVector()) {
-    has_feedback_vector_ = true;
-    feedback_vector_ = maybe_feedback_vector;
-  }
-
 #ifdef DEBUG
   serialized_ = true;
 #endif  // DEBUG
@@ -947,22 +930,6 @@ bool JSFunctionData::IsConsistentWithHeapState(JSHeapBroker* broker) const {
     return false;
   }
 
-  if (has_used_field(kHasFeedbackVector) &&
-      has_feedback_vector_ != f->has_feedback_vector()) {
-    TRACE_BROKER_MISSING(broker, "JSFunction::has_feedback_vector");
-    return false;
-  }
-
-  if (has_feedback_vector_) {
-    if (has_used_field(kFeedbackVector) &&
-        *feedback_vector_->object() != f->feedback_vector()) {
-      TRACE_BROKER_MISSING(broker, "JSFunction::feedback_vector");
-      return false;
-    }
-  } else {
-    DCHECK_NULL(feedback_vector_);
-  }
-
   return true;
 }
 
@@ -1660,6 +1627,11 @@ void RecordConsistentJSFunctionViewDependencyIfNeeded(
 
 }  // namespace
 
+base::Optional<FeedbackVectorRef> JSFunctionRef::feedback_vector(
+    CompilationDependencies* dependencies) const {
+  return raw_feedback_cell(dependencies).feedback_vector();
+}
+
 int JSFunctionRef::InitialMapInstanceSizeWithMinSlack(
     CompilationDependencies* dependencies) const {
   if (data_->should_access_heap()) {
@@ -2148,6 +2120,7 @@ ScopeInfoRef ScopeInfoRef::OuterScopeInfo() const {
 HEAP_ACCESSOR_C(SharedFunctionInfo, Builtin, builtin_id)
 
 BytecodeArrayRef SharedFunctionInfoRef::GetBytecodeArray() const {
+  CHECK(HasBytecodeArray());
   BytecodeArray bytecode_array;
   if (!broker()->IsMainThread()) {
     bytecode_array = object()->GetBytecodeArray(broker()->local_isolate());
@@ -2171,12 +2144,9 @@ SharedFunctionInfo::Inlineability SharedFunctionInfoRef::GetInlineability()
                                           broker()->is_turboprop());
 }
 
-base::Optional<FeedbackVectorRef> FeedbackCellRef::value() const {
-  DisallowGarbageCollection no_gc;
+ObjectRef FeedbackCellRef::value() const {
   DCHECK(data_->should_access_heap());
-  Object value = object()->value(kAcquireLoad);
-  if (!value.IsFeedbackVector()) return base::nullopt;
-  return MakeRefAssumeMemoryFence(broker(), FeedbackVector::cast(value));
+  return MakeRefAssumeMemoryFence(broker(), object()->value(kAcquireLoad));
 }
 
 base::Optional<ObjectRef> MapRef::GetStrongValue(
@@ -2657,11 +2627,17 @@ base::Optional<ObjectRef> DescriptorArrayRef::GetStrongValue(
   return TryMakeRef(broker(), heap_object);
 }
 
+base::Optional<FeedbackVectorRef> FeedbackCellRef::feedback_vector() const {
+  ObjectRef contents = value();
+  if (!contents.IsFeedbackVector()) return {};
+  return contents.AsFeedbackVector();
+}
+
 base::Optional<SharedFunctionInfoRef> FeedbackCellRef::shared_function_info()
     const {
-  base::Optional<FeedbackVectorRef> feedback_vector = value();
-  if (!feedback_vector.has_value()) return {};
-  return feedback_vector->shared_function_info();
+  base::Optional<FeedbackVectorRef> vector = feedback_vector();
+  if (!vector.has_value()) return {};
+  return vector->shared_function_info();
 }
 
 SharedFunctionInfoRef FeedbackVectorRef::shared_function_info() const {
@@ -2773,8 +2749,6 @@ HEAP_BROKER_OBJECT_LIST(V)
     return result;                                                          \
   }
 
-JSFUNCTION_BIMODAL_ACCESSOR_WITH_DEP_RELEVANT_C(
-    bool, has_feedback_vector, JSFunctionData::kHasFeedbackVector, true)
 JSFUNCTION_BIMODAL_ACCESSOR_WITH_DEP_RELEVANT_C(bool, has_initial_map,
                                                 JSFunctionData::kHasInitialMap,
                                                 true)
@@ -2790,8 +2764,6 @@ JSFUNCTION_BIMODAL_ACCESSOR_WITH_DEP(Object, instance_prototype,
                                      JSFunctionData::kInstancePrototype)
 JSFUNCTION_BIMODAL_ACCESSOR_WITH_DEP(FeedbackCell, raw_feedback_cell,
                                      JSFunctionData::kFeedbackCell)
-JSFUNCTION_BIMODAL_ACCESSOR_WITH_DEP(FeedbackVector, feedback_vector,
-                                     JSFunctionData::kFeedbackVector)
 
 BIMODAL_ACCESSOR(JSFunction, Context, context)
 BIMODAL_ACCESSOR(JSFunction, NativeContext, native_context)

src/compiler/heap-refs.h

@@ -462,8 +462,8 @@ class V8_EXPORT_PRIVATE JSFunctionRef : public JSObjectRef {
   ContextRef context() const;
   NativeContextRef native_context() const;
   SharedFunctionInfoRef shared() const;
+  CodeRef code() const;
 
-  bool has_feedback_vector(CompilationDependencies* dependencies) const;
   bool has_initial_map(CompilationDependencies* dependencies) const;
   bool PrototypeRequiresRuntimeLookup(
       CompilationDependencies* dependencies) const;
@@ -472,12 +472,10 @@ class V8_EXPORT_PRIVATE JSFunctionRef : public JSObjectRef {
   MapRef initial_map(CompilationDependencies* dependencies) const;
   int InitialMapInstanceSizeWithMinSlack(
       CompilationDependencies* dependencies) const;
-  FeedbackVectorRef feedback_vector(
-      CompilationDependencies* dependencies) const;
   FeedbackCellRef raw_feedback_cell(
       CompilationDependencies* dependencies) const;
-
-  CodeRef code() const;
+  base::Optional<FeedbackVectorRef> feedback_vector(
+      CompilationDependencies* dependencies) const;
 };
 
 class RegExpBoilerplateDescriptionRef : public HeapObjectRef {
@@ -614,8 +612,12 @@ class FeedbackCellRef : public HeapObjectRef {
   DEFINE_REF_CONSTRUCTOR(FeedbackCell, HeapObjectRef)
 
   Handle<FeedbackCell> object() const;
+
+  ObjectRef value() const;
+
+  // Convenience wrappers around {value()}:
+  base::Optional<FeedbackVectorRef> feedback_vector() const;
   base::Optional<SharedFunctionInfoRef> shared_function_info() const;
-  base::Optional<FeedbackVectorRef> value() const;
 };
 
 class FeedbackVectorRef : public HeapObjectRef {

src/compiler/js-call-reducer.cc

@@ -4453,7 +4453,8 @@ Reduction JSCallReducer::ReduceJSCall(Node* node) {
   } else if (feedback_target.has_value() && feedback_target->IsFeedbackCell()) {
     FeedbackCellRef feedback_cell =
         MakeRef(broker(), feedback_target.value().AsFeedbackCell().object());
-    if (feedback_cell.value().has_value()) {
+    // TODO(neis): This check seems unnecessary.
+    if (feedback_cell.feedback_vector().has_value()) {
       // Check that {target} is a closure with given {feedback_cell},
       // which uniquely identifies a given function inside a native context.
       Node* target_closure = effect =

src/compiler/js-inlining-heuristic.cc

@@ -28,7 +28,24 @@ bool IsSmall(int const size) {
 
 bool CanConsiderForInlining(JSHeapBroker* broker,
                             SharedFunctionInfoRef const& shared,
-                            FeedbackVectorRef const& feedback_vector) {
+                            FeedbackCellRef const& feedback_cell) {
+  if (!shared.HasBytecodeArray()) {
+    TRACE("Cannot consider " << shared << " for inlining (no bytecode)");
+    return false;
+  }
+  // Ensure we have a persistent handle to the bytecode in order to avoid
+  // flushing it during the remaining compilation.
+  shared.GetBytecodeArray();
+
+  base::Optional<FeedbackVectorRef> feedback_vector =
+      feedback_cell.feedback_vector();
+  if (!feedback_vector.has_value()) {
+    TRACE("Cannot consider " << shared << " for inlining (no feedback vector)");
+    return false;
+  }
+  // Note that due to prevented bytecode flushing, this {feedback_cell} won't
+  // change its value anymore during compilation.
+
   SharedFunctionInfo::Inlineability inlineability = shared.GetInlineability();
   if (inlineability != SharedFunctionInfo::kIsInlineable) {
     TRACE("Cannot consider "
@@ -36,22 +53,15 @@ bool CanConsiderForInlining(JSHeapBroker* broker,
     return false;
   }
 
-  DCHECK(shared.HasBytecodeArray());
-  TRACE("Considering " << shared << " for inlining with " << feedback_vector);
+  TRACE("Considering " << shared << " for inlining with " << *feedback_vector);
   return true;
 }
 
 bool CanConsiderForInlining(JSHeapBroker* broker,
                             JSFunctionRef const& function) {
-  if (!function.has_feedback_vector(broker->dependencies())) {
-    TRACE("Cannot consider " << function
-                             << " for inlining (no feedback vector)");
-    return false;
-  }
-
   return CanConsiderForInlining(
       broker, function.shared(),
-      function.feedback_vector(broker->dependencies()));
+      function.raw_feedback_cell(broker->dependencies()));
 }
 
 }  // namespace
@@ -98,9 +108,10 @@ JSInliningHeuristic::Candidate JSInliningHeuristic::CollectFunctions(
   if (m.IsCheckClosure()) {
     DCHECK(!out.functions[0].has_value());
     FeedbackCellRef feedback_cell = MakeRef(broker(), FeedbackCellOf(m.op()));
-    SharedFunctionInfoRef shared_info = *feedback_cell.shared_function_info();
+    SharedFunctionInfoRef shared_info =
+        feedback_cell.shared_function_info().value();
     out.shared_info = shared_info;
-    if (CanConsiderForInlining(broker(), shared_info, *feedback_cell.value())) {
+    if (CanConsiderForInlining(broker(), shared_info, feedback_cell)) {
       out.bytecode[0] = shared_info.GetBytecodeArray();
     }
     out.num_functions = 1;
@@ -113,8 +124,7 @@ JSInliningHeuristic::Candidate JSInliningHeuristic::CollectFunctions(
     FeedbackCellRef feedback_cell = n.GetFeedbackCellRefChecked(broker());
     SharedFunctionInfoRef shared_info = p.shared_info(broker());
     out.shared_info = shared_info;
-    if (feedback_cell.value().has_value() &&
-        CanConsiderForInlining(broker(), shared_info, *feedback_cell.value())) {
+    if (CanConsiderForInlining(broker(), shared_info, feedback_cell)) {
       out.bytecode[0] = shared_info.GetBytecodeArray();
     }
     out.num_functions = 1;

src/compiler/js-inlining.cc

@@ -305,7 +305,7 @@ base::Optional<SharedFunctionInfoRef> JSInliner::DetermineCallTarget(
     JSFunctionRef function = match.Ref(broker()).AsJSFunction();
 
     // The function might have not been called yet.
-    if (!function.has_feedback_vector(broker()->dependencies())) {
+    if (!function.feedback_vector(broker()->dependencies()).has_value()) {
       return base::nullopt;
     }
 
@@ -355,7 +355,7 @@ FeedbackCellRef JSInliner::DetermineCallContext(Node* node,
   if (match.HasResolvedValue() && match.Ref(broker()).IsJSFunction()) {
     JSFunctionRef function = match.Ref(broker()).AsJSFunction();
     // This was already ensured by DetermineCallTarget
-    CHECK(function.has_feedback_vector(broker()->dependencies()));
+    CHECK(function.feedback_vector(broker()->dependencies()).has_value());
 
     // The inlinee specializes to the context from the JSFunction object.
     *context_out = jsgraph()->Constant(function.context());

src/objects/feedback-cell-inl.h

@@ -40,7 +40,7 @@ void FeedbackCell::reset_feedback_vector(
   CHECK(value().IsFeedbackVector());
   ClosureFeedbackCellArray closure_feedback_cell_array =
       FeedbackVector::cast(value()).closure_feedback_cell_array();
-  set_value(closure_feedback_cell_array);
+  set_value(closure_feedback_cell_array, kReleaseStore);
   if (gc_notify_updated_slot) {
     (*gc_notify_updated_slot)(*this, RawField(FeedbackCell::kValueOffset),
                               closure_feedback_cell_array);