Обсуждение: 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

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 coverage
> 
> Which 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