[base] Lazily ensure ThreadList invariant

Since next() comes from ThreadedListTraits, the users of this class
can modify the list by modifying the next pointer. This however breaks
the invariant that `tail_` points to the last element of the list.

We ensure this invariant lazily. This should be _almost_ no effect
for users that do not manually modify the next pointer.

Change-Id: If46283ab4fc5036a81f353b25823b0fd39b3e232
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3578803Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Victor Gomes <victorgomes@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80042}

src/base/threaded-list.h

@@ -23,9 +23,15 @@ struct ThreadedListTraits {
 // Represents a linked list that threads through the nodes in the linked list.
 // Entries in the list are pointers to nodes. By default nodes need to have a
 // T** next() method that returns the location where the next value is stored.
+// The kSupportsUnsafeInsertion flag defines whether the list supports insertion
+// of new elements into the list by just rewiring the next pointers without
+// updating the list object itself. Such an insertion might invalidate the
+// pointer to list tail and thus requires additional steps to recover the
+// pointer to the tail.
 // The default can be overwritten by providing a ThreadedTraits class.
 template <typename T, typename BaseClass,
-          typename TLTraits = ThreadedListTraits<T>>
+          typename TLTraits = ThreadedListTraits<T>,
+          bool kSupportsUnsafeInsertion = false>
 class ThreadedListBase final : public BaseClass {
  public:
   ThreadedListBase() : head_(nullptr), tail_(&head_) {}
@@ -33,6 +39,7 @@ class ThreadedListBase final : public BaseClass {
   ThreadedListBase& operator=(const ThreadedListBase&) = delete;
 
   void Add(T* v) {
+    EnsureValidTail();
     DCHECK_NULL(*tail_);
     DCHECK_NULL(*TLTraits::next(v));
     *tail_ = v;
@@ -51,6 +58,15 @@ class ThreadedListBase final : public BaseClass {
     head_ = v;
   }
 
+  // This temporarily breaks the tail_ invariant, and it should only be called
+  // if we support unsafe insertions.
+  static void AddAfter(T* after_this, T* v) {
+    DCHECK(kSupportsUnsafeInsertion);
+    DCHECK_NULL(*TLTraits::next(v));
+    *TLTraits::next(v) = *TLTraits::next(after_this);
+    *TLTraits::next(after_this) = v;
+  }
+
   void DropHead() {
     DCHECK_NOT_NULL(head_);
 
@@ -70,6 +86,7 @@ class ThreadedListBase final : public BaseClass {
   void Append(ThreadedListBase&& list) {
     if (list.is_empty()) return;
 
+    EnsureValidTail();
     *tail_ = list.head_;
     tail_ = list.tail_;
     list.Clear();
@@ -78,6 +95,7 @@ class ThreadedListBase final : public BaseClass {
   void Prepend(ThreadedListBase&& list) {
     if (list.head_ == nullptr) return;
 
+    EnsureValidTail();
     T* new_head = list.head_;
     *list.tail_ = head_;
     if (head_ == nullptr) {
@@ -116,6 +134,7 @@ class ThreadedListBase final : public BaseClass {
       return true;
     }
 
+    EnsureValidTail();
     while (current != nullptr) {
       T* next = *TLTraits::next(current);
       if (next == v) {
@@ -213,10 +232,16 @@ class ThreadedListBase final : public BaseClass {
   };
 
   Iterator begin() { return Iterator(TLTraits::start(&head_)); }
-  Iterator end() { return Iterator(tail_); }
+  Iterator end() {
+    EnsureValidTail();
+    return Iterator(tail_);
+  }
 
   ConstIterator begin() const { return ConstIterator(TLTraits::start(&head_)); }
-  ConstIterator end() const { return ConstIterator(tail_); }
+  ConstIterator end() const {
+    EnsureValidTail();
+    return ConstIterator(tail_);
+  }
 
   // Rewinds the list's tail to the reset point, i.e., cutting of the rest of
   // the list, including the reset_point.
@@ -253,7 +278,7 @@ class ThreadedListBase final : public BaseClass {
     return *t;
   }
 
-  bool Verify() {
+  bool Verify() const {
     T* last = this->first();
     if (last == nullptr) {
       CHECK_EQ(&head_, tail_);
@@ -266,7 +291,19 @@ class ThreadedListBase final : public BaseClass {
     return true;
   }
 
-  void RevalidateTail() {
+  inline void EnsureValidTail() const {
+    if (!kSupportsUnsafeInsertion) {
+      DCHECK_EQ(*tail_, nullptr);
+      return;
+    }
+    // If kSupportsUnsafeInsertion, then we support adding a new element by
+    // using the pointer to a certain element. E.g., imagine list A -> B -> C,
+    // we can add D after B, by just moving the pointer of B to D and D to
+    // whatever B used to point to. We do not need to know the beginning of the
+    // list (ie. to have a pointer to the ThreadList class). This however might
+    // break the tail_ invariant. We ensure this here, by manually looking for
+    // the tail of the list.
+    if (*tail_ == nullptr) return;
     T* last = *tail_;
     if (last != nullptr) {
       while (*TLTraits::next(last) != nullptr) {
@@ -274,19 +311,26 @@ class ThreadedListBase final : public BaseClass {
       }
       tail_ = TLTraits::next(last);
     }
-    SLOW_DCHECK(Verify());
   }
 
  private:
   T* head_;
-  T** tail_;
+  mutable T** tail_;  // We need to ensure a valid `tail_` even when using a
+                      // const Iterator.
 };
 
 struct EmptyBase {};
 
+// Check ThreadedListBase::EnsureValidTail.
+static constexpr bool kUnsafeInsertion = true;
+
 template <typename T, typename TLTraits = ThreadedListTraits<T>>
 using ThreadedList = ThreadedListBase<T, EmptyBase, TLTraits>;
 
+template <typename T, typename TLTraits = ThreadedListTraits<T>>
+using ThreadedListWithUnsafeInsertions =
+    ThreadedListBase<T, EmptyBase, TLTraits, kUnsafeInsertion>;
+
 }  // namespace base
 }  // namespace v8
 

src/maglev/maglev-graph-builder.h

@@ -35,14 +35,6 @@ class MaglevGraphBuilder {
       // TODO(v8:7700): Clean up after all bytecodes are supported.
       if (found_unsupported_bytecode()) break;
     }
-
-    // During InterpreterFrameState merge points, we might emit CheckedSmiTags
-    // and add them unsafely to the basic blocks. This addition might break a
-    // list invariant (namely `tail_` might not point to the last element).
-    // We revalidate this invariant here in all basic blocks.
-    for (BasicBlock* block : *graph_) {
-      block->nodes().RevalidateTail();
-    }
   }
 
   Graph* graph() const { return graph_; }

src/maglev/maglev-interpreter-frame-state.h

@@ -388,7 +388,7 @@ class MergePointInterpreterFrameState {
         Node::New<CheckedSmiTag, std::initializer_list<ValueNode*>>(
             compilation_unit.zone(), compilation_unit,
             value->eager_deopt_info()->state, {value});
-    value->AddNodeAfter(tagged);
+    Node::List::AddAfter(value, tagged);
     compilation_unit.RegisterNodeInGraphLabeller(tagged);
     return tagged;
   }

src/maglev/maglev-ir.h

@@ -659,19 +659,10 @@ constexpr bool NodeBase::Is<UnconditionalControlNode>() const {
 // The Node class hierarchy contains all non-control nodes.
 class Node : public NodeBase {
  public:
-  using List = base::ThreadedList<Node>;
+  using List = base::ThreadedListWithUnsafeInsertions<Node>;
 
   inline ValueLocation& result();
 
-  // This might break ThreadedList invariants.
-  // Run ThreadedList::RevalidateTail afterwards.
-  void AddNodeAfter(Node* node) {
-    DCHECK_NOT_NULL(node);
-    DCHECK_NULL(node->next_);
-    node->next_ = next_;
-    next_ = node;
-  }
-
   Node* NextNode() const { return next_; }
 
  protected: