From f9df825c8c61f3e4c8597af3cfbccc5d69967872 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Tue, 2 Jul 2024 13:42:46 +0200 Subject: [PATCH v5] Remove bogus assertion in pg_atomic_monotonic_advance_u64 This code wanted to ensure that the 'exchange' variable passed to pg_atomic_compare_exchange_u64 has correct alignment, but apparently platforms don't actually require anything that doesn't come naturally. While messing with pg_atomic_monotonic_advance_u64: instead of using Max() to determine the value to return, just use pg_atomic_compare_exchange_u64()'s return value to decide; also, use pg_atomic_compare_exchange_u64 instead of the _impl version; also remove the unnecessary underscore at the end of variable name "target". Backpatch to 17. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/36796438-a718-cf9b-2071-b2c1b947c1b5@gmail.com --- src/include/port/atomics.h | 17 ++++++----------- src/include/port/atomics/arch-ppc.h | 2 ++ src/include/port/atomics/arch-x86.h | 2 ++ src/include/port/atomics/generic-gcc.h | 3 +++ src/include/port/atomics/generic-sunpro.h | 1 + 5 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h index c911c6b956..f6fa432d2d 100644 --- a/src/include/port/atomics.h +++ b/src/include/port/atomics.h @@ -507,7 +507,6 @@ pg_atomic_compare_exchange_u64(volatile pg_atomic_uint64 *ptr, { #ifndef PG_HAVE_ATOMIC_U64_SIMULATION AssertPointerAlignment(ptr, 8); - AssertPointerAlignment(expected, 8); #endif return pg_atomic_compare_exchange_u64_impl(ptr, expected, newval); } @@ -576,7 +575,7 @@ pg_atomic_sub_fetch_u64(volatile pg_atomic_uint64 *ptr, int64 sub_) * Full barrier semantics (even when value is unchanged). */ static inline uint64 -pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_) +pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target) { uint64 currval; @@ -585,23 +584,19 @@ pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_) #endif currval = pg_atomic_read_u64_impl(ptr); - if (currval >= target_) + if (currval >= target) { pg_memory_barrier(); return currval; } -#ifndef PG_HAVE_ATOMIC_U64_SIMULATION - AssertPointerAlignment(&currval, 8); -#endif - - while (currval < target_) + while (currval < target) { - if (pg_atomic_compare_exchange_u64_impl(ptr, &currval, target_)) - break; + if (pg_atomic_compare_exchange_u64(ptr, &currval, target)) + return target; } - return Max(target_, currval); + return currval; } #undef INSIDE_ATOMICS_H diff --git a/src/include/port/atomics/arch-ppc.h b/src/include/port/atomics/arch-ppc.h index 94ba2597fb..edaab7c895 100644 --- a/src/include/port/atomics/arch-ppc.h +++ b/src/include/port/atomics/arch-ppc.h @@ -173,6 +173,8 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr, uint32 condition_register; bool ret; + AssertPointerAlignment(expected, 8); + /* Like u32, but s/lwarx/ldarx/; s/stwcx/stdcx/; s/cmpw/cmpd/ */ #ifdef HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P if (__builtin_constant_p(*expected) && diff --git a/src/include/port/atomics/arch-x86.h b/src/include/port/atomics/arch-x86.h index 3efa79dc3d..2a8eca30fc 100644 --- a/src/include/port/atomics/arch-x86.h +++ b/src/include/port/atomics/arch-x86.h @@ -207,6 +207,8 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr, { char ret; + AssertPointerAlignment(expected, 8); + /* * Perform cmpxchg and use the zero flag which it implicitly sets when * equal to measure the success. diff --git a/src/include/port/atomics/generic-gcc.h b/src/include/port/atomics/generic-gcc.h index 9d91370fa8..872d2f02af 100644 --- a/src/include/port/atomics/generic-gcc.h +++ b/src/include/port/atomics/generic-gcc.h @@ -240,6 +240,7 @@ static inline bool pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 *expected, uint64 newval) { + AssertPointerAlignment(expected, 8); return __atomic_compare_exchange_n(&ptr->value, expected, newval, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); } @@ -253,6 +254,8 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr, { bool ret; uint64 current; + + AssertPointerAlignment(expected, 8); current = __sync_val_compare_and_swap(&ptr->value, *expected, newval); ret = current == *expected; *expected = current; diff --git a/src/include/port/atomics/generic-sunpro.h b/src/include/port/atomics/generic-sunpro.h index e060c0868a..840a45e778 100644 --- a/src/include/port/atomics/generic-sunpro.h +++ b/src/include/port/atomics/generic-sunpro.h @@ -102,6 +102,7 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr, bool ret; uint64 current; + AssertPointerAlignment(expected, 8); current = atomic_cas_64(&ptr->value, *expected, newval); ret = current == *expected; *expected = current; -- 2.39.2