[compiler] Fix a bug in PropertyCellData::Cache

Bug: chromium:1250660, v8:7790
Change-Id: If96ab8879f54549b3b3d92ef2b1c13344dca17b2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3171154
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76936}

src/compiler/heap-refs.cc

@@ -256,14 +256,10 @@ bool PropertyCellData::Cache(JSHeapBroker* broker) {
     }
   }
 
-  if (property_details.cell_type() == PropertyCellType::kConstant) {
-    Handle<Object> value_again =
-        broker->CanonicalPersistentHandle(cell->value(kAcquireLoad));
-    if (*value != *value_again) {
+  if (property_details.cell_type() == PropertyCellType::kInTransition) {
     DCHECK(!broker->IsMainThread());
     return false;
   }
-  }
 
   ObjectData* value_data = broker->TryGetOrCreateData(value);
   if (value_data == nullptr) {

src/compiler/js-native-context-specialization.cc

@@ -967,6 +967,7 @@ Reduction JSNativeContextSpecialization::ReduceGlobalAccess(
         break;
       }
       case PropertyCellType::kUndefined:
+      case PropertyCellType::kInTransition:
         UNREACHABLE();
     }
   }

src/objects/objects.cc

@@ -197,6 +197,8 @@ std::ostream& operator<<(std::ostream& os, PropertyCellType type) {
       return os << "ConstantType";
     case PropertyCellType::kMutable:
       return os << "Mutable";
+    case PropertyCellType::kInTransition:
+      return os << "InTransition";
   }
   UNREACHABLE();
 }
@@ -6532,6 +6534,8 @@ PropertyCellType PropertyCell::UpdatedType(Isolate* isolate,
       V8_FALLTHROUGH;
     case PropertyCellType::kMutable:
       return PropertyCellType::kMutable;
+    case PropertyCellType::kInTransition:
+      UNREACHABLE();
   }
 }
 
@@ -6587,6 +6591,7 @@ bool PropertyCell::CheckDataIsCompatible(PropertyDetails details,
                                          Object value) {
   DisallowGarbageCollection no_gc;
   PropertyCellType cell_type = details.cell_type();
+  CHECK_NE(cell_type, PropertyCellType::kInTransition);
   if (value.IsTheHole()) {
     CHECK_EQ(cell_type, PropertyCellType::kConstant);
   } else {
@@ -6620,8 +6625,9 @@ bool PropertyCell::CanTransitionTo(PropertyDetails new_details,
       return new_details.cell_type() == PropertyCellType::kMutable ||
              (new_details.cell_type() == PropertyCellType::kConstant &&
               new_value.IsTheHole());
-  }
+    case PropertyCellType::kInTransition:
       UNREACHABLE();
+  }
 }
 #endif  // DEBUG
 

src/objects/property-cell-inl.h

@@ -57,6 +57,9 @@ void PropertyCell::Transition(PropertyDetails new_details,
   DCHECK(CanTransitionTo(new_details, *new_value));
   // This code must be in sync with its counterpart in
   // PropertyCellData::Serialize.
+  PropertyDetails transition_marker = new_details;
+  transition_marker.set_cell_type(PropertyCellType::kInTransition);
+  set_property_details_raw(transition_marker.AsSmi(), kReleaseStore);
   set_value(*new_value, kReleaseStore);
   set_property_details_raw(new_details.AsSmi(), kReleaseStore);
 }

src/objects/property-details.h

@@ -242,6 +242,9 @@ enum class PropertyCellType {
   kUndefined,     // The PREMONOMORPHIC of property cells.
   kConstant,      // Cell has been assigned only once.
   kConstantType,  // Cell has been assigned only one type.
+  // Temporary value indicating an ongoing property cell state transition. Only
+  // observable by a background thread.
+  kInTransition,
   // Value for dictionaries not holding cells, must be 0:
   kNoCell = kMutable,
 };
@@ -381,8 +384,7 @@ class PropertyDetails {
   // Bit fields in value_ (type, shift, size). Must be public so the
   // constants can be embedded in generated code.
   using KindField = base::BitField<PropertyKind, 0, 1>;
-  using LocationField = KindField::Next<PropertyLocation, 1>;
-  using ConstnessField = LocationField::Next<PropertyConstness, 1>;
+  using ConstnessField = KindField::Next<PropertyConstness, 1>;
   using AttributesField = ConstnessField::Next<PropertyAttributes, 3>;
   static const int kAttributesReadOnlyMask =
       (READ_ONLY << AttributesField::kShift);
@@ -392,11 +394,12 @@ class PropertyDetails {
       (DONT_ENUM << AttributesField::kShift);
 
   // Bit fields for normalized/dictionary mode objects.
-  using PropertyCellTypeField = AttributesField::Next<PropertyCellType, 2>;
+  using PropertyCellTypeField = AttributesField::Next<PropertyCellType, 3>;
   using DictionaryStorageField = PropertyCellTypeField::Next<uint32_t, 23>;
 
   // Bit fields for fast objects.
-  using RepresentationField = AttributesField::Next<uint32_t, 3>;
+  using LocationField = AttributesField::Next<PropertyLocation, 1>;
+  using RepresentationField = LocationField::Next<uint32_t, 3>;
   using DescriptorPointer =
       RepresentationField::Next<uint32_t, kDescriptorIndexBitCount>;
   using FieldIndexField =
@@ -415,7 +418,6 @@ class PropertyDetails {
   STATIC_ASSERT(KindField::kLastUsedBit < 8);
   STATIC_ASSERT(ConstnessField::kLastUsedBit < 8);
   STATIC_ASSERT(AttributesField::kLastUsedBit < 8);
-  STATIC_ASSERT(LocationField::kLastUsedBit < 8);
 
   static const int kInitialIndex = 1;
 
@@ -445,12 +447,12 @@ class PropertyDetails {
   // with an enumeration index of 0 as a single byte.
   uint8_t ToByte() {
     // We only care about the value of KindField, ConstnessField, and
-    // AttributesField. LocationField is also stored, but it will always be
-    // kField. We've statically asserted earlier that all those fields fit into
-    // a byte together.
+    // AttributesField. We've statically asserted earlier that these fields fit
+    // into a byte together.
+
+    DCHECK_EQ(PropertyLocation::kField, location());
+    STATIC_ASSERT(static_cast<int>(PropertyLocation::kField) == 0);
 
-    // PropertyCellTypeField comes next, its value must be kNoCell == 0 for
-    // dictionary mode PropertyDetails anyway.
     DCHECK_EQ(PropertyCellType::kNoCell, cell_type());
     STATIC_ASSERT(static_cast<int>(PropertyCellType::kNoCell) == 0);
 
@@ -464,16 +466,13 @@ class PropertyDetails {
   // Only to be used for bytes obtained by ToByte. In particular, only used for
   // non-global dictionary properties.
   static PropertyDetails FromByte(uint8_t encoded_details) {
-    // The 0-extension to 32bit sets PropertyCellType to kNoCell and
-    // enumeration index to 0, as intended. Everything else is obtained from
-    // |encoded_details|.
-
+    // The 0-extension to 32bit sets PropertyLocation to kField,
+    // PropertyCellType to kNoCell, and enumeration index to 0, as intended.
+    // Everything else is obtained from |encoded_details|.
     PropertyDetails details(encoded_details);
-
-    DCHECK_EQ(0, details.dictionary_index());
     DCHECK_EQ(PropertyLocation::kField, details.location());
     DCHECK_EQ(PropertyCellType::kNoCell, details.cell_type());
-
+    DCHECK_EQ(0, details.dictionary_index());
     return details;
   }