[compiler] Fix an issue with deprecated maps

Various field dependencies assume that the receiver map and the field
owner map agree on field meta data. That's not necessarily true when
the receiver map is already deprecated. We should skip over deprecated
maps.

- Fix a bug in SerializerForBackgroundCompilation. It used to process
  even deprecated maps.
- Fix a bug in FilterRelevantReceiverMaps. It used to store the original
  map rather than the new version.
- Turn some compilation dependency DCHECKs into CHECKs.
- CHECK in MapRef::FindFieldOwner that the map is not deprecated. While
  there might be valid use cases for calling the underlying
  Map::FindFieldOwner on a deprecated map, we never want to do that in
  the compiler.

Note that we skip any deprecated maps in JSNativeContextSpecialization's
ReduceNamedAccess. That's why I believe the issue could only be observed
with --concurrent-inlining and only in the form of a failing DCHECK.

Bug: chromium:1221812, v8:7790
Change-Id: I998b4ce1954be01eb6e0feb491ccc6b8306c685f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2976655
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Santiago Aboy Solanes <solanes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75294}

src/compiler/compilation-dependencies.cc

@@ -396,8 +396,8 @@ class FieldRepresentationDependency final : public CompilationDependency {
       : owner_(owner),
         descriptor_(descriptor),
         representation_(representation) {
-    DCHECK(owner_.equals(owner_.FindFieldOwner(descriptor_)));
-    DCHECK(representation_.Equals(
+    CHECK(owner_.equals(owner_.FindFieldOwner(descriptor_)));
+    CHECK(representation_.Equals(
         owner_.GetPropertyDetails(descriptor_).representation()));
   }
 
@@ -435,8 +435,8 @@ class FieldTypeDependency final : public CompilationDependency {
   FieldTypeDependency(const MapRef& owner, InternalIndex descriptor,
                       const ObjectRef& type)
       : owner_(owner), descriptor_(descriptor), type_(type) {
-    DCHECK(owner_.equals(owner_.FindFieldOwner(descriptor_)));
-    DCHECK(type_.equals(owner_.GetFieldType(descriptor_)));
+    CHECK(owner_.equals(owner_.FindFieldOwner(descriptor_)));
+    CHECK(type_.equals(owner_.GetFieldType(descriptor_)));
   }
 
   bool IsValid() const override {
@@ -463,9 +463,9 @@ class FieldConstnessDependency final : public CompilationDependency {
  public:
   FieldConstnessDependency(const MapRef& owner, InternalIndex descriptor)
       : owner_(owner), descriptor_(descriptor) {
-    DCHECK(owner_.equals(owner_.FindFieldOwner(descriptor_)));
-    DCHECK_EQ(PropertyConstness::kConst,
-              owner_.GetPropertyDetails(descriptor_).constness());
+    CHECK(owner_.equals(owner_.FindFieldOwner(descriptor_)));
+    CHECK_EQ(PropertyConstness::kConst,
+             owner_.GetPropertyDetails(descriptor_).constness());
   }
 
   bool IsValid() const override {
@@ -931,7 +931,7 @@ CompilationDependencies::FieldRepresentationDependencyOffTheRecord(
   MapRef owner = map.FindFieldOwner(descriptor);
   DCHECK(!owner.IsNeverSerializedHeapObject());
   PropertyDetails details = owner.GetPropertyDetails(descriptor);
-  DCHECK(details.representation().Equals(
+  CHECK(details.representation().Equals(
       map.GetPropertyDetails(descriptor).representation()));
   return zone_->New<FieldRepresentationDependency>(owner, descriptor,
                                                    details.representation());
@@ -944,7 +944,7 @@ CompilationDependencies::FieldTypeDependencyOffTheRecord(
   MapRef owner = map.FindFieldOwner(descriptor);
   DCHECK(!owner.IsNeverSerializedHeapObject());
   ObjectRef type = owner.GetFieldType(descriptor);
-  DCHECK(type.equals(map.GetFieldType(descriptor)));
+  CHECK(type.equals(map.GetFieldType(descriptor)));
   return zone_->New<FieldTypeDependency>(owner, descriptor, type);
 }
 

src/compiler/heap-refs.cc

@@ -3180,6 +3180,7 @@ bool MapRef::IsPrimitiveMap() const {
 
 MapRef MapRef::FindFieldOwner(InternalIndex descriptor_index) const {
   CHECK_LT(descriptor_index.as_int(), NumberOfOwnDescriptors());
+  CHECK(!is_deprecated());
   if (data_->should_access_heap() || broker()->is_concurrent_inlining()) {
     // TODO(solanes, v8:7790): Consider caching the result of the field owner on
     // the descriptor array. It would be useful for same map as well as any

src/compiler/js-heap-broker.cc

@@ -534,7 +534,7 @@ bool JSHeapBroker::FeedbackIsInsufficient(FeedbackSource const& source) const {
 
 namespace {
 
-// Remove unupdatable and abandoned prototype maps in-place.
+// Update deprecated maps, drop unupdatable ones and abandoned prototype maps.
 void FilterRelevantReceiverMaps(Isolate* isolate, MapHandles* maps) {
   auto in = maps->begin();
   auto out = in;
@@ -545,7 +545,7 @@ void FilterRelevantReceiverMaps(Isolate* isolate, MapHandles* maps) {
     if (Map::TryUpdate(isolate, map).ToHandle(&map) &&
         !map->is_abandoned_prototype_map()) {
       DCHECK(!map->is_deprecated());
-      *out = *in;
+      *out = map;
       ++out;
     }
   }

src/compiler/serializer-for-background-compilation.cc

@@ -500,7 +500,7 @@ class SerializerForBackgroundCompilation {
   void ProcessUnaryOrBinaryOperation(FeedbackSlot slot,
                                      bool honor_bailout_on_uninitialized);
 
-  PropertyAccessInfo ProcessMapForNamedPropertyAccess(
+  void ProcessMapForNamedPropertyAccess(
       Hints* receiver, base::Optional<MapRef> receiver_map,
       MapRef lookup_start_object_map, NameRef const& name,
       AccessMode access_mode, base::Optional<JSObjectRef> concrete_receiver,
@@ -2943,22 +2943,6 @@ void SerializerForBackgroundCompilation::VisitLdaLookupContextSlotInsideTypeof(
   ProcessLdaLookupContextSlot(iterator);
 }
 
-// TODO(neis): Avoid duplicating this.
-namespace {
-template <class MapContainer>
-MapHandles GetRelevantReceiverMaps(Isolate* isolate, MapContainer const& maps) {
-  MapHandles result;
-  for (Handle<Map> map : maps) {
-    if (Map::TryUpdate(isolate, map).ToHandle(&map) &&
-        !map->is_abandoned_prototype_map()) {
-      DCHECK(!map->is_deprecated());
-      result.push_back(map);
-    }
-  }
-  return result;
-}
-}  // namespace
-
 void SerializerForBackgroundCompilation::ProcessCompareOperation(
     FeedbackSlot slot) {
   if (slot.IsInvalid() || feedback_vector().is_null()) return;
@@ -2990,13 +2974,23 @@ void SerializerForBackgroundCompilation::ProcessUnaryOrBinaryOperation(
   environment()->accumulator_hints() = Hints();
 }
 
-PropertyAccessInfo
-SerializerForBackgroundCompilation::ProcessMapForNamedPropertyAccess(
+void SerializerForBackgroundCompilation::ProcessMapForNamedPropertyAccess(
     Hints* receiver, base::Optional<MapRef> receiver_map,
     MapRef lookup_start_object_map, NameRef const& name, AccessMode access_mode,
     base::Optional<JSObjectRef> concrete_receiver, Hints* result_hints) {
   DCHECK_IMPLIES(concrete_receiver.has_value(), receiver_map.has_value());
 
+  {
+    Handle<Map> map;
+    if (!Map::TryUpdate(broker()->isolate(), lookup_start_object_map.object())
+             .ToHandle(&map) ||
+        map->is_abandoned_prototype_map()) {
+      return;
+    }
+    lookup_start_object_map = MakeRef(broker(), map);
+  }
+  CHECK(!lookup_start_object_map.is_deprecated());
+
   // For JSNativeContextSpecialization::InferRootMap
   lookup_start_object_map.SerializeRootMap();
 
@@ -3114,8 +3108,6 @@ SerializerForBackgroundCompilation::ProcessMapForNamedPropertyAccess(
     case AccessMode::kHas:
       break;
   }
-
-  return access_info;
 }
 
 void SerializerForBackgroundCompilation::ProcessMinimorphicPropertyAccess(
@@ -3240,8 +3232,7 @@ void SerializerForBackgroundCompilation::ProcessNamedAccess(
     receiver->AddMap(map, zone(), broker_, false);
   }
 
-  for (Handle<Map> map :
-       GetRelevantReceiverMaps(broker()->isolate(), receiver->maps())) {
+  for (Handle<Map> map : receiver->maps()) {
     MapRef map_ref = MakeRef(broker(), map);
     ProcessMapForNamedPropertyAccess(receiver, map_ref, map_ref,
                                      feedback.name(), access_mode,
@@ -3275,8 +3266,7 @@ void SerializerForBackgroundCompilation::ProcessNamedAccess(
 void SerializerForBackgroundCompilation::ProcessNamedSuperAccess(
     Hints* receiver, NamedAccessFeedback const& feedback,
     AccessMode access_mode, Hints* result_hints) {
-  MapHandles receiver_maps =
-      GetRelevantReceiverMaps(broker()->isolate(), receiver->maps());
+  MapsSet receiver_maps = receiver->maps();
   for (Handle<Map> receiver_map : receiver_maps) {
     MapRef receiver_map_ref = MakeRef(broker(), receiver_map);
     for (Handle<Map> feedback_map : feedback.maps()) {
@@ -3286,7 +3276,7 @@ void SerializerForBackgroundCompilation::ProcessNamedSuperAccess(
           access_mode, base::nullopt, result_hints);
     }
   }
-  if (receiver_maps.empty()) {
+  if (receiver_maps.IsEmpty()) {
     for (Handle<Map> feedback_map : feedback.maps()) {
       MapRef feedback_map_ref = MakeRef(broker(), feedback_map);
       ProcessMapForNamedPropertyAccess(

test/mjsunit/compiler/regress-crbug-1221812.js

+// Copyright 2021 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: --allow-natives-syntax
+
+function C(a, b) {
+  this.a = a;
+  this.b = b;
+}
+
+var c = new C(42, 4.2);
+var cc = new C(4.2, 42);
+
+function foo() { return cc.a }
+
+%PrepareFunctionForOptimization(foo);
+assertEquals(4.2, foo());
+
+// Transition representation of 'a' from Double to Tagged.
+Object.defineProperty(c, 'a', { value: undefined });
+
+%OptimizeFunctionOnNextCall(foo);
+assertEquals(4.2, foo());