[ic] Use the existing prototype validity cell when recomputing handlers

For keyed stores we recompute handlers based on the receiver maps
we have seen. This is done so that we can transition to the most generic
elements kind we have seen so far. When we recompute this handlers we
get a new prototype validity cell and ignore the existing cell. This
leads to incorrect behaviour if the cell was invalid. Recomputing the
handler may be extra work which is not worth doing at this point. So
we just reuse the existing validity cell and let the IC recompute the
handler if we see the map again.

Bug: chromium:1053939
Change-Id: Ifc891d70f5a4b8b774238e12fb40e29b4d174e37
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2122032
Commit-Queue: Mythri Alle <mythria@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66963}

src/ic/handler-configuration.cc

@@ -200,11 +200,14 @@ KeyedAccessStoreMode StoreHandler::GetKeyedAccessStoreMode(
 // static
 Handle<Object> StoreHandler::StoreElementTransition(
     Isolate* isolate, Handle<Map> receiver_map, Handle<Map> transition,
-    KeyedAccessStoreMode store_mode) {
+    KeyedAccessStoreMode store_mode, MaybeHandle<Object> prev_validity_cell) {
   Handle<Code> stub =
       CodeFactory::ElementsTransitionAndStore(isolate, store_mode).code();
-  Handle<Object> validity_cell =
-      Map::GetOrCreatePrototypeChainValidityCell(receiver_map, isolate);
+  Handle<Object> validity_cell;
+  if (!prev_validity_cell.ToHandle(&validity_cell)) {
+    validity_cell =
+        Map::GetOrCreatePrototypeChainValidityCell(receiver_map, isolate);
+  }
   Handle<StoreHandler> handler = isolate->factory()->NewStoreHandler(1);
   handler->set_smi_handler(*stub);
   handler->set_validity_cell(*validity_cell);

src/ic/handler-configuration.h

@@ -275,10 +275,10 @@ class StoreHandler final : public DataHandler {
       MaybeObjectHandle maybe_data1 = MaybeObjectHandle(),
       MaybeObjectHandle maybe_data2 = MaybeObjectHandle());
 
-  static Handle<Object> StoreElementTransition(Isolate* isolate,
-                                               Handle<Map> receiver_map,
-                                               Handle<Map> transition,
-                                               KeyedAccessStoreMode store_mode);
+  static Handle<Object> StoreElementTransition(
+      Isolate* isolate, Handle<Map> receiver_map, Handle<Map> transition,
+      KeyedAccessStoreMode store_mode,
+      MaybeHandle<Object> prev_validity_cell = MaybeHandle<Object>());
 
   static Handle<Object> StoreProxy(Isolate* isolate, Handle<Map> receiver_map,
                                    Handle<JSProxy> proxy,

src/ic/ic.h

@@ -82,6 +82,8 @@ class IC {
   // Configure the vector for POLYMORPHIC.
   void ConfigureVectorState(Handle<Name> name, MapHandles const& maps,
                             MaybeObjectHandles* handlers);
+  void ConfigureVectorState(
+      Handle<Name> name, std::vector<MapAndHandler> const& maps_and_handlers);
 
   char TransitionMarkFromState(IC::State state);
   void TraceIC(const char* type, Handle<Object> name);
@@ -312,12 +314,13 @@ class KeyedStoreIC : public StoreIC {
   Handle<Map> ComputeTransitionedMap(Handle<Map> map,
                                      TransitionMode transition_mode);
 
-  Handle<Object> StoreElementHandler(Handle<Map> receiver_map,
-                                     KeyedAccessStoreMode store_mode);
+  Handle<Object> StoreElementHandler(
+      Handle<Map> receiver_map, KeyedAccessStoreMode store_mode,
+      MaybeHandle<Object> prev_validity_cell = MaybeHandle<Object>());
 
-  void StoreElementPolymorphicHandlers(MapHandles* receiver_maps,
-                                       MaybeObjectHandles* handlers,
-                                       KeyedAccessStoreMode store_mode);
+  void StoreElementPolymorphicHandlers(
+      std::vector<MapAndHandler>* receiver_maps_and_handlers,
+      KeyedAccessStoreMode store_mode);
 
   friend class IC;
 };

src/objects/feedback-vector.cc

@@ -916,11 +916,9 @@ void FeedbackNexus::ConfigureMonomorphic(Handle<Name> name,
   }
 }
 
-void FeedbackNexus::ConfigurePolymorphic(Handle<Name> name,
-                                         MapHandles const& maps,
-                                         MaybeObjectHandles* handlers) {
-  DCHECK_EQ(handlers->size(), maps.size());
-  int receiver_count = static_cast<int>(maps.size());
+void FeedbackNexus::ConfigurePolymorphic(
+    Handle<Name> name, std::vector<MapAndHandler> const& maps_and_handlers) {
+  int receiver_count = static_cast<int>(maps_and_handlers.size());
   DCHECK_GT(receiver_count, 1);
   Handle<WeakFixedArray> array;
   if (name.is_null()) {
@@ -933,10 +931,11 @@ void FeedbackNexus::ConfigurePolymorphic(Handle<Name> name,
   }
 
   for (int current = 0; current < receiver_count; ++current) {
-    Handle<Map> map = maps[current];
+    Handle<Map> map = maps_and_handlers[current].first;
     array->Set(current * 2, HeapObjectReference::Weak(*map));
-    DCHECK(IC::IsHandler(*handlers->at(current)));
-    array->Set(current * 2 + 1, *handlers->at(current));
+    MaybeObjectHandle handler = maps_and_handlers[current].second;
+    DCHECK(IC::IsHandler(*handler));
+    array->Set(current * 2 + 1, *handler);
   }
 }
 
@@ -982,11 +981,13 @@ int FeedbackNexus::ExtractMaps(MapHandles* maps) const {
   return 0;
 }
 
-int FeedbackNexus::ExtractMapsAndHandlers(MapHandles* maps,
-                                          MaybeObjectHandles* handlers) const {
-  DCHECK(IsLoadICKind(kind()) || IsStoreICKind(kind()) ||
-         IsKeyedLoadICKind(kind()) || IsKeyedStoreICKind(kind()) ||
-         IsStoreOwnICKind(kind()) || IsStoreDataPropertyInLiteralKind(kind()) ||
+int FeedbackNexus::ExtractMapsAndHandlers(
+    std::vector<std::pair<Handle<Map>, MaybeObjectHandle>>* maps_and_handlers,
+    bool drop_deprecated) const {
+  DCHECK(IsLoadICKind(kind()) ||
+         IsStoreICKind(kind()) | IsKeyedLoadICKind(kind()) ||
+         IsKeyedStoreICKind(kind()) || IsStoreOwnICKind(kind()) ||
+         IsStoreDataPropertyInLiteralKind(kind()) ||
          IsStoreInArrayLiteralICKind(kind()) || IsKeyedHasICKind(kind()));
 
   DisallowHeapAllocation no_gc;
@@ -1007,6 +1008,7 @@ int FeedbackNexus::ExtractMapsAndHandlers(MapHandles* maps,
     }
     const int increment = 2;
     HeapObject heap_object;
+    maps_and_handlers->reserve(array.length() / increment);
     for (int i = 0; i < array.length(); i += increment) {
       DCHECK(array.Get(i)->IsWeakOrCleared());
       if (array.Get(i)->GetHeapObjectIfWeak(&heap_object)) {
@@ -1014,8 +1016,9 @@ int FeedbackNexus::ExtractMapsAndHandlers(MapHandles* maps,
         if (!handler->IsCleared()) {
           DCHECK(IC::IsHandler(handler));
           Map map = Map::cast(heap_object);
-          maps->push_back(handle(map, isolate));
-          handlers->push_back(handle(handler, isolate));
+          if (drop_deprecated && map.is_deprecated()) continue;
+          maps_and_handlers->push_back(
+              MapAndHandler(handle(map, isolate), handle(handler, isolate)));
           found++;
         }
       }
@@ -1026,8 +1029,9 @@ int FeedbackNexus::ExtractMapsAndHandlers(MapHandles* maps,
     if (!handler->IsCleared()) {
       DCHECK(IC::IsHandler(handler));
       Map map = Map::cast(heap_object);
-      maps->push_back(handle(map, isolate));
-      handlers->push_back(handle(handler, isolate));
+      if (drop_deprecated && map.is_deprecated()) return 0;
+      maps_and_handlers->push_back(
+          MapAndHandler(handle(map, isolate), handle(handler, isolate)));
       return 1;
     }
   }
@@ -1099,14 +1103,14 @@ Name FeedbackNexus::GetName() const {
 
 KeyedAccessLoadMode FeedbackNexus::GetKeyedAccessLoadMode() const {
   DCHECK(IsKeyedLoadICKind(kind()) || IsKeyedHasICKind(kind()));
-  MapHandles maps;
-  MaybeObjectHandles handlers;
 
   if (GetKeyType() == PROPERTY) return STANDARD_LOAD;
 
-  ExtractMapsAndHandlers(&maps, &handlers);
-  for (MaybeObjectHandle const& handler : handlers) {
-    KeyedAccessLoadMode mode = LoadHandler::GetKeyedAccessLoadMode(*handler);
+  std::vector<MapAndHandler> maps_and_handlers;
+  ExtractMapsAndHandlers(&maps_and_handlers);
+  for (MapAndHandler map_and_handler : maps_and_handlers) {
+    KeyedAccessLoadMode mode =
+        LoadHandler::GetKeyedAccessLoadMode(*map_and_handler.second);
     if (mode != STANDARD_LOAD) return mode;
   }
 
@@ -1167,13 +1171,13 @@ KeyedAccessStoreMode FeedbackNexus::GetKeyedAccessStoreMode() const {
   DCHECK(IsKeyedStoreICKind(kind()) || IsStoreInArrayLiteralICKind(kind()) ||
          IsStoreDataPropertyInLiteralKind(kind()));
   KeyedAccessStoreMode mode = STANDARD_STORE;
-  MapHandles maps;
-  MaybeObjectHandles handlers;
 
   if (GetKeyType() == PROPERTY) return mode;
 
-  ExtractMapsAndHandlers(&maps, &handlers);
-  for (const MaybeObjectHandle& maybe_code_handler : handlers) {
+  std::vector<MapAndHandler> maps_and_handlers;
+  ExtractMapsAndHandlers(&maps_and_handlers);
+  for (const MapAndHandler& map_and_handler : maps_and_handlers) {
+    const MaybeObjectHandle maybe_code_handler = map_and_handler.second;
     // The first handler that isn't the slow handler will have the bits we need.
     Handle<Code> handler;
     if (maybe_code_handler.object()->IsStoreHandler()) {

src/objects/feedback-vector.h

@@ -59,6 +59,8 @@ enum class FeedbackSlotKind {
   kKindsNumber  // Last value indicating number of kinds.
 };
 
+using MapAndHandler = std::pair<Handle<Map>, MaybeObjectHandle>;
+
 inline bool IsCallICKind(FeedbackSlotKind kind) {
   return kind == FeedbackSlotKind::kCall;
 }
@@ -648,8 +650,8 @@ class V8_EXPORT_PRIVATE FeedbackNexus final {
   Map GetFirstMap() const;
 
   int ExtractMaps(MapHandles* maps) const;
-  int ExtractMapsAndHandlers(MapHandles* maps,
-                             MaybeObjectHandles* handlers) const;
+  int ExtractMapsAndHandlers(std::vector<MapAndHandler>* maps_and_handlers,
+                             bool drop_deprecated = false) const;
   MaybeObjectHandle FindHandlerForMap(Handle<Map> map) const;
 
   bool IsCleared() const {
@@ -673,8 +675,8 @@ class V8_EXPORT_PRIVATE FeedbackNexus final {
   void ConfigureMonomorphic(Handle<Name> name, Handle<Map> receiver_map,
                             const MaybeObjectHandle& handler);
 
-  void ConfigurePolymorphic(Handle<Name> name, MapHandles const& maps,
-                            MaybeObjectHandles* handlers);
+  void ConfigurePolymorphic(
+      Handle<Name> name, std::vector<MapAndHandler> const& maps_and_handlers);
 
   BinaryOperationHint GetBinaryOperationFeedback() const;
   CompareOperationHint GetCompareOperationFeedback() const;

test/mjsunit/regress/regress-crbug-1053939.js

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --no-lazy-feedback-allocation
+
+
+function foo(a, b) {
+  a[b] = 1;
+  return a[b];
+}
+v = [];
+assertEquals(foo(v, 1), 1);
+v.__proto__.__proto__ = new Int32Array();
+assertEquals(foo(Object(), 1), 1);
+assertEquals(foo(v, 2), undefined);