[PATCH] D69265: [scudo][standalone] Consolidate lists

[Mitch Phillips via Phabricator via llvm-commits]

hctim added a comment.

Can we consider adding a fuzz target for the linked lists? I recently did a similar thing in android for a ringbuffer, which you can see here <https://android-review.googlesource.com/c/platform/system/nfc/+/1147940>. If you get it checked in, I can help with getting it running on OSS-Fuzz.



================
Comment at: lib/scudo/standalone/list.h:17
+// Intrusive POD singly and doubly linked list.
 // An object with all zero fields should represent a valid empty list. clear()
 // should be called on all non-zero-initialized objects before using.
----------------
Why does the requirements to call `clear()` exist for non-zero-inited objects? For globally zero-init'ed instances of this object, we don't encounter a perf overhead of having `Size`, `First`, and `Last` be initialised to zero. For non-global instances, we have to call `clear()`, which seems easy to forget, and moving this initialisation into the c'tor doesn't cause any additional overhead (and I'd assume the call is elided, making it faster).

It also feels easy to forget to call `clear()`, and if any global objects are moved into static scopage, it may be incredibly hard to patch any instances where `clear()` isn't called.


================
Comment at: lib/scudo/standalone/list.h:57
+
+  void checkConsistency() const {
+    if (Size == 0) {
----------------
Looks like this is test-only, can we move it out of the core header?


================
Comment at: lib/scudo/standalone/list.h:85
+  void push_back(T *X) {
+    if (this->empty()) {
       X->Next = nullptr;
----------------
Shouldn't need `this->` if you add `using IntrusiveList<T>::empty;`

(and similar elsewhere)


================
Comment at: lib/scudo/standalone/list.h:121
     DCHECK_NE(X, nullptr);
-    DCHECK_EQ(Prev->Next, X);
+    CHECK_EQ(Prev->Next, X);
     Prev->Next = X->Next;
----------------
Why `/s/DCHECK/CHECK` here? This shouldn't fail in normal program execution, right?


================
Comment at: lib/scudo/standalone/list.h:151
+    if (this->empty()) {
+      X->Next = nullptr;
+      First = Last = X;
----------------
Isn't there an invariant held that if `empty() == true`, then `First == nullptr`? In which case, can't we hoist the `X->Next = First;` out from this conditional?


================
Comment at: lib/scudo/standalone/list.h:154
     } else {
-      uptr Count = 0;
-      for (Item *I = First;; I = I->Next) {
-        Count++;
-        if (I == Last)
-          break;
-      }
-      CHECK_EQ(size(), Count);
-      CHECK_EQ(Last->Next, nullptr);
+      CHECK_EQ(First->Prev, nullptr);
+      X->Next = First;
----------------
DCHECK?


================
Comment at: lib/scudo/standalone/list.h:163
+  void insert(T *X, T *Y) {
+    if (Y == First) {
+      CHECK_EQ(Y->Prev, nullptr);
----------------
Would the specialisation:

```
if (Y == First)
  return push_front(X);
```
be more performant?

You should then be able to remove the `if (Prev)` check below, as assumedly there's an invariant that if `Y` isn't the first element, it has a `Prev`.


================
Comment at: lib/scudo/standalone/list.h:164
+    if (Y == First) {
+      CHECK_EQ(Y->Prev, nullptr);
+      First = X;
----------------
DCHECK?


================
Comment at: lib/scudo/standalone/list.h:181
+    if (this->empty()) {
+      X->Prev = nullptr;
+      First = Last = X;
----------------
Same as `push_front`, isn't there an invariant that if `empty() == true`, `Last == nullptr`, and the `X->Prev = Last` can be hoisted out?


================
Comment at: lib/scudo/standalone/list.h:184
+    } else {
+      CHECK_EQ(Last->Next, nullptr);
+      X->Prev = Last;
----------------
DCHECK?


================
Comment at: lib/scudo/standalone/list.h:193
+  void pop_front() {
+    CHECK(!this->empty());
+    First = First->Next;
----------------
DCHECK?


================
Comment at: lib/scudo/standalone/list.h:206
+    if (Prev) {
+      CHECK_EQ(Prev->Next, X);
+      Prev->Next = Next;
----------------
DCHECK? (and throughout this function)


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69265/new/

https://reviews.llvm.org/D69265