NetBSD Problem Report #59260

From www@netbsd.org  Sun Apr  6 23:52:46 2025
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)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits)
	 client-signature RSA-PSS (2048 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id C1EB31A9239
	for <gnats-bugs@gnats.NetBSD.org>; Sun,  6 Apr 2025 23:52:46 +0000 (UTC)
Message-Id: <20250406235245.7B7BC1A923C@mollari.NetBSD.org>
Date: Sun,  6 Apr 2025 23:52:45 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: t___sync_lock:__sync_lock_release_N tests are failing
X-Send-Pr-Version: www-1.0

>Number:         59260
>Category:       port-vax
>Synopsis:       t___sync_lock:__sync_lock_release_N tests are failing
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    port-vax-maintainer
>State:          needs-pullups
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Apr 06 23:55:00 +0000 2025
>Closed-Date:    
>Last-Modified:  Fri Apr 25 12:20:54 +0000 2025
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The __sync_vax_bsd_4 Lockification
>Environment:
>Description:
FAILED: /tmp/build/2025.04.04.21.52.19-vax/src/tests/lib/libc/atomic/t___sync_lock.c:98: val expects 0x0 but 0x88

https://releng.netbsd.org/b5reports/vax/2025/2025.04.04.21.52.19/test.html#lib_libc_atomic_t___sync_lock___sync_lock_release_1
>How-To-Repeat:
cd /usr/tests/lib/libc/atomic
atf-run t___sync_lock | atf-report
>Fix:
Yes, please!

gcc documentation is a little vague:

https://gcc.gnu.org/onlinedocs/gcc-12.4.0/gcc/_005f_005fsync-Builtins.html

But I think gcc's intent is that the __sync_lock_test_and_set_*/__sync_lock_release_* interface treats 0 as unlocked and 1 as locked, and any other values are architecture-dependent (which is unfortunate, because on, e.g., hppa, it's better if unlocked is _nonzero_ and locked is 0 -- this allows use of the LDCW instruction, load and clear word).

We could apply the patch to continue testing exciting values on non-vax architectures, and even test the quirks on vax (it only sets/clears a single bit in the word preserving the rest):

diff -r 2f4e70ccac63 tests/lib/libc/atomic/t___sync_lock.c
--- a/tests/lib/libc/atomic/t___sync_lock.c	Sun Apr 06 21:12:14 2025 +0000
+++ b/tests/lib/libc/atomic/t___sync_lock.c	Sun Apr 06 23:51:34 2025 +0000
@@ -44,8 +44,60 @@
  * It's better to run only when target is actually in libc.
  */

-#define OLDVAL (0x1122334455667788UL)
-#define NEWVAL (0x8090a0b0c0d0e0f0UL)
+#if defined __vax__
+/*
+ * On VAX, __sync_lock_test_and_set_* test and set the low-order bit
+ * with BBSSI, and __sync_lock_release_* clear the low-order bit with
+ * BBCCI, so the other bits are not relevant.
+ *
+ * It is possible that, by using values other than 0 and 1, we are
+ * relying on more than gcc guarantees about __sync_lock_test_and_set_*
+ * and __sync_lock_release_*.  But, well, if so, we will be alerted by
+ * a failing test.
+ */
+#define INITVAL 0x1122334455667788
+#define LOCKVAL 1
+#define LOCKRET 0
+#define LOCKEDVAL 0x1122334455667789
+#define UNLOCKEDVAL 0x1122334455667788
+#elif 0 && defined __hppa__
+/*
+ * On HPPA, the native atomic r/m/w instruction, LDCW, atomically loads
+ * a word and clears it, so the obvious choice is for the unlocked
+ * state to be nonzero and the locked state to be zero.
+ *
+ * But gcc doesn't do that.
+ *
+ * Instead, it uses zero for unlocked and nonzero for locked.  So for
+ * __sync_lock_test_and_set_* it issues an out-of-line call (which on
+ * NetBSD implements by atomic_swap_N), and for __sync_lock_release_*,
+ * it issues LDCW on a scratch stack location only as a barrier and
+ * then issues STW to store a zero.
+ *
+ * So we don't use this branch after all.  But I'm leaving it here as a
+ * reminder to anyone who suspects something might be wrong on HPPA.
+ */
+#define INITVAL 0x1122334455667788
+#define LOCKVAL 0
+#define LOCKRET 0x1122334455667788
+#define LOCKEDVAL 0
+#define UNLOCKEDVAL 1
+#else
+/*
+ * According to GCC documentation at
+ * <https://gcc.gnu.org/onlinedocs/gcc-12.4.0/gcc/_005f_005fsync-Builtins.html>,
+ * the only guaranteed supported value for LOCKVAL is 1, and it is not
+ * guaranteed that __sync_lock_release_* stores zero.  But on many
+ * architectures other values work too, and __sync_lock_release_* does
+ * just store zero, so let's test these by default; the exceptions can
+ * be listed above.
+ */
+#define INITVAL 0x1122334455667788
+#define LOCKVAL 0x8090a0b0c0d0e0f0
+#define LOCKRET 0x1122334455667788
+#define LOCKEDVAL 0x8090a0b0c0d0e0f0
+#define UNLOCKEDVAL 0
+#endif

 #define atf_sync_tas(NAME, TYPE, FMT) \
 ATF_TC(NAME); \
@@ -60,10 +112,10 @@ ATF_TC_BODY(NAME, tc) \
 	TYPE expval; \
 	TYPE expres; \
 	TYPE res; \
-	val = (TYPE)OLDVAL; \
-	newval = (TYPE)NEWVAL; \
-	expval = (TYPE)NEWVAL; \
-	expres = (TYPE)OLDVAL; \
+	val = (TYPE)INITVAL; \
+	newval = (TYPE)LOCKVAL; \
+	expval = (TYPE)LOCKEDVAL; \
+	expres = (TYPE)LOCKRET; \
 	res = NAME(&val, newval); \
 	ATF_REQUIRE_MSG(val == expval, \
 	    "val expects 0x%" FMT " but 0x%" FMT, expval, val); \
@@ -88,8 +140,8 @@ ATF_TC_BODY(NAME, tc) \
 { \
 	volatile TYPE val; \
 	TYPE expval; \
-	val = (TYPE)OLDVAL; \
-	expval = (TYPE)0; \
+	val = (TYPE)LOCKEDVAL; \
+	expval = (TYPE)UNLOCKEDVAL; \
 	NAME(&val); \
 	ATF_REQUIRE_MSG(val == expval, \
 	    "val expects 0x%" FMT " but 0x%" FMT, expval, val); \

But maybe we should really just use 0 and 1 and ditch all the other exciting values.

>Release-Note:

>Audit-Trail:
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59260 CVS commit: src/tests/lib/libc/atomic
Date: Mon, 7 Apr 2025 01:34:43 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Mon Apr  7 01:34:43 UTC 2025

 Modified Files:
 	src/tests/lib/libc/atomic: t___sync_lock.c

 Log Message:
 t___sync_lock: Try to make this work on VAX.

 Add note about why it _isn't_ also special on HPPA even though it may
 seems like it ought to be.

 PR port-vax/59260: t___sync_lock:__sync_lock_release_N tests are failing


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.2 src/tests/lib/libc/atomic/t___sync_lock.c

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

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Fri, 25 Apr 2025 12:20:54 +0000
State-Changed-Why:
As of 2025-04-08, these tests are now passing on the releng vax
testbed:

https://releng.netbsd.org/b5reports/vax/commits-2025.04.html#build-2025.04.08.04.19.28


>Unformatted:

Home	PR Database Search