[Bug target/109166] New: Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[Bug target/109166] New: Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[jdx at o2 dot pl]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166

            Bug ID: 109166
           Summary: Built-in  __atomic_test_and_set does not seem to be
                    atomic on ARMv4T
           Product: gcc
           Version: 11.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jdx at o2 dot pl
  Target Milestone: ---
              Host: x86_64-w64-mingw32
            Target: arm-eabi

For the following source code:

bool tas(unsigned char *ptr)
{
    return __atomic_test_and_set(ptr, __ATOMIC_SEQ_CST);
}

gcc -march=armv4t -marm -O3 -Wall -Wextra gives:

tas:
        mov     r3, r0
        mov     r2, #1
        ldrb    r0, [r0]        @ zero_extendqisi2
        strb    r2, [r3]
        bx      lr

I am not an ARM assembler expert, but I think that LDRB/STRB pair should be
replaced with SWPB as shown here: https://godbolt.org/z/116MbYK79.

See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107567.

[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://inbox.sourceware.org/gcc-patches/4F596367.2050001@redhat.com/

[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Armv4t does not have smp so the question is how do you think the below is not
atomic? Yes interrupts but that requires more.

[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[jdx at o2 dot pl]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166

--- Comment #3 from Jan Dubiec <jdx at o2 dot pl> ---
I do not get what "but that requires more" means in this context.

Lets assume that two threads test and set the same memory location which
initial value is 0 ("unlocked"/"false"). Now, when the first thread gets
preempted (a timer interrupt is requested) just after LDRB (i.e.
__atomic_test_and_set will return 0) and it happens that the second thread gets
its time slot, then it may happen that __atomic_test_and_set called by the
second thread also will return 0 and bam! – both threads will have "exclusive"
access to a shared resource. See get_lock() from sample libatomic as a typical
example of TAS usage:
https://gcc.gnu.org/wiki/Atomic/GCCMM?action=AttachFile&do=view&target=libatomic.c.

[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Read that thread I pointed to.

[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[jdx at o2 dot pl]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166

--- Comment #5 from Jan Dubiec <jdx at o2 dot pl> ---
I read that thread a few days ago and I understand concerns regarding SWP, in
particular on ARMv6 which has made SWP obsolete (AFAIR it is optional on
ARMv6-A/R, ARMv6-M has neither SWP nor LDREX/STREX). However I have never heard
of an ARMv4T based MCU which does not have SWP. So I do not understand why gcc
targeting various "bare metal" ARMs for ARMv4T a) emits non-atomic
__atomic_test_and_set, b) does not use SWP in that code. Especially that the
code attached to the linked message for ARMv4T uses SWP.

[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166

Hans-Peter Nilsson <hp at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
                 CC|                            |hp at gcc dot gnu.org
   Last reconfirmed|                            |2023-09-22

--- Comment #6 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
Confirmed at r14-4210-g94982a6b9cf4 for arm-unknown-eabi cross from x86-linux.

I was about to enter a mostly-identical report.  Exactly the same code is still
emitted (as expected with the bug present).  It's not just ARMv4T, it's any ARM
architecture variant below ARMv7.

I expect swp *or at least a call to a library function to be emitted, as
happens for all other builtin atomics*.  I think this much is hard to dispute. 
The cause  I guess, is just a bad fall-through in the arm/sync.md.

For reference, compare to the code emitted at -O2 for:
unsigned char f1 (unsigned char *x)
{
  unsigned char c = *x;
  *x = 1;
  return c;
}
(for which the same code is emitted) and also:
unsigned char f2 (volatile unsigned char *x)
{
  return __sync_lock_test_and_set (x, 1);
}
(note the call to the __sync_lock_test_and_set_1 library function)

A related issue is that those gcc doesn't provide those library functions - but
that's a whole different bug.

Though, I agree it seems that SWP should be used in preference to library
calls.  Not being ARM-literate, I did some digging which indicates that SWP is
present on "V2S" and later, i.e. including armv4t, which happens to be the
default for arm-eabi, see config.gcc entry for arm-eabi, observe
'target_cpu_cname="arm7tdmi"' (and if necessary consult
gcc/config/arm/arm-cpus.in).  I don't know what runtime environment caveats
there are for "swp" to work, or why rth's patch was never committed (referring
to the thread pointed to in #c2).

[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166

--- Comment #7 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Hans-Peter Nilsson from comment #6)
> The cause  I guess, is just a bad fall-through in the arm/sync.md.

Or rather, optabs.cc:expand_atomic_test_and_set, which makes this issue
somewhat less target-specific.  To wit:
/* Failing all else, assume a single threaded environment and simply
   perform the operation.  */

[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[rearnsha at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166

Richard Earnshaw <rearnsha at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |WONTFIX
             Status|NEW                         |RESOLVED

--- Comment #8 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
I'm going to close this as WONTFIX.

There are several reasons for this.

There's no SWPH operation, so it's impossible to generalize atomic operations
for all basic data types.  It's not possible to synthesize a 16-bit atomic type
with either SWP or SWPB.

There's no support in Thumb state for SWP[B].

The instruction was removed in later versions of the architecture, which makes
code non-portable.

Finally, Armv4, which dates to around 1995, is essentially in maintenance only
mode and this is really a new feature request.  In fact, I don't think we'd
really want to add new features for anything before Armv7 these days (even that
is more than 10 years old).

[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166

--- Comment #9 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to Richard Earnshaw from comment #8)
> I'm going to close this as WONTFIX.

I guess I'll have to find another PR to lean on, for fixing the underlying
cause for the nonatomic code.

[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Hans-Peter Nilsson <hp@gcc.gnu.org>:

https://gcc.gnu.org/g:8e6757b30d0f3f13d47d0f842801a751ba6293c2

commit r14-4286-g8e6757b30d0f3f13d47d0f842801a751ba6293c2
Author: Hans-Peter Nilsson <hp@axis.com>
Date:   Sat Sep 23 05:06:52 2023 +0200

    __atomic_test_and_set: Fall back to library, not non-atomic code

    Make __atomic_test_and_set consistent with other __atomic_ and __sync_
    builtins: call a matching library function instead of emitting
    non-atomic code when the target has no direct insn support.

    There's special-case code handling targetm.atomic_test_and_set_trueval
    != 1 trying a modified maybe_emit_sync_lock_test_and_set.  Previously,
    if that worked but its matching emit_store_flag_force returned NULL,
    we'd segfault later on.  Now that the caller handles NULL, gcc_assert
    here instead.

    While the referenced PR:s are ARM-specific, the issue is general.

            PR target/107567
            PR target/109166
            * builtins.cc (expand_builtin) <case BUILT_IN_ATOMIC_TEST_AND_SET>:
            Handle failure from expand_builtin_atomic_test_and_set.
            * optabs.cc (expand_atomic_test_and_set): When all attempts fail to
            generate atomic code through target support, return NULL
            instead of emitting non-atomic code.  Also, for code handling
            targetm.atomic_test_and_set_trueval != 1, gcc_assert result
            from calling emit_store_flag_force instead of returning NULL.