[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166 --- Comment #10 from CVS Commits --- The master branch has been updated by Hans-Peter Nilsson : https://gcc.gnu.org/g:8e6757b30d0f3f13d47d0f842801a751ba6293c2 commit r14-4286-g8e6757b30d0f3f13d47d0f842801a751ba6293c2 Author: Hans-Peter Nilsson 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) : 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.
[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166 --- Comment #9 from Hans-Peter Nilsson --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166 Richard Earnshaw changed: What|Removed |Added Resolution|--- |WONTFIX Status|NEW |RESOLVED --- Comment #8 from Richard Earnshaw --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166 --- Comment #7 from Hans-Peter Nilsson --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166
Hans-Peter Nilsson 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 ---
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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166 --- Comment #5 from Jan Dubiec --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166 --- Comment #4 from Andrew Pinski --- Read that thread I pointed to.
[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166
--- Comment #3 from Jan Dubiec ---
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=view=libatomic.c.
[Bug target/109166] Built-in __atomic_test_and_set does not seem to be atomic on ARMv4T
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166 --- Comment #2 from Andrew Pinski --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109166 --- Comment #1 from Andrew Pinski --- https://inbox.sourceware.org/gcc-patches/4f596367.2050...@redhat.com/
