Add Atomic<T> template #10274

kjbracey · 2019-03-29T14:47:51Z

Description

Add a C++ Atomic<T> template to make atomics even easier. Basically compatible with C++11 std::atomic<T>, but using the underlying core_util_atomic_xxx functions from mbed_atomic.h, so appropriate for synchronising with interrupts and optimised for uniprocessor.

One extra piece of functionality beyond the core_util_atomic_xxx functions is the ability to have an arbitrary atomic type - eg a small structure with 2 uint16_ts can be stored in a uint32_t container.

Based on mbed_cxxsupport.h from #10868.

Requires MBED_STRUCT_STATIC_ASSERT fix now in #10837

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Release Notes

Added Atomic<T> template to make atomics easier in C++.

ciarmcom · 2019-03-29T16:00:35Z

@kjbracey-arm, thank you for your changes.
@screamerbg @Ronny-Liu @ARMmbed/mbed-os-core @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-test please review.

cmonr · 2019-03-29T16:21:51Z

@kegilbert @kjbracey-arm

platform/Atomic.h
=================================
Errors: 
131: Eg
133: ifdef
_________________________________

Eg? Is this similar to IE/ie?
ifdef should probably not be getting flagged.

platform/Atomic.h

kegilbert · 2019-03-29T17:29:21Z

@AnotherButler for the Eg question.

As for the ifdef, I'd recommend adding @code around the code segment (adding a review for it). If you'd rather not do that, ifdef can be added to the whitelist of ignored words in the spellchecker by adding it here:

https://github.com/ARMmbed/mbed-os/blob/master/tools/test/travis-ci/doxy-spellchecker/ignore.en.pws

AnotherButler · 2019-03-29T17:32:48Z

We don't use eg or ie (or any other Latin phrases) in our technical documentation. It can be confusing to readers.

NirSonnenschein · 2019-03-31T09:39:55Z

@kjbracey-arm , please see travis failures

kjbracey · 2019-04-02T14:41:11Z

Any thoughts on the naming here? To follow general class rules, the top level class is mbed::Atomic<T>, rather than mbed::atomic<T>, but then we have the convenience typedefs mbed::atomic_int. I can't quite bring myself to have mbed::AtomicInt or mbed::Atomic_int. What about atomic_flag?

Like atomic_int, Lots of the functions and types and enum values here are identical to C++, just in namespace mbed instead of std: memory_order_acquire, atomic_init, atomic_flag. Is that a problem? Do we assume people can use namespaces yet?

kjbracey · 2019-07-09T09:52:41Z

Thinking further on the C++ glue - new proposed scheme:

<mstd_atomic>/<mstd_utility> etc headers
They include standard headers to get std from toolchain.
They also provide namespace mstd:
- mstd has aliases to known-working std stuff - eg mstd::nullptr_t == std::nullptr_t.
- mstd has its own implementations where needed - eg mstd::swap for ARM C 5, or mstd::atomic for everyone.
- mstd can also add post-C++14 stuff not present in all toolchains yet.

Thus using mstd instead of std universally should provide good results. mstd will omit stuff known not to universally work across toolchains, eg mstd::thread. Just doing #include <xxx> may work, but you'll be at the mercy of the toolchain. Eg std::swap won't move in ARM C 5. #include <mstd_xxx> gives us the chance to interpose compatibility glue. mstd::swap can always move.

For portable (to non-Mbed OS) code, eg for unit tests or module tests, you can use a simple alias to cover both builds:

#if TARGET_LIKE_MBED && !UNIT_TEST
#include <mstd_atomic>
#else
#include <atomic>
using namespace mstd = std;
#endif

mstd::atomic my_atomic;

kjbracey · 2019-07-09T10:06:35Z

Unit test failure corrected (it was a non-C++14-compatible stub). Rebased.

On the discussion about naming/namespaces, that can follow this PR - it can go in now as the current platform/Atomic.h and mbed::Atomic, but I suspect it will move to join other glue as <mstd_atomic> and mstd::atomic.

artokin · 2019-07-09T10:57:00Z

CI started

mbed-ci · 2019-07-09T12:05:35Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 9
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

0xc0170 · 2019-07-09T14:22:23Z

Please review unittests

Adjust some non-C++11-compatible code that failed as a result.

Even though C/C++11 don't offer pre-op forms (that do the operation then return the new value) of their freestanding functions, we will be making them visible via pre-op operators on `Atomic<T>` (++, --, +=, -=, &=, |=, ^=). It is easier to do a pre-op than a post-op, as we can use one fewer register in the assembler, so it's worth optimising for what will be quite common cases. Make these forms accessible for `Atomic<T>`, but don't document them for standalone use at this stage.

Add noexcept for consistency with upcoming Atomic.h

Avoid template ambiguities using type_identity_t. Previously the compiler would be unable to figure out whether uint8_t x; core_util_atomic_store(&x, 0); should invoke core_util_atomic_store<uint8_t>, matching the pointer type, or core_util_atomic_store<int>, matching the value, leading to an ambiguity error. Templates now select only on the type of the atomic pointer parameter.

