lib/56832: __atomic_compare_exchange[_n] is wrong on non-mainstream platforms

[tgl%sss.pgh.pa.us@localhost]

>Number:         56832
>Category:       lib
>Synopsis:       __atomic_compare_exchange[_n] is wrong on non-mainstream platforms
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat May 14 01:00:01 +0000 2022
>Originator:     Tom Lane
>Release:        HEAD/202205021430Z (problem is old, though)
>Organization:
PostgreSQL Global Development Group
>Environment:
NetBSD sss2.sss.pgh.pa.us 9.99.96 NetBSD 9.99.96 (GENERIC) #0: Wed May 11 15:39:57 EDT 2022  tgl%nuc1.sss.pgh.pa.us@localhost:/home/tgl/netbsd-H-202205021430Z/obj.hppa/sys/arch/hppa/compile/GENERIC hppa
>Description:
The C11 spec defines __atomic_compare_exchange[_n] as:

bool __atomic_compare_exchange_n (type *ptr, type *expected, type desired, bool weak, int success_memorder, int failure_memorder)

This built-in function implements an atomic compare and exchange operation. This compares the contents of *ptr with the contents of *expected. If equal, the operation is a read-modify-write operation that writes desired into *ptr. If they are not equal, the operation is a read and
the current contents of *ptr are written into *expected. [etc]
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

(Actually, I'm quoting the GCC docs because I don't have C11 at hand.  But it's hard to see why the "expected" argument would be defined as a pointer if there were no intention of updating its value.)

The implementations of these functions in common/lib/libc/atomic/atomic_c11_compare_exchange_cas_*.c fail to perform the step of storing the contents of *ptr back into *expected.

This appears to have been wrong since these functions were added in 2014.  I surmise that it hasn't been noticed because (a) these implementations are only used on non-mainstream platforms, and/or (b) most programs don't rely on the detail of *expected getting updated.
>How-To-Repeat:
Inspection of the source code is sufficient to show that these functions never update *expected.

Alternatively, running PostgreSQL's "make check" tests on an affected platform (I was using HPPA) will usually show the problem, in the form of some process getting stuck in a wait loop.

>Fix:
It could be coded in a couple of ways, but I did this:

Index: common/lib/libc/atomic/atomic_c11_compare_exchange_cas_32.c
===================================================================
RCS file: /cvsroot/src/common/lib/libc/atomic/atomic_c11_compare_exchange_cas_32.c,v
retrieving revision 1.3
diff -u -r1.3 atomic_c11_compare_exchange_cas_32.c
--- common/lib/libc/atomic/atomic_c11_compare_exchange_cas_32.c 7 Sep 2020 00:52:19 -0000       1.3
+++ common/lib/libc/atomic/atomic_c11_compare_exchange_cas_32.c 14 May 2022 00:19:46 -0000
@@ -45,11 +45,16 @@
     bool weak, int success, int failure)
 {
        uint32_t old = *(uint32_t *)expected;
+       uint32_t prev;
 
        /*
         * Ignore the details (weak, memory model on success and failure)
         * and just do the cas. If we get here the compiler couldn't
         * do better and it mostly will not matter at all.
         */
-       return atomic_cas_32(mem, old, desired) == old;
+       prev = atomic_cas_32(mem, old, desired);
+       if (prev == old)
+               return true;
+       *(uint32_t *)expected = prev;
+       return false;
 }

and similarly in the other two files.