NetBSD Problem Report #56832

From www@netbsd.org  Sat May 14 00:58:29 2022
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 5241F1A9239
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 14 May 2022 00:58:29 +0000 (UTC)
Message-Id: <20220514005827.C03A11A923C@mollari.NetBSD.org>
Date: Sat, 14 May 2022 00:58:27 +0000 (UTC)
From: tgl@sss.pgh.pa.us
Reply-To: tgl@sss.pgh.pa.us
To: gnats-bugs@NetBSD.org
Subject: __atomic_compare_exchange[_n] is wrong on non-mainstream platforms
X-Send-Pr-Version: www-1.0

>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:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat May 14 01:00:01 +0000 2022
>Closed-Date:    Sun May 15 17:12:23 +0000 2022
>Last-Modified:  Mon May 16 06:20: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:/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.  

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->pending-pullups
State-Changed-By: skrll@NetBSD.org
State-Changed-When: Sat, 14 May 2022 05:40:54 +0000
State-Changed-Why:
Fix committed. Pending pullups now.


From: Nick Hudson <nick.hudson@gmx.co.uk>
To: gnats-bugs@netbsd.org, lib-bug-people@netbsd.org, netbsd-bugs@netbsd.org,
 gnats-admin@netbsd.org, tgl@sss.pgh.pa.us
Cc: 
Subject: Re: lib/56832 (__atomic_compare_exchange[_n] is wrong on
 non-mainstream platforms)
Date: Sun, 15 May 2022 09:08:11 +0100

 [pullup-9 #1451] lib/56832 fix

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56832 CVS commit: [netbsd-9] src/common/lib/libc/atomic
Date: Sun, 15 May 2022 12:37:00 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Sun May 15 12:37:00 UTC 2022

 Modified Files:
 	src/common/lib/libc/atomic [netbsd-9]:
 	    atomic_c11_compare_exchange_cas_16.c
 	    atomic_c11_compare_exchange_cas_32.c
 	    atomic_c11_compare_exchange_cas_8.c

 Log Message:
 Pull up following revision(s) (requested by skrll in ticket #1451):

 	common/lib/libc/atomic/atomic_c11_compare_exchange_cas_8.c: revision 1.4
 	common/lib/libc/atomic/atomic_c11_compare_exchange_cas_32.c: revision 1.4
 	common/lib/libc/atomic/atomic_c11_compare_exchange_cas_16.c: revision 1.4

 PR 56832:
 fix C implementations of __atomic_compare_exchange*


 To generate a diff of this commit:
 cvs rdiff -u -r1.2 -r1.2.20.1 \
     src/common/lib/libc/atomic/atomic_c11_compare_exchange_cas_16.c \
     src/common/lib/libc/atomic/atomic_c11_compare_exchange_cas_32.c \
     src/common/lib/libc/atomic/atomic_c11_compare_exchange_cas_8.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: skrll@NetBSD.org
State-Changed-When: Sun, 15 May 2022 17:12:23 +0000
State-Changed-Why:
pullup processed.
Thanks for the PR.


From: Tom Lane <tgl@sss.pgh.pa.us>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/56832: __atomic_compare_exchange[_n] is wrong on non-mainstream platforms
Date: Sun, 15 May 2022 14:02:00 -0400

 Hmm, the version of this that was pulled up into netbsd-9 has one
 serious error: atomic_c11_compare_exchange_cas_32.c uses atomic_cas_8
 where it should use atomic_cas_32.  I did not look at HEAD but
 I suppose it's got the same problem.

 			regards, tom lane

From: Nick Hudson <nick.hudson@gmx.co.uk>
To: gnats-bugs@netbsd.org, lib-bug-people@netbsd.org, gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org, tgl@sss.pgh.pa.us
Cc: 
Subject: Re: lib/56832: __atomic_compare_exchange[_n] is wrong on
 non-mainstream platforms
Date: Mon, 16 May 2022 07:15:33 +0100

 >   Hmm, the version of this that was pulled up into netbsd-9 has one
 >   serious error: atomic_c11_compare_exchange_cas_32.c uses atomic_cas_8
 >   where it should use atomic_cas_32.  I did not look at HEAD but
 >   I suppose it's got the same problem.

 Good spot.

 [pullup-9 #1453] more lib/56832: Fwd: CVS commit: src/common/lib/libc/atomic

 Thanks,
 Nick

>Unformatted:

Home	PR Database Search