Add a C++ `Atomic<T>` template to make atomics even easier. Basically compatible with C++11 `std::atomic<T>`, but using the underlying `core_util_atomic_xxx` functions from mbed_atomic.h, so appropriate for synchronising with interrupts and optimised for uniprocessor. One extra piece of functionality beyond the `core_util_atomic_xxx` functions is the ability to have an arbitrary atomic type - eg a small structure with 2 `uint16_t`s can be stored in a `uint32_t` container.

Make the atomic functional test use `Atomic<T>` rather than the standalone functions - this then tests both layers simultaneously.

kjbracey · 2019-07-09T16:11:21Z

Another C++14 fix made. Can't do

char x[] = { 100, 200 };

if char is signed in C++11 :(

That was actually in real cellular code rather than a stub - shows up when unit testing it on x86 with signed char.

mbed-ci · 2019-07-10T10:27:53Z

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 10
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_build-ARM

mbed-ci · 2019-07-11T13:47:01Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 11
Build artifacts

ciarmcom requested review from Ronny-Liu, screamerbg and a team March 29, 2019 16:00

ciarmcom added the needs: review label Mar 29, 2019

kegilbert reviewed Mar 29, 2019

View reviewed changes

platform/Atomic.h Outdated Show resolved Hide resolved

kegilbert reviewed Mar 29, 2019

View reviewed changes

platform/Atomic.h Show resolved Hide resolved

SeppoTakalo approved these changes Apr 1, 2019

View reviewed changes

kjbracey requested review from pan- and removed request for screamerbg April 1, 2019 13:21

kjbracey force-pushed the atomic_template branch 5 times, most recently from ba11975 to a4b7d9c Compare April 2, 2019 14:17

kjbracey force-pushed the atomic_template branch 5 times, most recently from 5a6d374 to 5bba169 Compare April 4, 2019 12:26

0xc0170 added the needs: work label Jul 9, 2019

kjbracey force-pushed the atomic_template branch from ea2d02a to 4b0244b Compare July 9, 2019 10:04

artokin added needs: CI and removed needs: work labels Jul 9, 2019

0xc0170 approved these changes Jul 9, 2019

View reviewed changes

kjbracey added 7 commits July 9, 2019 19:08

Set unit tests to C++14

607856e

Adjust some non-C++11-compatible code that failed as a result.

mbed_atomic templates: add noexcept

3fd7e11

Add noexcept for consistency with upcoming Atomic.h

Make atomic test use Atomic<T>

e2f65ec

Make the atomic functional test use `Atomic<T>` rather than the standalone functions - this then tests both layers simultaneously.

Test Atomic<T> with user T

c9faf24

kjbracey force-pushed the atomic_template branch from 4b0244b to c9faf24 Compare July 9, 2019 16:09

kjbracey mentioned this pull request Jul 11, 2019

Reduce Event.h and EventQueue.h using C++11 #10895

Merged

kjbracey added ready for merge and removed needs: CI labels Jul 12, 2019

kjbracey merged commit d53bd61 into ARMmbed:master Jul 12, 2019

kjbracey mentioned this pull request Jul 12, 2019

Add <mstd_xxx> C++ headers #11039

Merged

artokin added release-version: 5.14.0-rc1 and removed ready for merge labels Jul 15, 2019

This was referenced Aug 13, 2019

M2351: Compile failed on GCC #11102

Closed

Atomics: GCC fix for M23 (again) #11208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Atomic<T> template #10274

Add Atomic<T> template #10274

kjbracey commented Mar 29, 2019 •

edited

Loading

ciarmcom commented Mar 29, 2019

cmonr commented Mar 29, 2019

kegilbert commented Mar 29, 2019

AnotherButler commented Mar 29, 2019

NirSonnenschein commented Mar 31, 2019

kjbracey commented Apr 2, 2019

kjbracey commented Jul 9, 2019 •

edited

Loading

kjbracey commented Jul 9, 2019

artokin commented Jul 9, 2019

mbed-ci commented Jul 9, 2019

0xc0170 commented Jul 9, 2019

kjbracey commented Jul 9, 2019

mbed-ci commented Jul 10, 2019

mbed-ci commented Jul 11, 2019

Add Atomic<T> template #10274

Add Atomic<T> template #10274

Conversation

kjbracey commented Mar 29, 2019 • edited Loading

Description

Pull request type

Release Notes

ciarmcom commented Mar 29, 2019

cmonr commented Mar 29, 2019

kegilbert commented Mar 29, 2019

AnotherButler commented Mar 29, 2019

NirSonnenschein commented Mar 31, 2019

kjbracey commented Apr 2, 2019

kjbracey commented Jul 9, 2019 • edited Loading

kjbracey commented Jul 9, 2019

artokin commented Jul 9, 2019

mbed-ci commented Jul 9, 2019

Test run: FAILED

0xc0170 commented Jul 9, 2019

kjbracey commented Jul 9, 2019

mbed-ci commented Jul 10, 2019

Test run: FAILED

mbed-ci commented Jul 11, 2019

Test run: SUCCESS

kjbracey commented Mar 29, 2019 •

edited

Loading

kjbracey commented Jul 9, 2019 •

edited

Loading