Reland "[string] Fix non-SeqStrings in IsEqualTo"

This is a reland of e70cbb83

Moved the ConsString comparison logic out-of-line, both to make gcc
happy, and to reduce the size of the fast-path in IsEqualToImpl.

Original change's description:
> [string] Fix non-SeqStrings in IsEqualTo
>
> Bug: chromium:1193903
> Change-Id: I80704dd3cba5754779432356b20bd3ea99630291
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2794426
> Commit-Queue: Leszek Swirski <leszeks@chromium.org>
> Commit-Queue: Igor Sheludko <ishell@chromium.org>
> Reviewed-by: Igor Sheludko <ishell@chromium.org>
> Auto-Submit: Leszek Swirski <leszeks@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#73746}

Bug: chromium:1193903
Change-Id: Iae6f078853438427e86d3ac68bcfed0712a85bf7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2797288
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73763}

src/objects/string-inl.h

@@ -9,6 +9,7 @@
 #include "src/common/external-pointer-inl.h"
 #include "src/common/external-pointer.h"
 #include "src/common/globals.h"
+#include "src/execution/isolate-utils.h"
 #include "src/handles/handles-inl.h"
 #include "src/heap/factory.h"
 #include "src/numbers/conversions-inl.h"
@@ -403,7 +404,8 @@ class SeqSubStringKey final : public StringTableKey {
     DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*string_));
     DisallowGarbageCollection no_gc;
     return string.IsEqualTo<String::EqualityType::kNoLengthCheck>(
-        Vector<const Char>(string_->GetChars(no_gc) + from_, length()));
+        Vector<const Char>(string_->GetChars(no_gc) + from_, length()),
+        isolate);
   }
 
   Handle<String> AsHandle(Isolate* isolate) {
@@ -454,19 +456,26 @@ bool String::Equals(Isolate* isolate, Handle<String> one, Handle<String> two) {
 template <String::EqualityType kEqType, typename Char>
 bool String::IsEqualTo(Vector<const Char> str, Isolate* isolate) const {
   DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this));
-  return IsEqualToImpl<kEqType>(str,
+  return IsEqualToImpl<kEqType>(str, isolate,
+                                SharedStringAccessGuardIfNeeded::NotNeeded());
+}
+
+template <String::EqualityType kEqType, typename Char>
+bool String::IsEqualTo(Vector<const Char> str) const {
+  DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this));
+  return IsEqualToImpl<kEqType>(str, GetIsolateForPtrCompr(*this),
                                 SharedStringAccessGuardIfNeeded::NotNeeded());
 }
 
 template <String::EqualityType kEqType, typename Char>
 bool String::IsEqualTo(Vector<const Char> str, LocalIsolate* isolate) const {
   SharedStringAccessGuardIfNeeded access_guard(isolate);
-  return IsEqualToImpl<kEqType>(str, access_guard);
+  return IsEqualToImpl<kEqType>(str, isolate, access_guard);
 }
 
 template <String::EqualityType kEqType, typename Char>
 bool String::IsEqualToImpl(
-    Vector<const Char> str,
+    Vector<const Char> str, IsolateRoot isolate,
     const SharedStringAccessGuardIfNeeded& access_guard) const {
   size_t len = str.size();
   switch (kEqType) {
@@ -483,60 +492,85 @@ bool String::IsEqualToImpl(
 
   DisallowGarbageCollection no_gc;
 
-  class IsEqualToDispatcher : public AllStatic {
-   public:
-    static inline bool HandleSeqOneByteString(
-        SeqOneByteString str, const Char* data, size_t len,
-        const DisallowGarbageCollection& no_gc,
-        const SharedStringAccessGuardIfNeeded& access_guard) {
-      return CompareCharsEqual(str.GetChars(no_gc, access_guard), data, len);
-    }
-    static inline bool HandleSeqTwoByteString(
-        SeqTwoByteString str, const Char* data, size_t len,
-        const DisallowGarbageCollection& no_gc,
-        const SharedStringAccessGuardIfNeeded& access_guard) {
-      return CompareCharsEqual(str.GetChars(no_gc, access_guard), data, len);
-    }
-    static inline bool HandleExternalOneByteString(
-        ExternalOneByteString str, const Char* data, size_t len,
-        const DisallowGarbageCollection& no_gc,
-        const SharedStringAccessGuardIfNeeded& access_guard) {
-      return CompareCharsEqual(str.GetChars(), data, len);
-    }
-    static inline bool HandleExternalTwoByteString(
-        ExternalTwoByteString str, const Char* data, size_t len,
-        const DisallowGarbageCollection& no_gc,
-        const SharedStringAccessGuardIfNeeded& access_guard) {
-      return CompareCharsEqual(str.GetChars(), data, len);
-    }
-    static inline bool HandleConsString(
-        ConsString str, const Char* data, size_t len,
-        const DisallowGarbageCollection& no_gc,
-        const SharedStringAccessGuardIfNeeded& access_guard) {
-      UNREACHABLE();
-    }
-    static inline bool HandleSlicedString(
-        SlicedString str, const Char* data, size_t len,
-        const DisallowGarbageCollection& no_gc,
-        const SharedStringAccessGuardIfNeeded& access_guard) {
-      UNREACHABLE();
-    }
-    static inline bool HandleThinString(
-        ThinString str, const Char* data, size_t len,
-        const DisallowGarbageCollection& no_gc,
-        const SharedStringAccessGuardIfNeeded& access_guard) {
-      UNREACHABLE();
-    }
-    static inline bool HandleInvalidString(
-        String str, const Char* data, size_t len,
-        const DisallowGarbageCollection& no_gc,
-        const SharedStringAccessGuardIfNeeded& access_guard) {
-      UNREACHABLE();
+  int slice_offset = 0;
+  String string = *this;
+  const Char* data = str.data();
+  while (true) {
+    int32_t type = string.map(isolate).instance_type();
+    switch (type & (kStringRepresentationMask | kStringEncodingMask)) {
+      case kSeqStringTag | kOneByteStringTag:
+        return CompareCharsEqual(
+            SeqOneByteString::cast(string).GetChars(no_gc, access_guard) +
+                slice_offset,
+            data, len);
+      case kSeqStringTag | kTwoByteStringTag:
+        return CompareCharsEqual(
+            SeqTwoByteString::cast(string).GetChars(no_gc, access_guard) +
+                slice_offset,
+            data, len);
+      case kExternalStringTag | kOneByteStringTag:
+        return CompareCharsEqual(
+            ExternalOneByteString::cast(string).GetChars() + slice_offset, data,
+            len);
+      case kExternalStringTag | kTwoByteStringTag:
+        return CompareCharsEqual(
+            ExternalTwoByteString::cast(string).GetChars() + slice_offset, data,
+            len);
+
+      case kSlicedStringTag | kOneByteStringTag:
+      case kSlicedStringTag | kTwoByteStringTag: {
+        SlicedString slicedString = SlicedString::cast(string);
+        slice_offset += slicedString.offset();
+        string = slicedString.parent(isolate);
+        continue;
+      }
+
+      case kConsStringTag | kOneByteStringTag:
+      case kConsStringTag | kTwoByteStringTag: {
+        // The ConsString path is more complex and rare, so call out to an
+        // out-of-line handler.
+        return IsConsStringEqualToImpl<Char>(
+            ConsString::cast(string), slice_offset, str, isolate, access_guard);
+      }
+
+      case kThinStringTag | kOneByteStringTag:
+      case kThinStringTag | kTwoByteStringTag:
+        string = ThinString::cast(string).actual(isolate);
+        continue;
+
+      default:
+        UNREACHABLE();
     }
-  };
+  }
+}
 
-  return StringShape(*this).DispatchToSpecificType<IsEqualToDispatcher, bool>(
-      *this, str.data(), len, no_gc, access_guard);
+// static
+template <typename Char>
+bool String::IsConsStringEqualToImpl(
+    ConsString string, int slice_offset, Vector<const Char> str,
+    IsolateRoot isolate, const SharedStringAccessGuardIfNeeded& access_guard) {
+  // Already checked the len in IsEqualToImpl. Check GE rather than EQ in case
+  // this is a prefix check.
+  DCHECK_GE(string.length(), str.size());
+
+  ConsStringIterator iter(ConsString::cast(string), slice_offset);
+  Vector<const Char> remaining_str = str;
+  for (String segment = iter.Next(&slice_offset); !segment.is_null();
+       segment = iter.Next(&slice_offset)) {
+    // Compare the individual segment against the appropriate subvector of the
+    // remaining string.
+    size_t len = std::min<size_t>(segment.length(), remaining_str.size());
+    Vector<const Char> sub_str = remaining_str.SubVector(0, len);
+    if (!segment.IsEqualToImpl<EqualityType::kNoLengthCheck>(sub_str, isolate,
+                                                             access_guard)) {
+      return false;
+    }
+    remaining_str += len;
+    if (remaining_str.empty()) break;
+  }
+  DCHECK_EQ(remaining_str.data(), str.end());
+  DCHECK_EQ(remaining_str.size(), 0);
+  return true;
 }
 
 bool String::IsOneByteEqualTo(Vector<const char> str) { return IsEqualTo(str); }

src/objects/string.cc

@@ -6,6 +6,7 @@
 
 #include "src/common/assert-scope.h"
 #include "src/common/globals.h"
+#include "src/execution/isolate-utils.h"
 #include "src/execution/thread-id.h"
 #include "src/handles/handles-inl.h"
 #include "src/heap/heap-inl.h"
@@ -1286,7 +1287,10 @@ Object String::LastIndexOf(Isolate* isolate, Handle<Object> receiver,
 }
 
 bool String::HasOneBytePrefix(Vector<const char> str) {
-  return IsEqualTo<EqualityType::kPrefix>(str);
+  DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this));
+  return IsEqualToImpl<EqualityType::kPrefix>(
+      str, GetIsolateForPtrCompr(*this),
+      SharedStringAccessGuardIfNeeded::NotNeeded());
 }
 
 namespace {

src/objects/string.h

@@ -326,8 +326,15 @@ class String : public TorqueGeneratedString<String, Name> {
   // The Isolate is passed as "evidence" that this call is on the main thread,
   // and to distiguish from the LocalIsolate overload.
   template <EqualityType kEqType = EqualityType::kWholeString, typename Char>
-  inline bool IsEqualTo(Vector<const Char> str,
-                        Isolate* isolate = nullptr) const;
+  inline bool IsEqualTo(Vector<const Char> str, Isolate* isolate) const;
+
+  // Check if this string matches the given vector of characters, either as a
+  // whole string or just a prefix.
+  //
+  // This is main-thread only, like the Isolate* overload, but additionally
+  // computes the IsolateRoot for IsEqualToImpl.
+  template <EqualityType kEqType = EqualityType::kWholeString, typename Char>
+  inline bool IsEqualTo(Vector<const Char> str) const;
 
   // Check if this string matches the given vector of characters, either as a
   // whole string or just a prefix.
@@ -539,9 +546,15 @@ class String : public TorqueGeneratedString<String, Name> {
   // Implementation of the IsEqualTo() public methods. Do not use directly.
   template <EqualityType kEqType, typename Char>
   V8_INLINE bool IsEqualToImpl(
-      Vector<const Char> str,
+      Vector<const Char> str, IsolateRoot isolate,
       const SharedStringAccessGuardIfNeeded& access_guard) const;
 
+  // Out-of-line IsEqualToImpl for ConsString.
+  template <typename Char>
+  V8_NOINLINE static bool IsConsStringEqualToImpl(
+      ConsString string, int slice_offset, Vector<const Char> str,
+      IsolateRoot isolate, const SharedStringAccessGuardIfNeeded& access_guard);
+
   V8_EXPORT_PRIVATE static Handle<String> SlowFlatten(
       Isolate* isolate, Handle<ConsString> cons, AllocationType allocation);
 

test/mjsunit/regress/regress-1193903.js

+// Copyright 2014 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
+
+var no_sync_uninternalized = "no " + "sync";
+%InternalizeString(no_sync_uninternalized);
+
+// Make sure %GetOptimizationStatus works with a non-internalized string
+// parameter.
+%GetOptimizationStatus(function() {}, no_sync_uninternalized)