[Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[Bug target/109933] New: __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[rory.bolt at gmail dot com]

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

            Bug ID: 109933
           Summary: __atomic_test_and_set is broken for BIG ENDIAN riscv
                    targets
           Product: gcc
           Version: 12.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rory.bolt at gmail dot com
  Target Milestone: ---
            Target: riscv64be-unknown-linux-gnu

Created attachment 55136
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55136&action=edit
Simple test program illustrating the bug

PLEASE NOTE: this bug is in reference to BIG ENDIAN riscv targets...

Although reported on riscv64be, it most likely exists for 32 bit targets too.

Please see the attached test program.

The issue appears to be that the value atomically or'ed should be 0x01000000
not 1 for big endian systems, since we want the non-zero value in the lowest
memory address and the amoor.w command is writing a 32 bit value to memory.

The current (broken) implementation will pass a simple unit test, the problem
only manifests itself on a call to __atomic_test_and_set AFTER a previous call
to __atomic_test_and_set to the same address and a __atomic_clear to the same
address.

This problem originally manifested itself in the linux libcap library, which
uses __atomic_test_and_set in conjunction with __atomic_clear to implement a
mutex to guard the manipulation of capabilities.

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This might be fixed on the trunk with recent patches; note those patches are
NOT backportable from what I remember.

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
So it looks like it is still broken.

        andi    a4,s0,3
        slli    a4,a4,3
        li      a2,1
        andi    a3,s0,-4
        sll     a2,a2,a4
        amoor.w.aqrl    a5,a2,0(a3)



        andi    a4,s0,3
        slli    a4,a4,3
        li      a2,1
        andi    a3,s0,-4
        sll     a2,a2,a4
        amoor.w.aqrl    a5,a2,0(a3)

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-05-23
             Status|UNCONFIRMED                 |NEW

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[patrick at rivosinc dot com]

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

Patrick O'Neill <patrick at rivosinc dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |patrick at rivosinc dot com

--- Comment #3 from Patrick O'Neill <patrick at rivosinc dot com> ---
The recent patches just copied the libatomic code inline

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[patrick at rivosinc dot com]

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

--- Comment #4 from Patrick O'Neill <patrick at rivosinc dot com> ---
Sorry about the nonsense comment, I clicked enter too early. I'm taking a look
at this.

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[patrick at rivosinc dot com]

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

--- Comment #5 from Patrick O'Neill <patrick at rivosinc dot com> ---
I don't have a big-endian environment set up to validate the issue/test a fix
and I likely won't be able to get to this for a while.

The relevant code is here (untouched by the recent patches):
Trunk
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/sync.md;h=9fc626267de3840ce15d196bff72440a980fd234;hb=HEAD#l535

GCC 12
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/sync.md;h=86b41e6b00a9331ed4b46e73cc66c102c6d2d981;hb=2ee5e4300186a92ad73f1a1a64cb918dc76c8d67#l155

The main difference from 12->trunk in this testcase is that AMOOR now uses
.aqrl instead of a fence. That does not affect this testcase, so a fix on trunk
should be applicable to the 12 branch (and vice-versa).

Thanks for the bug report!

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[rory.bolt at gmail dot com]

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

--- Comment #6 from Rory Bolt <rory.bolt at gmail dot com> ---
Ah... that code makes so much sense now...

So my original comment about simply using a different constant was too
simplistic; what is being attempted is to shift the constant 1 into the correct
byte position since the flag is only a single byte but the atomic store is done
on a word... so the shift logic will need to be rewritten for big endian
targets. This also explains the masking of the low order address bits...

Interesting!

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[palmer at gcc dot gnu.org]

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

palmer at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |palmer at gcc dot gnu.org

--- Comment #7 from palmer at gcc dot gnu.org ---
(In reply to Rory Bolt from comment #6)
> Ah... that code makes so much sense now...
> 
> So my original comment about simply using a different constant was too
> simplistic; what is being attempted is to shift the constant 1 into the
> correct byte position since the flag is only a single byte but the atomic
> store is done on a word... so the shift logic will need to be rewritten for
> big endian targets. This also explains the masking of the low order address
> bits...
> 
> Interesting!

That seems likely to be the culprit.  Do you have time to send a patch?

We should probably also poke through the other sub-word patterns and make sure
nothing else got dropped for BE.

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[rory.bolt at gmail dot com]

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

--- Comment #8 from Rory Bolt <rory.bolt at gmail dot com> ---
So...

The logic for this is simple:

For little endian the shift amount is ((address & 3) * 8)

For big endian the shift amount is ((3 -(address & 3)) * 8)

Unfortunately I have ZERO experience modifying GCC, and the mechanism to
determine if it is generating big endian code or little endian code is not
obvious to me...

So working on this in my spare time it will be a while for me to create a
patch. That said, I do have a full big endian linux environment so I can test a
patch (relatively quickly - it takes a while to build GCC ;-)) if some one
beats me to this.

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[rory.bolt at gmail dot com]

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

--- Comment #9 from Rory Bolt <rory.bolt at gmail dot com> ---
Created attachment 55153
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55153&action=edit
patch

Tested fix for big endian, NOT tested on little endian

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[rory.bolt at gmail dot com]

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

--- Comment #10 from Rory Bolt <rory.bolt at gmail dot com> ---
Tested and verified on little endian too.

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[patrick at rivosinc dot com]

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

--- Comment #11 from Patrick O'Neill <patrick at rivosinc dot com> ---
I can confirm that your fix does *not* break the testsuite on little endian.

We also recently added inline subword atomics to GCC 13, will this code also
need to change for big endian?

Trunk:
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/riscv.cc;h=09fc9e5d95e611f94bc05b4851fef6f50a651c28;hb=HEAD#l7439

GCC-13 branch:
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/riscv.cc;h=5f44f6dc5c9c4ad2416e4195570473ab49c2e535;hb=6506590e70e57ed8d7fb68ab9443e31c31208fb0#l7146

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[rory.bolt at gmail dot com]

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

--- Comment #12 from Rory Bolt <rory.bolt at gmail dot com> ---
(In reply to Patrick O'Neill from comment #11)
> I can confirm that your fix does *not* break the testsuite on little endian.
> 
> We also recently added inline subword atomics to GCC 13, will this code also
> need to change for big endian?
> 
> Trunk:
> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/riscv.cc;
> h=09fc9e5d95e611f94bc05b4851fef6f50a651c28;hb=HEAD#l7439
> 
> GCC-13 branch:
> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/riscv/riscv.cc;
> h=5f44f6dc5c9c4ad2416e4195570473ab49c2e535;
> hb=6506590e70e57ed8d7fb68ab9443e31c31208fb0#l7146

It looks like the same patch will be required there...

[Bug target/109933] __atomic_test_and_set is broken for BIG ENDIAN riscv targets

[patrick at rivosinc dot com]

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

--- Comment #13 from Patrick O'Neill <patrick at rivosinc dot com> ---
Created attachment 56487
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56487&action=edit
Proposed fix

Hi Rory,

Sorry that this slipped off my plate for way too long.
I just got around to refactoring the surrounding code on trunk and revised your
patch to fix this for both test_and_set along with inline subword amo
sequences.
When you have the time can you please test this using your big-endian setup?
I'll handle the little-endian testing.

Patrick