core: Provide all C11 atomic operations by jnohlgard · Pull Request #5688 · RIOT-OS/RIOT

jnohlgard · 2016-07-25T13:43:24Z

This PR adds the remaining __atomic library functions for C11 support and also provides the __sync functions for older GCC versions (msp430).

All atomic.h uses in the tree have been replaced by their C11 equivalents.

The functions provided here are only used when the compiler doesn't know how to do a lockless atomic operation for the given target architecture. Therefore, with a recent GCC, most of these functions are only used on Cortex-M0, MSP430.

Note: This PR works fine with -std=gnu99 (the current setting), even though the title says C11

jnohlgard · 2016-07-25T13:46:56Z

Kijewski · 2016-07-25T13:52:21Z

core/atomic_c11.c

+    (void) memorder;
+    unsigned int mask = irq_disable();
+    memcpy(ret, ptr, size);
+    memcpy(ptr, val, size);


No line memcpy(val, ret, size);?

It is implemented as the manual specifies it:

https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html

jnohlgard · 2016-07-25T13:53:28Z

msp430-libc was missing stdatomic.h so I copied Newlib's header and the required support headers to msp430-common (BSD license).

Full C11 atomic functionality is still provided when compiling with -std=gnu99, at least for the newlib platforms, so we don't need to switch to -std=gnu11 yet.

kaspar030 · 2016-08-08T20:46:02Z

Nice! Did you do some codesize comparisons?

jnohlgard · 2016-08-08T22:48:31Z

@kaspar030 I did not, it's difficult to find anything to compare since the unit tests had to be modified. The emb6 test app saved 28 bytes of ROM on samr21-xpro. Almost nothing was using atomic.h anymore before this PR so its removal was well deserved.

DipSwitch · 2016-08-09T06:16:24Z

Does this override the GCC buildins which for ARM use LDREX and STREX by default? Instead of disabling and enabling the interrupt?

jnohlgard · 2016-08-09T08:13:46Z

@DipSwitch GCC only generates library calls if there is no lock-free hardware implementation available. If building for a platform which have atomic operations in hardware then GCC will generate the proper instructions instead (LDREX, STREX on Cortex-M3,M4)

You can verify by building the tests-core in tests/unittests and inspecting the disassembly (make tests-core && arm-none-eabi-objdump -d bin/$BOARD/tests-core/tests-core-atomic.o)

josephnoir · 2016-08-30T18:12:03Z

Admittedly, I don't understand most of the changes and thus have a hard time reviewing the changes.

Edit: I noticed that the files I have most problems with are imported from newlib. Do we assume those to be correct?

jnohlgard · 2016-08-30T19:12:09Z

@josephnoir that's OK, I'll reassign.
@kaspar030 do you mind taking a look at this?

miri64 · 2016-10-31T17:52:03Z

