BUG/MEDIUM: tasks: don't type-pun state and tid in builds without threads

__task_set_state_and_tid() updates t->state and t->tid at once through
an uint64_t cast, which violates strict aliasing. With threads the
__atomic builtins keep the compiler conservative, but without them the
accesses are plain, and gcc -O2 legitimately keeps using a stale
t->state: the CAS always fails and _task_wakeup() spins forever on the
first request until the watchdog kills the process.

Without threads there is no concurrency to protect against, so just
compare and assign the two fields directly.

This is what commit c5f41118c01db4babcf6919f5eae4ce308adcfef should have
been.

diff --git a/include/haproxy/task.h b/include/haproxy/task.h
index b0da721afe703549727588e6e349d32a36cdde14..2a70cdcfd36f12fe4af3ff0c099b8deb9c644035 100644 (file)
--- a/include/haproxy/task.h
+++ b/include/haproxy/task.h
@@ -210,7 +210,20 @@ __decl_thread(extern HA_SPINLOCK_T task_state_tid);
 
 static inline int __task_set_state_and_tid(struct task *t, int expected_tid, int new_tid, unsigned int current, unsigned int wanted)
 {
-#if defined(HA_CAS_IS_8B) || defined(HA_HAVE_CAS_DW)
+#if !defined(USE_THREAD)
+       /* No concurrency here, so plain accesses to the two fields are
+        * sufficient. It's also required, as the 64-bit accesses performed
+        * below break strict-aliasing rules, which only the atomic builtins
+        * protect against being optimized: with them turned into plain
+        * accesses, the compiler may reorder or cache the fields' values
+        * across the cast and make the callers' retry loops spin forever.
+        */
+       if (t->tid != expected_tid || t->state != current)
+               return 0;
+       t->state = wanted;
+       t->tid = new_tid;
+       return 1;
+#elif defined(HA_CAS_IS_8B) || defined(HA_HAVE_CAS_DW)
        uint64_t expected_value;
        uint64_t new_value;