From a7fabe2328d840bc6f1f7a11c2435057b63c94d9 Mon Sep 17 00:00:00 2001
From: Allan Sandfeld Jensen <allan.jensen@qt.io>
Date: Fri, 9 Apr 2021 11:53:58 +0200
Subject: [PATCH] Light cleanup in QSemaphore Futex implementation

Gets rid of a goto, fixes overflow detection with wakeAll set,
and fixes 64-bit futex mode with futexHasWaiterCount = false.

Change-Id: I8bb98118013fc1dc2a8a405845bec0cb3350494f
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
---
 src/corelib/thread/qsemaphore.cpp | 62 ++++++++++++++-----------------
 1 file changed, 27 insertions(+), 35 deletions(-)

diff --git a/src/corelib/thread/qsemaphore.cpp b/src/corelib/thread/qsemaphore.cpp
index 213852250fc..d89367c1cbe 100644
--- a/src/corelib/thread/qsemaphore.cpp
+++ b/src/corelib/thread/qsemaphore.cpp
@@ -132,8 +132,8 @@ static constexpr bool futexHasWaiterCount = true;
 static constexpr bool futexHasWaiterCount = false;
 #endif
 
-static const quintptr futexNeedsWakeAllBit =
-        Q_UINT64_C(1) << (sizeof(quintptr) * CHAR_BIT - 1);
+static constexpr quintptr futexNeedsWakeAllBit = futexHasWaiterCount ?
+        (Q_UINT64_C(1) << (sizeof(quintptr) * CHAR_BIT - 1)) : 0x80000000U;
 
 static int futexAvailCounter(quintptr v)
 {
@@ -169,6 +169,7 @@ static QBasicAtomicInteger<quint32> *futexLow32(QBasicAtomicInteger<quintptr> *p
 
 static QBasicAtomicInteger<quint32> *futexHigh32(QBasicAtomicInteger<quintptr> *ptr)
 {
+    Q_ASSERT(futexHasWaiterCount);
     auto result = reinterpret_cast<QBasicAtomicInteger<quint32> *>(ptr);
 #if Q_BYTE_ORDER == Q_LITTLE_ENDIAN && QT_POINTER_SIZE > 4
     ++result;
@@ -184,38 +185,16 @@ futexSemaphoreTryAcquire_loop(QBasicAtomicInteger<quintptr> &u, quintptr curValu
     int n = int(unsigned(nn));
 
     // we're called after one testAndSet, so start by waiting first
-    goto start_wait;
-
-    forever {
-        if (futexAvailCounter(curValue) >= n) {
-            // try to acquire
-            quintptr newValue = curValue - nn;
-            if (u.testAndSetOrdered(curValue, newValue, curValue))
-                return true;        // succeeded!
-            continue;
-        }
-
-        // not enough tokens available, put us to wait
-        if (remainingTime == 0)
-            return false;
-
+    for (;;) {
         // indicate we're waiting
-start_wait:
-        auto ptr = [&u]() {
-            if constexpr (futexHasWaiterCount)
-                return futexLow32(&u);
-            else
-                return &u;
-        }();
+        auto ptr = futexLow32(&u);
         if (n > 1 || !futexHasWaiterCount) {
             u.fetchAndOrRelaxed(futexNeedsWakeAllBit);
             curValue |= futexNeedsWakeAllBit;
             if constexpr (futexHasWaiterCount) {
-                if (n > 1) {
-                    ptr = futexHigh32(&u);
-                    // curValue >>= 32;  // but this is UB in 32-bit, so roundabout:
-                    curValue = quint64(curValue) >> 32;
-                }
+                Q_ASSERT(n > 1);
+                ptr = futexHigh32(&u);
+                curValue >>= 32;
             }
         }
 
@@ -230,6 +209,17 @@ start_wait:
         curValue = u.loadAcquire();
         if (IsTimed)
             remainingTime = timer.remainingTimeNSecs();
+
+        // try to acquire
+        while (futexAvailCounter(curValue) >= n) {
+            quintptr newValue = curValue - nn;
+            if (u.testAndSetOrdered(curValue, newValue, curValue))
+                return true;        // succeeded!
+        }
+
+        // not enough tokens available, put us to wait
+        if (remainingTime == 0)
+            return false;
     }
 }
 
@@ -252,12 +242,14 @@ template <bool IsTimed> bool futexSemaphoreTryAcquire(QBasicAtomicInteger<quintp
         return false;
 
     // we need to wait
-    quintptr oneWaiter = quintptr(Q_UINT64_C(1) << 32); // zero on 32-bit
-    if (futexHasWaiterCount) {
+    constexpr quintptr oneWaiter = quintptr(Q_UINT64_C(1) << 32); // zero on 32-bit
+    if constexpr (futexHasWaiterCount) {
         // We don't use the fetched value from above so futexWait() fails if
         // it changed after the testAndSetOrdered above.
-        if ((quint64(curValue) >> 32) == 0x7fffffff)
-            return false;       // overflow!
+        if (((curValue >> 32) & 0x7fffffffU) == 0x7fffffffU) {
+            qCritical() << "Waiter count overflow in QSemaphore";
+            return false;
+        }
 
         // increase the waiter count
         u.fetchAndAddRelaxed(oneWaiter);
@@ -270,6 +262,8 @@ template <bool IsTimed> bool futexSemaphoreTryAcquire(QBasicAtomicInteger<quintp
     if (futexSemaphoreTryAcquire_loop<IsTimed>(u, curValue, nn, timeout))
         return true;
 
+    Q_ASSERT(IsTimed);
+
     if (futexHasWaiterCount) {
         // decrement the number of threads waiting
         Q_ASSERT(futexHigh32(&u)->loadRelaxed() & 0x7fffffffU);
@@ -501,8 +495,6 @@ bool QSemaphore::tryAcquire(int n, int timeout)
         return false;
     d->avail -= n;
     return true;
-
-
 }
 
 /*!