Kaspar is on vacation, so I need to postpone this one :(

jnohlgard · 2016-12-01T08:12:01Z

Ping @kaspar030
Any other maintainer who would be willing to review this in time for christmas?
It cleans up the implementation quite a lot and it is not overly complex, despite the seemingly large change set.

jnohlgard · 2016-12-20T14:23:26Z

ping maintainers!
Can we get this merged in time for 2017.01?

…eatures.h Used in msp430-common

jnohlgard · 2017-02-08T15:25:08Z

@OlegHahm ping!

kaspar030 · 2017-02-08T15:37:46Z

I just tested the core unittests on msb-430h, looks good!

2017-02-08 16:37:06,321 - INFO # RIOT MSP430 hardware initialization complete.
2017-02-08 16:37:06,322 - INFO #
2017-02-08 16:37:06,330 - INFO # main(): This is RIOT! (Version: 2017.04-devel-131-gf0551-6b9ed7c519ae-pr5688)
2017-02-08 16:37:08,213 - INFO # .....................................................................................
2017-02-08 16:37:08,216 - INFO # OK (85 tests)

kaspar030

ACK from my side. Thanks for all the work here, must have been awful to keep the "old" platforms supported!

jnohlgard · 2017-02-08T15:57:57Z

Fixes #6491
(to let GitHub auto-close the issue)

jnohlgard · 2017-02-08T16:24:14Z

Core change needs a second ACK. @melshuber, @OlegHahm, @Kijewski?

@kaspar030 Thanks, I have felt responsible for this since I was the one who introduced the old incomplete atomic.h API. It feels nice to finally make the support complete, and also standards compliant.

The resulting assembly code for a simple load is quite heavy (see #6491 (comment)), but I guess that's an optimization we can postpone until it becomes an issue.

melshuber

ACK

One could argue to leave the old API (maybe by calling the implementation) for compaibility issues

jnohlgard · 2017-02-09T08:00:11Z

@melshuber I would prefer to not leave the old implementation in, since it was not really working as intended (ATOMIC_VALUE on avr for example)

melshuber · 2017-02-09T08:47:16Z

@gebart well enough, I just wanted to mention it

jnohlgard added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: core Area: RIOT kernel. Handle PRs marked with this with care! labels Jul 25, 2016

jnohlgard added this to the Release 2016.10 milestone Jul 25, 2016

jnohlgard assigned OlegHahm Jul 25, 2016

jnohlgard force-pushed the pr/atomics-refactor branch from 8812c3c to 618c595 Compare July 25, 2016 13:45

jnohlgard mentioned this pull request Jul 25, 2016

Volatile might not be the best (or even right) implementation for atomics #4700

Closed

Kijewski reviewed Jul 25, 2016
View reviewed changes

josephnoir mentioned this pull request Jul 26, 2016

cpp11: clang doesn't allow mutex_t to be used with constexpr #4583

Closed

OlegHahm assigned josephnoir and BytesGalore and unassigned OlegHahm Jul 27, 2016

jnohlgard mentioned this pull request Jul 30, 2016

core: atomic: add atomic_and() and atomic_or() #4888

Closed

jnohlgard mentioned this pull request Aug 8, 2016

core: recursive mutex implementation #5731

Merged

jnohlgard force-pushed the pr/atomics-refactor branch from 618c595 to 76707eb Compare August 8, 2016 22:23

jnohlgard assigned kaspar030 and unassigned josephnoir Aug 30, 2016

jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 14, 2016

miri64 modified the milestones: Release 2017.01, Release 2016.10 Oct 31, 2016

Joakim Nohlgård added 16 commits February 8, 2017 16:23

core/include/mutex.h: remove unused #include "atomic.h"

d322af3

gnrc/pktbuf: remove unused #include "atomic.h"

7376419

core/c11_atomic.c: Add remaining C11 atomic library helpers for GCC

6d20ac0

core: rename c11_atomic -> atomic_c11

fcdb157

core: add __sync_x library helpers for <gcc-4.7

4297e06

msp430-common: Add stdatomic.h, sys/cdefs.h, sys/features.h from newlib

0cb3848

msp430_common: Add supplemental __clzhi2, __ctzhi2, for old MSPGCC

ee0cae6

sys/posix/pthread: Use C11 atomic instead of atomic.h

597db03

tests/emb6: Use stdatomic.h instead of custom atomic.h

c1b84f0

tests/unittests/core-atomic: Use stdatomic.h instead of custom atomic.h

cb34eed

cortexm_common: Remove atomic_arch

20b1846

x86: Remove atomic_arch

83c2630

core: Remove atomic in favour of C11 stdatomic

4fdd2ea

dist/tools/licenses: Add BSD 1 clause style license from newlib sys/f…

4bc120f

…eatures.h Used in msp430-common

doc/doxygen: Exclude msp430-common imported newlib headers

bec5b57

sys/posix: Add missing Doxygen comment on spinlock flag member

f055137

jnohlgard force-pushed the pr/atomics-refactor branch from be63739 to f055137 Compare February 8, 2017 15:24

jnohlgard mentioned this pull request Feb 8, 2017

kinetis: Add DMA support to UART driver #4931

Closed

kaspar030 approved these changes Feb 8, 2017

View reviewed changes

melshuber approved these changes Feb 8, 2017

View reviewed changes

jnohlgard merged commit 0941078 into RIOT-OS:master Feb 9, 2017

jnohlgard deleted the pr/atomics-refactor branch February 9, 2017 08:00

jnohlgard mentioned this pull request Feb 9, 2017

vfs: file system abstraction #5616

Merged

55 tasks

haukepetersen mentioned this pull request Feb 9, 2017

native/atmega broken on Mint17 (gcc 4.8.4) #6583

Closed

Conversation

jnohlgard commented Jul 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnohlgard commented Jul 25, 2016

Uh oh!

Kijewski Jul 25, 2016

Choose a reason for hiding this comment

Uh oh!

jnohlgard Jul 25, 2016

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Jul 25, 2016

Uh oh!

kaspar030 commented Aug 8, 2016

Uh oh!

jnohlgard commented Aug 8, 2016

Uh oh!

DipSwitch commented Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnohlgard commented Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephnoir commented Aug 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnohlgard commented Aug 30, 2016

Uh oh!

miri64 commented Oct 31, 2016

Uh oh!

jnohlgard commented Dec 1, 2016

Uh oh!

jnohlgard commented Dec 20, 2016

Uh oh!

jnohlgard commented Feb 8, 2017

Uh oh!

kaspar030 commented Feb 8, 2017

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Feb 8, 2017

Uh oh!

jnohlgard commented Feb 8, 2017

Uh oh!

melshuber left a comment

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Feb 9, 2017

Uh oh!

melshuber commented Feb 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jnohlgard commented Jul 25, 2016 •

edited

Loading

DipSwitch commented Aug 9, 2016 •

edited

Loading

jnohlgard commented Aug 9, 2016 •

edited

Loading

josephnoir commented Aug 30, 2016 •

edited

Loading