typo in fallback implementation for pg_atomic_test_set_flag()
I noticed that the fallback pg_atomic_test_set_flag_impl() implementation
that uses atomic-exchange is giving pg_atomic_exchange_u32_impl() an extra
argument. This appears to be copy/pasted from the atomic-compare-exchange
version a few lines down. It looks like it's been this way since this code
was introduced in commit b64d92f (2014). Patch attached.
I'd ordinarily suggest removing this section of code since it doesn't seem
to have gotten much coverage, but I'm actually looking into adding some
faster atomic-exchange implementations that may activate this code for
certain compiler/architecture combinations.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
fix_atomic_exchange_typo_v1.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/include/port/atomics/generic.h b/src/include/port/atomics/generic.h
index cb5804adbf..95d99dd0be 100644
--- a/src/include/port/atomics/generic.h
+++ b/src/include/port/atomics/generic.h
@@ -83,7 +83,7 @@ pg_atomic_init_flag_impl(volatile pg_atomic_flag *ptr)
static inline bool
pg_atomic_test_set_flag_impl(volatile pg_atomic_flag *ptr)
{
- return pg_atomic_exchange_u32_impl(ptr, &value, 1) == 0;
+ return pg_atomic_exchange_u32_impl(ptr, 1) == 0;
}
#define PG_HAVE_ATOMIC_UNLOCKED_TEST_FLAG
Hi,
On 2023-11-13 21:54:39 -0600, Nathan Bossart wrote:
I noticed that the fallback pg_atomic_test_set_flag_impl() implementation
that uses atomic-exchange is giving pg_atomic_exchange_u32_impl() an extra
argument. This appears to be copy/pasted from the atomic-compare-exchange
version a few lines down. It looks like it's been this way since this code
was introduced in commit b64d92f (2014). Patch attached.
Oops.
I guess it's not too surprising this wasn't required - if the compiler has any
atomic intrinsics it's going to have support for the flag stuff. And there's
practically no compiler that
Are you planning to apply the fix?
I'd ordinarily suggest removing this section of code since it doesn't seem
to have gotten much coverage
Which section precisely?
but I'm actually looking into adding some faster atomic-exchange
implementations that may activate this code for certain
compiler/architecture combinations.
Hm. I don't really see how adding a faster atomic-exchange implementation
could trigger this implementation being used?
Greetings,
Andres Freund
On Tue, Nov 14, 2023 at 07:17:32PM -0800, Andres Freund wrote:
Are you planning to apply the fix?
Yes, I'll take care of it.
I'd ordinarily suggest removing this section of code since it doesn't seem
to have gotten much coverageWhich section precisely?
The lines below this:
/*
* provide fallback for test_and_set using atomic_exchange if available
*/
#if !defined(PG_HAVE_ATOMIC_TEST_SET_FLAG) && defined(PG_HAVE_ATOMIC_EXCHANGE_U32)
but above this:
/*
* provide fallback for test_and_set using atomic_compare_exchange if
* available.
*/
#elif !defined(PG_HAVE_ATOMIC_TEST_SET_FLAG) && defined(PG_HAVE_ATOMIC_COMPARE_EXCHANGE_U32)
but I'm actually looking into adding some faster atomic-exchange
implementations that may activate this code for certain
compiler/architecture combinations.Hm. I don't really see how adding a faster atomic-exchange implementation
could trigger this implementation being used?
That'd define PG_HAVE_ATOMIC_EXCHANGE_U32, so this fallback might be used
if PG_HAVE_ATOMIC_TEST_SET_FLAG is not defined. I haven't traced through
all the #ifdefs that lead to this point exhaustively, though, so perhaps
this is still unlikely.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, Nov 15, 2023 at 09:52:34AM -0600, Nathan Bossart wrote:
On Tue, Nov 14, 2023 at 07:17:32PM -0800, Andres Freund wrote:
Are you planning to apply the fix?
Yes, I'll take care of it.
Committed and back-patched. I probably could've skipped back-patching this
one since it doesn't seem to be causing any problems yet, but I didn't see
any reason not to back-patch, either.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com