On Mon, May 04, 2026 at 06:16:47PM -0400, Tom Lane wrote: > Nathan Bossart <[email protected]> writes: >> I noticed that s_lock.h points to a default implementation of tas() in >> tas.s or s_lock.c, but AFAICT there hasn't been a tas() implementation in >> s_lock.c since commit 718aa43a4e, and commit 25f36066dd seems to have >> removed the last remaining tas.s files. So, I think this is dead code. > > It is, but I think the 0001 patch should be more like > > #if !defined(TAS) > -extern int tas(volatile slock_t *lock); /* in port/.../tas.s, or > - * s_lock.c */ > - > -#define TAS(lock) tas(lock) > +#error "must provide a spinlock implementation" > #endif /* TAS */ > > Perhaps this could be merged with the earlier bit about erroring > if not HAS_TEST_AND_SET. > >> I also noticed that HAS_TEST_AND_SET just means that TAS is defined, so I >> wrote a 0002 that removes it in favor of checking TAS directly. > > I'm pretty much -1 on that; HAS_TEST_AND_SET is clearer than TAS, and > removing it seems quite likely to break someone's code. We could > perhaps collect all the separate instances into this end location: > > #if defined(TAS) > #define HAS_TEST_AND_SET > #else > #error "must provide a spinlock implementation" > #endif /* TAS */
Okay, here's a new version of the patch that I believe addresses both points. -- nathan
>From 0fd70f722bd9e16f82b93bf125e04dac9d599a7e Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Tue, 5 May 2026 10:59:19 -0500 Subject: [PATCH v2 1/1] fix up TAS in s_lock.h --- src/include/storage/s_lock.h | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h index c9e52511990..5ff863d1da5 100644 --- a/src/include/storage/s_lock.h +++ b/src/include/storage/s_lock.h @@ -124,7 +124,6 @@ #ifdef __i386__ /* 32-bit i386 */ -#define HAS_TEST_AND_SET typedef unsigned char slock_t; @@ -194,7 +193,6 @@ spin_delay(void) #ifdef __x86_64__ /* AMD Opteron, Intel EM64T */ -#define HAS_TEST_AND_SET typedef unsigned char slock_t; @@ -249,7 +247,6 @@ spin_delay(void) */ #if defined(__arm__) || defined(__arm) || defined(__aarch64__) #ifdef HAVE_GCC__SYNC_INT32_TAS -#define HAS_TEST_AND_SET #define TAS(lock) tas(lock) @@ -292,7 +289,6 @@ spin_delay(void) /* S/390 and S/390x Linux (32- and 64-bit zSeries) */ #if defined(__s390__) || defined(__s390x__) -#define HAS_TEST_AND_SET typedef unsigned int slock_t; @@ -321,7 +317,6 @@ tas(volatile slock_t *lock) * acquire/release semantics. The CPU will treat superfluous members as * NOPs, so it's just code space. */ -#define HAS_TEST_AND_SET typedef unsigned char slock_t; @@ -392,7 +387,6 @@ do \ /* PowerPC */ #if defined(__ppc__) || defined(__powerpc__) || defined(__ppc64__) || defined(__powerpc64__) -#define HAS_TEST_AND_SET typedef unsigned int slock_t; @@ -453,7 +447,6 @@ do \ #if defined(__mips__) && !defined(__sgi) /* non-SGI MIPS */ -#define HAS_TEST_AND_SET typedef unsigned int slock_t; @@ -531,10 +524,9 @@ do \ * grounds that that's known to be more likely to work in the ARM ecosystem. * (But we dealt with ARM above.) */ -#if !defined(HAS_TEST_AND_SET) +#if !defined(TAS) #if defined(HAVE_GCC__SYNC_INT32_TAS) -#define HAS_TEST_AND_SET #define TAS(lock) tas(lock) @@ -549,7 +541,6 @@ tas(volatile slock_t *lock) #define S_UNLOCK(lock) __sync_lock_release(lock) #elif defined(HAVE_GCC__SYNC_CHAR_TAS) -#define HAS_TEST_AND_SET #define TAS(lock) tas(lock) @@ -565,7 +556,7 @@ tas(volatile slock_t *lock) #endif /* HAVE_GCC__SYNC_INT32_TAS */ -#endif /* !defined(HAS_TEST_AND_SET) */ +#endif /* !defined(TAS) */ /* @@ -592,12 +583,11 @@ tas(volatile slock_t *lock) * --------------------------------------------------------------------- */ -#if !defined(HAS_TEST_AND_SET) /* We didn't trigger above, let's try here */ +#if !defined(TAS) /* We didn't trigger above, let's try here */ #ifdef _MSC_VER typedef LONG slock_t; -#define HAS_TEST_AND_SET #define TAS(lock) (InterlockedCompareExchange(lock, 1, 0)) #define SPIN_DELAY() spin_delay() @@ -649,11 +639,13 @@ spin_delay(void) #endif -#endif /* !defined(HAS_TEST_AND_SET) */ +#endif /* !defined(TAS) */ /* Blow up if we didn't have any way to do spinlocks */ -#ifndef HAS_TEST_AND_SET +#ifdef TAS +#define HAS_TEST_AND_SET +#else #error PostgreSQL does not have spinlock support on this platform. Please report this to [email protected]. #endif @@ -697,13 +689,6 @@ extern void s_unlock(volatile slock_t *lock); #define SPIN_DELAY() ((void) 0) #endif /* SPIN_DELAY */ -#if !defined(TAS) -extern int tas(volatile slock_t *lock); /* in port/.../tas.s, or - * s_lock.c */ - -#define TAS(lock) tas(lock) -#endif /* TAS */ - #if !defined(TAS_SPIN) #define TAS_SPIN(lock) TAS(lock) #endif /* TAS_SPIN */ -- 2.50.1 (Apple Git-155)
