Add C11 standard atomic support #1763

sgstreet · 2024-07-09T18:10:01Z

This new version of the PR #1645 which was merged, closed and reverted. This version removes all usage of the GCC specific optimize attribute.

… lock state types

sgstreet · 2024-07-09T23:34:22Z

@kilograham @petrhosek Here is some fixes for the MacOS and Windows compilation failures. I also fixed a problem with the preinit section so it works as intended. Is is possible to launch the build checks so I can verify the fixes?

sgstreet · 2024-07-09T23:39:24Z

FYI the error:

D:/a/pico-sdk/pico-sdk/src/rp2_common/pico_atomic/pico_atomic.c:208:21: error: mismatch in return type of built-in function '__atomic_fetch_sub_4'; expected 'unsigned int' [-Werror=builtin-declaration-mismatch]
  208 | __optimize uint32_t __atomic_fetch_sub_4(volatile void *mem, uint32_t val, __unused int model) {
      |                     ^~~~~~~~~~~~~~~~~~~~

This is a GCC bug as uint32_t and unsigned int are the same type on the ARMv6 arch and interesting the other atomic types do not generate compiler warnings.

lurch · 2024-07-09T23:40:19Z

src/rp2_common/pico_atomic/pico_atomic.c

-    volatile uint32_t *ptr = mem;
-    uint32_t state = __atomic_lock(mem);
-    uint32_t result = *ptr;
+unsigned int __atomic_fetch_add_4(volatile void *mem, unsigned int val, __unused int model) {


It looks like this (and the similiar foo_4 functions) should probably have remained as

uint32_t __atomic_fetch_add_4(volatile void *mem, uint32_t val, __unused int model) {

?

Esepcially as the other functions have signatures like:

uint8_t __atomic_fetch_add_1(volatile void *mem, uint8_t val, __unused int model) { uint16_t __atomic_fetch_add_2(volatile void *mem, uint16_t val, __unused int model) { uint64_t __atomic_fetch_add_8(volatile void *mem, uint64_t val, __unused int model) {

I agree but there is a GCC bug here when -Wextra is enabled. Since uint32_t and unsigned int are the same type on ARMv6 this should not be a warning nor error. See https://github.com/raspberrypi/pico-sdk/actions/runs/9848449043/job/27190379443. The current linux cmake configuration does not enable -Wextra but I'm not clear how to enable it as my CMake skill are not great.

Ah, sorry! I made this comment from just looking at the diff view on GitHub, before realising that you'd actually specifically commented on this already!! 🤦

No, no I was hacking up a comment at the same time as you were reviewing and commenting.

sgstreet · 2024-07-09T23:40:35Z

Is the way the turn on -Wextra for the linux builds so that any further problems are exposed before I push more to the PR.

kilograham · 2024-07-10T02:16:33Z

Ah, sorry, a lot going on, so maybe didn't mention this yesterday: since when we have a rather different implementation from a separate source.

sgstreet · 2024-07-10T02:53:16Z

So no interest in this?

kilograham · 2024-07-10T14:17:08Z

yup; no need for further fixes in this PR; we took it internally to fix the issues (compile, and things like "save" size that you have now fixed)...it will drop in as part of another PR

kilograham · 2024-07-11T20:08:02Z

@sgstreet did you actually verify that your functions are called by GCC; I saw that you #undefed a few functions, but I see on GCC that it just uses its own intrinsics by default

sgstreet · 2024-07-11T21:13:11Z

Yes I tested it, the functions

atomic_flag_test_and_set
atomic_flag_test_and_set_explicit
atomic_flag_clear
atomic_flag_clear_explicit

are handled by GCC slightly differently than others, thus the overide. This following is an excerpt from my test map file:

 .text.__atomic_init
                0x10003e3c        0xc CMakeFiles/atomic-test.dir/pico-sdk/src/rp2_common/pico_atomic/pico_atomic.c.obj
 .text.__atomic_fetch_add_4
                0x10003e48       0x98 CMakeFiles/atomic-test.dir/pico-sdk/src/rp2_common/pico_atomic/pico_atomic.c.obj
                0x10003e48                __atomic_fetch_add_4
 .text.__atomic_compare_exchange_4
                0x10003ee0       0xb4 CMakeFiles/atomic-test.dir/pico-sdk/src/rp2_common/pico_atomic/pico_atomic.c.obj
                0x10003ee0                __atomic_compare_exchange_4
 .text.__atomic_test_and_set_m0
                0x10003f94       0xa0 CMakeFiles/atomic-test.dir/pico-sdk/src/rp2_common/pico_atomic/pico_atomic.c.obj
                0x10003f94                __atomic_test_and_set_m0
 .text.__atomic_clear_m0
                0x10004034        0xc CMakeFiles/atomic-test.dir/pico-sdk/src/rp2_common/pico_atomic/pico_atomic.c.obj
                0x10004034                __atomic_clear_m0

Did you decide to take PR?

sgstreet · 2024-07-11T21:16:57Z

I have used the extensively in a slightly different form pico-tookit See https://github.com/sgstreet/pico-toolkit/blob/a9172fc66659217560a13445877723b804a71347/test/atomic-test/atomic-test.c#L12.

kilograham · 2024-07-11T21:18:10Z

What functions was your test calling?

Did you decide to take PR?

I am not taking the PR as Clang works somewhat differently; i'm trying to take the best parts of both (yours and another), but when i tried calling this on GCC

volatile atomic_uint_fast32_t foo;
uint32_t bar = atomic_load(&foo);

it uses its own non multi-core aware code; are you using explcit methods of the right size? also what GCC version?

sgstreet · 2024-07-11T22:11:35Z

Disappointed you did not take the PR nor ask for a clang compatible solution as I would have researched it and built one.

But to answer you question, please consider how the AHB Lite Bus operates. 1, 2 and 4 byte bus loads and stores are guaranteed atomic with respect to multi-masters. In some circumstances (not so important on ARMv6-m), a memory barrier will be required to ensure memory ordering. I'm sure you examined the generated code for the atomic_load above and saw the dmb ish or dmb sys instructions. The generated load/store code is sufficient and there is no need to implement replacements. This is not the case for 8 byte quantities and atomic read/modify/write functions.

If you replace atomic_uint_fast32_t with atomic_uint_fast64_t you should see in the disassembly a call to __atomic_load_8. Further, any call to a read/modify/write atomic function such as atomic_exchange will generate a function call to one of the size matched functions in the library.

How are you implementing the multi-core awareness? Using the hardware semaphores? If so how many semaphore bits are you allocating? I have code which does this, but uses all 32 semaphore bits, not a big deal as once you have atomic variables the need for the raw hardware semaphores is reduced. This of course is not compatible with the SDK as current implemented. This could be reduced to 8 or 16 bits with minor impact to performance. Just curious really.

Hope this helps.

sgstreet · 2024-07-11T22:27:43Z

There is not a write buffer in front of the SRAM correct?

kilograham · 2024-07-12T01:16:18Z

But to answer you question, please consider how the AHB Lite Bus operates. 1, 2 and 4 byte bus loads and stores are guaranteed atomic with respect to multi-masters

Yes, sorry i wasn't thinking thru; CLang still emits calls on m0plus, and I was just comparing that with GCC.

Disappointed you did not take the PR nor ask for a clang compatible solution as I would have researched it and built one.

I needed it yesterday, and when i first took the PR, there were a bunch of glaring bugs (which you have since fixed)... this interacts with some other stuff we're doing, so I didnt have time to go back and forth and fix the issues on github. thanks for your help, the spirit of your code is intact!!

kilograham · 2024-07-12T02:24:13Z

There is not a write buffer in front of the SRAM correct?

correct

kilograham · 2024-08-09T19:57:12Z

fixed in SDK 2.0.0 thanks for your PR

sgstreet and others added 9 commits July 7, 2024 14:52

Add runtime support for stdatomics

1a2cbff

Fix lock calculation and enable atomic_flag support

ca51664

Put include headers on correct library

263fb7e

Use get_core_num() instead of accessing SIO explicitly

3df9312

Add RPi copyright

594b34e

add rpi copyright

2ef268a

blind addition of __unused to fix -Werror

47d9d34

two more compiler warnings (well one correct)

ed14084

Remove clang unsupport optimization attribute

f21a54e

sgstreet mentioned this pull request Jul 9, 2024

Add C11 standard atomic support #1645

Merged

sgstreet changed the title ~~Add atomic support~~ Add C11 standard atomic support Jul 9, 2024

sgstreet mentioned this pull request Jul 9, 2024

Runtime for C11 atomics missing #1642

Closed

Fix builtin function signatures, preinit section handling and cleanup…

69bcb62

… lock state types

lurch reviewed Jul 9, 2024

View reviewed changes

sgstreet requested a review from lurch July 10, 2024 00:06

lurch removed their request for review July 10, 2024 16:51

kilograham added this to the 1.6.2 milestone Jul 19, 2024

kilograham modified the milestones: 1.6.2, 2.0.0 Aug 8, 2024

kilograham closed this Aug 9, 2024

Add C11 standard atomic support #1763

Add C11 standard atomic support #1763

Uh oh!

Conversation

sgstreet commented Jul 9, 2024

Uh oh!

sgstreet commented Jul 9, 2024

Uh oh!

sgstreet commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lurch Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

sgstreet Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lurch Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

sgstreet Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

sgstreet commented Jul 9, 2024

Uh oh!

kilograham commented Jul 10, 2024

Uh oh!

sgstreet commented Jul 10, 2024

Uh oh!

kilograham commented Jul 10, 2024

Uh oh!

kilograham commented Jul 11, 2024

Uh oh!

sgstreet commented Jul 11, 2024

Uh oh!

sgstreet commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kilograham commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgstreet commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgstreet commented Jul 11, 2024

Uh oh!

kilograham commented Jul 12, 2024

Uh oh!

kilograham commented Jul 12, 2024

Uh oh!

kilograham commented Aug 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sgstreet commented Jul 9, 2024 •

edited

Loading

sgstreet Jul 9, 2024 •

edited

Loading

sgstreet commented Jul 11, 2024 •

edited

Loading

kilograham commented Jul 11, 2024 •

edited

Loading

sgstreet commented Jul 11, 2024 •

edited

Loading