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

[campbell+netbsd%mumble.net@localhost]

>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:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Apr 06 23:55:00 +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.