[runtime] Fix sorted order of DescriptorArray entries

... and add respective regression tests.

This CL also adds similar regression tests for TransitionArray but it
doesn't have the same issue as DescriptorArray.

Bug: chromium:1133527
Change-Id: I668a90f126d76af0a39816ce8697cb29bc65d01b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2465833Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70570}

src/codegen/code-stub-assembler.cc

@@ -1832,12 +1832,13 @@ TNode<IntPtrT> CodeStubAssembler::LoadJSReceiverIdentityHash(
   return var_hash.value();
 }
 
-TNode<Uint32T> CodeStubAssembler::LoadNameHashField(SloppyTNode<Name> name) {
-  CSA_ASSERT(this, IsName(name));
-  return LoadObjectField<Uint32T>(name, Name::kHashFieldOffset);
+TNode<Uint32T> CodeStubAssembler::LoadNameHashAssumeComputed(TNode<Name> name) {
+  TNode<Uint32T> hash_field = LoadNameHashField(name);
+  CSA_ASSERT(this, IsClearWord32(hash_field, Name::kHashNotComputedMask));
+  return Unsigned(Word32Shr(hash_field, Int32Constant(Name::kHashShift)));
 }
 
-TNode<Uint32T> CodeStubAssembler::LoadNameHash(SloppyTNode<Name> name,
+TNode<Uint32T> CodeStubAssembler::LoadNameHash(TNode<Name> name,
                                                Label* if_hash_not_computed) {
   TNode<Uint32T> hash_field = LoadNameHashField(name);
   if (if_hash_not_computed != nullptr) {
@@ -8110,7 +8111,7 @@ void CodeStubAssembler::LookupBinary(TNode<Name> unique_name,
   TNode<Uint32T> limit =
       Unsigned(Int32Sub(NumberOfEntries<Array>(array), Int32Constant(1)));
   TVARIABLE(Uint32T, var_high, limit);
-  TNode<Uint32T> hash = LoadNameHashField(unique_name);
+  TNode<Uint32T> hash = LoadNameHashAssumeComputed(unique_name);
   CSA_ASSERT(this, Word32NotEqual(hash, Int32Constant(0)));
 
   // Assume non-empty array.
@@ -8128,7 +8129,7 @@ void CodeStubAssembler::LookupBinary(TNode<Name> unique_name,
     TNode<Uint32T> sorted_key_index = GetSortedKeyIndex<Array>(array, mid);
     TNode<Name> mid_name = GetKey<Array>(array, sorted_key_index);
 
-    TNode<Uint32T> mid_hash = LoadNameHashField(mid_name);
+    TNode<Uint32T> mid_hash = LoadNameHashAssumeComputed(mid_name);
 
     Label mid_greater(this), mid_less(this), merge(this);
     Branch(Uint32GreaterThanOrEqual(mid_hash, hash), &mid_greater, &mid_less);
@@ -8155,7 +8156,7 @@ void CodeStubAssembler::LookupBinary(TNode<Name> unique_name,
     TNode<Uint32T> sort_index =
         GetSortedKeyIndex<Array>(array, var_low.value());
     TNode<Name> current_name = GetKey<Array>(array, sort_index);
-    TNode<Uint32T> current_hash = LoadNameHashField(current_name);
+    TNode<Uint32T> current_hash = LoadNameHashAssumeComputed(current_name);
     GotoIf(Word32NotEqual(current_hash, hash), if_not_found);
     Label next(this);
     GotoIf(TaggedNotEqual(current_name, unique_name), &next);

src/codegen/code-stub-assembler.h

@@ -1268,13 +1268,12 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
   // Check if the map is set for slow properties.
   TNode<BoolT> IsDictionaryMap(SloppyTNode<Map> map);
 
-  // Load the hash field of a name as an uint32 value.
-  TNode<Uint32T> LoadNameHashField(SloppyTNode<Name> name);
-  // Load the hash value of a name as an uint32 value.
+  // Load the Name::hash() value of a name as an uint32 value.
   // If {if_hash_not_computed} label is specified then it also checks if
   // hash is actually computed.
-  TNode<Uint32T> LoadNameHash(SloppyTNode<Name> name,
+  TNode<Uint32T> LoadNameHash(TNode<Name> name,
                               Label* if_hash_not_computed = nullptr);
+  TNode<Uint32T> LoadNameHashAssumeComputed(TNode<Name> name);
 
   // Load length field of a String object as Smi value.
   TNode<Smi> LoadStringLengthAsSmi(TNode<String> string);

src/diagnostics/objects-debug.cc

@@ -1726,12 +1726,13 @@ bool DescriptorArray::IsSortedNoDuplicates() {
   uint32_t current = 0;
   for (int i = 0; i < number_of_descriptors(); i++) {
     Name key = GetSortedKey(i);
+    CHECK(key.HasHashCode());
     if (key == current_key) {
       Print();
       return false;
     }
     current_key = key;
-    uint32_t hash = GetSortedKey(i).Hash();
+    uint32_t hash = key.hash();
     if (hash < current) {
       Print();
       return false;
@@ -1749,7 +1750,8 @@ bool TransitionArray::IsSortedNoDuplicates() {
 
   for (int i = 0; i < number_of_transitions(); i++) {
     Name key = GetSortedKey(i);
-    uint32_t hash = key.Hash();
+    CHECK(key.HasHashCode());
+    uint32_t hash = key.hash();
     PropertyKind kind = kData;
     PropertyAttributes attributes = NONE;
     if (!TransitionsAccessor::IsSpecialTransition(key.GetReadOnlyRoots(),

src/objects/descriptor-array-inl.h

@@ -227,7 +227,7 @@ void DescriptorArray::Append(Descriptor* desc) {
 
   for (insertion = descriptor_number; insertion > 0; --insertion) {
     Name key = GetSortedKey(insertion - 1);
-    if (key.Hash() <= hash) break;
+    if (key.hash() <= hash) break;
     SetSortedKey(insertion, GetSortedKeyIndex(insertion - 1));
   }
 

src/objects/descriptor-array.h

@@ -113,7 +113,7 @@ class DescriptorArray
       int slack = 0);
 
   // Sort the instance descriptors by the hash codes of their keys.
-  void Sort();
+  V8_EXPORT_PRIVATE void Sort();
 
   // Search the instance descriptors for given name. {concurrent_search} signals
   // if we are doing the search on a background thread. If so, we will sacrifice

src/objects/fixed-array-inl.h

@@ -233,7 +233,7 @@ int BinarySearch(T* array, Name name, int valid_entries,
   // index). After doing the binary search and getting the correct internal
   // index we check to have the index lower than valid_entries, if needed.
   int high = array->number_of_entries() - 1;
-  uint32_t hash = name.hash_field();
+  uint32_t hash = name.hash();
   int limit = high;
 
   DCHECK(low <= high);
@@ -241,7 +241,7 @@ int BinarySearch(T* array, Name name, int valid_entries,
   while (low != high) {
     int mid = low + (high - low) / 2;
     Name mid_name = array->GetSortedKey(mid);
-    uint32_t mid_hash = mid_name.hash_field();
+    uint32_t mid_hash = mid_name.hash();
 
     if (mid_hash >= hash) {
       high = mid;
@@ -253,7 +253,7 @@ int BinarySearch(T* array, Name name, int valid_entries,
   for (; low <= limit; ++low) {
     int sort_index = array->GetSortedKeyIndex(low);
     Name entry = array->GetKey(InternalIndex(sort_index));
-    uint32_t current_hash = entry.hash_field();
+    uint32_t current_hash = entry.hash();
     if (current_hash != hash) {
       // 'search_mode == ALL_ENTRIES' here and below is not needed since
       // 'out_insertion_index != nullptr' implies 'search_mode == ALL_ENTRIES'.
@@ -285,12 +285,12 @@ template <SearchMode search_mode, typename T>
 int LinearSearch(T* array, Name name, int valid_entries,
                  int* out_insertion_index) {
   if (search_mode == ALL_ENTRIES && out_insertion_index != nullptr) {
-    uint32_t hash = name.hash_field();
+    uint32_t hash = name.hash();
     int len = array->number_of_entries();
     for (int number = 0; number < len; number++) {
       int sorted_index = array->GetSortedKeyIndex(number);
       Name entry = array->GetKey(InternalIndex(sorted_index));
-      uint32_t current_hash = entry.hash_field();
+      uint32_t current_hash = entry.hash();
       if (current_hash > hash) {
         *out_insertion_index = sorted_index;
         return T::kNotFound;

src/objects/name-inl.h

@@ -94,6 +94,12 @@ uint32_t Name::Hash() {
   return String::cast(*this).ComputeAndSetHash();
 }
 
+uint32_t Name::hash() const {
+  uint32_t field = hash_field();
+  DCHECK(IsHashFieldComputed(field));
+  return field >> kHashShift;
+}
+
 DEF_GETTER(Name, IsInterestingSymbol, bool) {
   return IsSymbol(isolate) && Symbol::cast(*this).is_interesting_symbol();
 }

src/objects/name.h

@@ -23,9 +23,15 @@ class Name : public TorqueGeneratedName<Name, PrimitiveHeapObject> {
   // Tells whether the hash code has been computed.
   inline bool HasHashCode();
 
-  // Returns a hash value used for the property table
+  // Returns a hash value used for the property table. Ensures that the hash
+  // value is computed.
+  // TODO(ishell): rename to EnsureHash().
   inline uint32_t Hash();
 
+  // Returns a hash value used for the property table (same as Hash()), assumes
+  // the hash is already computed.
+  inline uint32_t hash() const;
+
   // Equality operations.
   inline bool Equals(Name other);
   inline static bool Equals(Isolate* isolate, Handle<Name> one,

src/objects/objects.cc

@@ -4362,16 +4362,16 @@ void DescriptorArray::Sort() {
   // Reset sorting since the descriptor array might contain invalid pointers.
   for (int i = 0; i < len; ++i) SetSortedKey(i, i);
   // Bottom-up max-heap construction.
-  // Index of the last node with children
+  // Index of the last node with children.
   const int max_parent_index = (len / 2) - 1;
   for (int i = max_parent_index; i >= 0; --i) {
     int parent_index = i;
-    const uint32_t parent_hash = GetSortedKey(i).Hash();
+    const uint32_t parent_hash = GetSortedKey(i).hash();
     while (parent_index <= max_parent_index) {
       int child_index = 2 * parent_index + 1;
-      uint32_t child_hash = GetSortedKey(child_index).Hash();
+      uint32_t child_hash = GetSortedKey(child_index).hash();
       if (child_index + 1 < len) {
-        uint32_t right_child_hash = GetSortedKey(child_index + 1).Hash();
+        uint32_t right_child_hash = GetSortedKey(child_index + 1).hash();
         if (right_child_hash > child_hash) {
           child_index++;
           child_hash = right_child_hash;
@@ -4390,13 +4390,13 @@ void DescriptorArray::Sort() {
     SwapSortedKeys(0, i);
     // Shift down the new top element.
     int parent_index = 0;
-    const uint32_t parent_hash = GetSortedKey(parent_index).Hash();
+    const uint32_t parent_hash = GetSortedKey(parent_index).hash();
     const int max_parent_index = (i / 2) - 1;
     while (parent_index <= max_parent_index) {
       int child_index = parent_index * 2 + 1;
-      uint32_t child_hash = GetSortedKey(child_index).Hash();
+      uint32_t child_hash = GetSortedKey(child_index).hash();
       if (child_index + 1 < i) {
-        uint32_t right_child_hash = GetSortedKey(child_index + 1).Hash();
+        uint32_t right_child_hash = GetSortedKey(child_index + 1).hash();
         if (right_child_hash > child_hash) {
           child_index++;
           child_hash = right_child_hash;

src/objects/transitions-inl.h

@@ -177,12 +177,20 @@ int TransitionArray::SearchNameForTesting(Name name, int* out_insertion_index) {
   return SearchName(name, out_insertion_index);
 }
 
+Map TransitionArray::SearchAndGetTargetForTesting(
+    PropertyKind kind, Name name, PropertyAttributes attributes) {
+  return SearchAndGetTarget(kind, name, attributes);
+}
+
 int TransitionArray::SearchSpecial(Symbol symbol, int* out_insertion_index) {
   return SearchName(symbol, out_insertion_index);
 }
 
 int TransitionArray::SearchName(Name name, int* out_insertion_index) {
   DCHECK(name.IsUniqueName());
+  // The name is taken from DescriptorArray, so it must already has a computed
+  // hash.
+  DCHECK(name.HasHashCode());
   return internal::Search<ALL_ENTRIES>(this, name, number_of_entries(),
                                        out_insertion_index);
 }

src/objects/transitions.cc

@@ -645,8 +645,8 @@ void TransitionArray::Sort() {
         temp_kind = details.kind();
         temp_attributes = details.attributes();
       }
-      int cmp = CompareKeys(temp_key, temp_key.Hash(), temp_kind,
-                            temp_attributes, key, key.Hash(), kind, attributes);
+      int cmp = CompareKeys(temp_key, temp_key.hash(), temp_kind,
+                            temp_attributes, key, key.hash(), kind, attributes);
       if (cmp > 0) {
         SetKey(j + 1, temp_key);
         SetRawTarget(j + 1, temp_target);

src/objects/transitions.h

@@ -150,6 +150,8 @@ class V8_EXPORT_PRIVATE TransitionsAccessor {
 
   inline int Capacity();
 
+  inline TransitionArray transitions();
+
  private:
   friend class MarkCompactCollector;  // For HasSimpleTransitionTo.
   friend class TransitionArray;
@@ -182,8 +184,6 @@ class V8_EXPORT_PRIVATE TransitionsAccessor {
   void TraverseTransitionTreeInternal(TraverseCallback callback, void* data,
                                       DisallowHeapAllocation* no_gc);
 
-  inline TransitionArray transitions();
-
   Isolate* isolate_;
   Handle<Map> map_handle_;
   Map map_;
@@ -239,7 +239,7 @@ class TransitionArray : public WeakFixedArray {
   V8_EXPORT_PRIVATE bool IsSortedNoDuplicates();
 #endif
 
-  void Sort();
+  V8_EXPORT_PRIVATE void Sort();
 
   void PrintInternal(std::ostream& os);
 
@@ -268,6 +268,9 @@ class TransitionArray : public WeakFixedArray {
   inline int SearchNameForTesting(Name name,
                                   int* out_insertion_index = nullptr);
 
+  inline Map SearchAndGetTargetForTesting(PropertyKind kind, Name name,
+                                          PropertyAttributes attributes);
+
  private:
   friend class Factory;
   friend class MarkCompactCollector;
@@ -304,7 +307,7 @@ class TransitionArray : public WeakFixedArray {
   int Search(PropertyKind kind, Name name, PropertyAttributes attributes,
              int* out_insertion_index = nullptr);
 
-  Map SearchAndGetTarget(PropertyKind kind, Name name,
+  V8_EXPORT_PRIVATE Map SearchAndGetTarget(PropertyKind kind, Name name,
                                            PropertyAttributes attributes);
 
   // Search a non-property transition (like elements kind, observe or frozen

test/cctest/BUILD.gn

@@ -217,6 +217,7 @@ v8_source_set("cctest_sources") {
     "test-debug.cc",
     "test-decls.cc",
     "test-deoptimization.cc",
+    "test-descriptor-array.cc",
     "test-dictionary.cc",
     "test-diy-fp.cc",
     "test-double.cc",

test/cctest/test-transitions.h

@@ -27,6 +27,8 @@ class TestTransitionsAccessor : public TransitionsAccessor {
   }
 
   int Capacity() { return TransitionsAccessor::Capacity(); }
+
+  TransitionArray transitions() { return TransitionsAccessor::transitions(); }
 };
 
 }  // namespace internal