Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add <mstd_xxx> C++ headers #11039

Merged
merged 11 commits into from Jul 19, 2019

Conversation

@kjbracey-arm
Copy link
Contributor

commented Jul 12, 2019

Description

Restructure mbed_cxxsupport.h from #10868, to make it mirror the standard C library.

  • <mstd_xxxx> is equivalent to C++14 or later <xxxx>
  • Gets base toolchain header in to populate namespace std - some local implementation for ARM C 5.
  • Imports all relevant stuff into namespace mstd - alternative implementations supplied where we want to bypass toolchain deficiencies. Eg mstd::mutex maps to rtos::Mutex.
  • namespace mstd can also contain post-C++14 extensions not necessarily present in all toolchains.

Some C++11 library extensions and fixes, notably invoke, <mstd_mutex>.

mbed::Atomic in platform/Atomic.h moved to mstd::atomic in <mstd_atomic> to fit the general pattern.

Using separate namespace and filename gives us the ability to work around toolchain differences. In portable code, the following model could be used, assuming the choice between mbed OS and a fully C++11-compliant system.

#ifdef TARGET_LIKE_MBED
#include <mstd_atomic>
#include <mstd_mutex>
#else
#include <atomic>
#include <mutex>
using namespace mstd = std;
#endif

using mstd::mutex;
 
mutex m;
mstd::atomic_int i;

This is a breaking change with respect to #10868 and #10274 now on master (reworking Atomic.h and mbed_cxxsupport.h), but they're new since 5.13. I'd like this change to go in before 5.14, so there's no visible released API breakage.

Pull request type

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

Reviewers

@pan-, @bulislaw

Release Notes

  • A set of C++11/14 library support headers have been included in platform/cxxsupport. These include standard-C++-like APIs, eg mstd::atomic in <mstd_atomic>, to improve on implementations provided with toolchains. See platform/cxxsupport/README.md for more information.
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:mstd branch from b0bfc60 to 1e6c4e1 Jul 12, 2019
@ciarmcom ciarmcom requested review from bulislaw, pan- and ARMmbed/mbed-os-maintainers Jul 12, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:mstd branch 2 times, most recently from 75b62ef to 3a48668 Jul 15, 2019
Copy link
Member

left a comment

Looks good, should be accompanied by good handbook docs to explain how and why to use it.

* A pointer to the singleton, or NULL if not
* initialized.
*/
T *get_no_init() const

This comment has been minimized.

Copy link
@bulislaw

bulislaw Jul 15, 2019

Member

Would it be worth to add an assert here?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 16, 2019

Author Contributor

I think not here - I think returning NULL if it's not initialised is potentially useful API. You might want to perform some operation if it's active, but not start it.

Callers could maybe assert, but I don't think asserting ahead of a NULL dereference is that useful either - it will go bang with a pretty easy to debug backtrace.

@@ -160,9 +160,9 @@ def version_check(self):

def _get_toolchain_labels(self):
if getattr(self.target, "default_toolchain", "ARM") == "uARM":
return ["ARM", "ARM_MICRO"]
return ["ARM", "ARM_MICRO", "ARMC5"]

This comment has been minimized.

Copy link
@bulislaw

bulislaw Jul 15, 2019

Member

What does it do?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 16, 2019

Author Contributor

Means we can optionally include stuff via a TOOLCHAIN_ARMC5 folder. At the minute we only have the labels TOOLCHAIN_ARM and TOOLCHAIN_ARMC6. Used for the base <array>, <initializer_list> bits for ARM C 5.

(We only need to tiptoe around with namespace mstd and <mstd_xxx> for places where we are up against an existing header file; when we're supplying something ARM C 5 lacks altogether, we can just go straight in with the standard name in namespace std.)

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 16, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-ARM
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:mstd branch 2 times, most recently from a588881 to ed7909e Jul 16, 2019
@kjbracey-arm kjbracey-arm referenced this pull request Jul 17, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Jul 17, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:mstd branch from ed7909e to 60e71d1 Jul 17, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Jul 17, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:mstd branch from 60e71d1 to 3e739be Jul 17, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Jul 17, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 4
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

This comment has been minimized.

Copy link

commented Jul 17, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
kjbracey-arm added 4 commits Jul 1, 2019
For all compilers add post-C++14 stuff to namespace mbed:

* invoke, invoke_result, is_invocable, is_invocable_r,
  is_nothrow_invocable, is_nothrow_invocable_r, unwrap_reference,
  unwrap_ref_decay

For ARM C 5, add C++11 bits based on above to namespace std:

* result_of, reference_wrapper, ref, cref, mem_fn
We have some files that are needed for ARMC5 only.
As something of a cheat, exclude platform/cxxsupport from astyle.

astyle really can't process the sort of template metaprogramming going
on in there; treat it as a "black box" like the toolchains' own
libraries.
Treat files with no extension as headers, if they are in the cxxsupport
folder.
@mbed-ci

This comment has been minimized.

Copy link

commented Jul 18, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:mstd branch from 3e739be to 3e71da3 Jul 18, 2019
Regularise the C++ support glue, adding `<mstd_type_traits>` etc.

These include the base toolchain file, backfill `namespace std` as much
as possible for ARM C 5, and then arrange to create unified
`namespace mstd`, which can incorporate toolchain bypasses
(eg `mstd::swap` for ARM C 5, or `mstd::atomic`), and include local
implementations of post-ARM C++14 stuff.

All APIs in `namespace mstd` are intended to function as their
`namespace std` equivalent, and their presence there indicates they
are functional on all toolchains, and should be safe to use in
an Mbed OS build (including not unreasonable memory footprint).

See README.md for more info.
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:mstd branch 2 times, most recently from 13b8f67 to 1bc9b70 Jul 18, 2019
* Adjust definition to make the default constructor `constexpr`.
  This permits use in classes that want lazy initialization and their
  own `constexpr` constructor, such as `mstd::mutex`.

* Add `get_no_init()` method to allow an explicit optimisation for
  paths that know they won be the first call (such as
  `mstd::mutex::unlock`).

* Add `destroy()` method to permit destruction of the contained object.
  (`SingletonPtr`'s destructor does not call its destructor - a cheat
  to omit destructors of static objects). Needed if using in a class
  that needs proper destruction.
Add add-standard-as-possible version of C++11 <mutex>.

A lot of the stuff in there is generic, but the actual mutex classes and
call_once need to interface with the OS.

For those, they're not available in ARMC5 or IAR; retargetting would be
necessary for ARMC6 and GCC, and I've yet to investigate how whether
that's possible. So for now I'm using local implementations.

Although `Mutex` in principle could support `timed_mutex` and
`recursive_timed_mutex`, we don't have `chrono` for the time parameters,
so hold off for now.

For the generic stuff like mstd::unique_lock, they are aliased to
std::unique_lock where possible.
`mbed::Atomic<T>` becomes `mstd::atomic<T>`, alongside the other
standard C++ library lookalikes.
Standard library implementation should be fine for other toolchains.
Stripped down headers that route through to `std` forms more - eg
no local implementation of `atomic` or `mutex`.
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:mstd branch from 1bc9b70 to 852a626 Jul 18, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Jul 18, 2019

Test run: SUCCESS

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

@SeppoTakalo SeppoTakalo merged commit 7778692 into ARMmbed:master Jul 19, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+36 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8629 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Copy link
Member

left a comment

Awesome work; I skimmed through files (it still took me few hours) and this looks solid. Some tests would help for future maintenance.

@@ -28,6 +30,8 @@ using utest::v1::Case;

namespace {

using mstd::atomic;

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

mbed.h is included above, so there's using namespace std and using mstd:atomic that both brings atomic in the global namespace.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 19, 2019

Author Contributor

This is actually pulling it into this unnamed namespace.

I previously claimed that a global-scope using mstd::atomic would override a more general using namespace std for atomic, and I'm sure I'd tested this, but seemed not to work here.

But putting it into an inner namespace like this does work - it searches innermost namespace first.

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

I somehow missed the anonymous namespace declaration. Good to know.

*
* - provides <algorithm>
* - For ARM C 5, standard C++11/14 features:
* - std::min, std::max for std::initializer_list

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

Why not have <mstd_initializer_list> for initialiser list ?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 19, 2019

Author Contributor

Good general question - for consistency, it would indeed make sense to have mstd_xxx for all headers we think are good, to "approve" them, and to allow a single using namespace mstd, even if we currently don't need any adaptation, and it would just copy everything over. If you want to fill them in, I wouldn't object, at least for "not-too-bloaty-for-embedded" stuff.

* - std::find_if_not
* - std::equal (2-range forms)
* - std::copy_n, std::move, std::move_backward
* - mstd::min, mstd::max constexpr replacements

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

How do we get constexpr replacement if std::min and std::max are directly imported ?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 19, 2019

Author Contributor

As mstd::min and mstd::max. For ARM C 5 those are these replacement implementations, rather than being aliased to std::min like for the other toolchains.

#define MSTD_CONSTEXPR_OBJ_14 constexpr
#else
#define MSTD_CONSTEXPR_FN_14 inline
#define MSTD_CONSTEXPR_OBJ_14 const

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

I'm not sure to agree with that definition; in C++14 a constexpr object can invoke non const member function in a constexpr context. In C++11; constexpr equals const.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 19, 2019

Author Contributor

constexpr implies const method in a C++11, yes, but I don't think that's an issue here. That would be used as:

MSTD_CONSTEXPR_FN_14 int my_complicated_method() /* const or not */ {
      // some-multi-line thing
     return whatever;
}

Flipping from constexpr in C++14 to inline in C++11 seems right to me - either member or non-member.

Or were you misunderstanding OBJ - that was intended for a constexpr object:

static MSTD_CONSTEXPR_OBJ_14 int npos = -1;

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

I was thinking of the following case:

struct Foo { 
    constexpr Foo() { }

    // not a const member function!
    MSTD_CONSTEXPR_FN_14 int get_something() { /* ... */ }
};

static MSTD_CONSTEXPR_OBJ_14 Foo foo;

void foo() { 
   // error call of a non const function on a const object
   auto v = foo.get_something();
}

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 19, 2019

Author Contributor

Seems fine to me? In C++14, the function is constexpr, not const, and it fails to compile.

In C++11, the function is inline, not const, and it fails to compile.

User code should normally be able to include C++14 headers and get
most expected functionality. (But bear in mind that many C++ library
features like streams and STL containers are quite heavy and may
not be appropriate for small embnedded use).

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

embnedded => embedded

#define MSTD_CONSTEXPR_OBJ_14 constexpr
#else
#define MSTD_CONSTEXPR_FN_14 inline
#define MSTD_CONSTEXPR_OBJ_14 const

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

See comment above.

// [refwrap]
template <typename T>
class reference_wrapper {
T &FUN(T &x) noexcept { return x; }

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

Question, why use FUN as identifier ? __fun or _Fun where not good enough ?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 12, 2019

Author Contributor

It came from the revised-in-2017 standard text (https://cplusplus.github.io/LWG/issue2993). We're not actually using it anyway, as that version seems to be too advanced for ARM C 5. I'll remove the dead code.

}

template <typename F>
impl::not_fn_t<F> not_fn_t(F&& f)

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

Do you mean not_fn as the function name ?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 19, 2019

Author Contributor

Spot the person rushing to get this in before holidays. (To ensure we don't have a breaking change versus a shipped mbed_cxxsupport.h)

To some extent this is justified for the bulk of the code being for ARMC5 only, and it's not-officially-supported status, but yes, testing clearly needed.

template<
class T,
class D = default_delete<T>
> class unique_ptr : public impl::deleter_store<D>

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

Glad to see that class getting in.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 19, 2019

Author Contributor

Yes, but sadly in use, it increases code size, at least in my tests.

You'd think it was "free", but the compiler doesn't seem to be as good at optimising out shuffling. For example, given:

 if (!ptr) {
     ptr = make_unique<Thing>(whatever);
 }

I've seen ARMC5 at least put in a pointless delete-of-current-pointer-contents on the assignment (but we just tested that the pointer is currently empty), and then a pointless delete of the temporary returned by make_unique (we know we just moved out of it).

Avoiding make_unique helped a bit, but still not as good as doing it by hand.

Maybe other compilers (and their std::unique_ptr) do better?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 19, 2019

Author Contributor

Much as I want to believe the C++ guys' claims that C++ is as efficient as C, I keep seeing cases like this where it's just a bit too hard for the compiler to totally eliminate the generic stuff, and it does get bigger. At this embedded scale of tens of kilobytes of ROM and RAM, the impact is real...

Lots of little micro things you wouldn't think of.

Plus the annoyances like what I hit with SingletonPtr here - took me a number of hours to understand the initialization rules enough to comprehend the "mixed zero and constant initialization" problem I was hitting and deal with it to get them back out of the final link.

Was watching this while debugging that one: https://www.youtube.com/watch?v=D7Sd8A6_fYU

Made me a little grumpy - yes, there are misconceptions about C++ for embedded use, but it does also have real issues. And yes, sure, stronger type checking can move more debugging to compilation time, but often that debugging is much harder...

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

It would be sweet if our documentations were explaining which C++ features is safe to use and what are the impacts on code size/speed (I'd love to write that). I hope most of the code won't turn into crazy template so debug ability should be okay; too bad that compilers doesn't have attributes to treat constructors as literals. It would be really useful for basic building blocks such as Callback.

Much as I want to believe the C++ guys' claims that C++ is as efficient as C, I keep seeing cases like this where it's just a bit too hard for the compiler to totally eliminate the generic stuff, and it does get bigger. At this embedded scale of tens of kilobytes of ROM and RAM, the impact is real...

I'd be interested to measure the impact on a real world application. Not that I doubt there is one (there will be one as long as there's no destructive moves) but an accurate picture would help us create guidelines to our community of users and internal developers.

};

// [thread.mutex.recursive]
class recursive_mutex : public _Mutex_base {

This comment has been minimized.

Copy link
@pan-

pan- Jul 19, 2019

Member

any plan to add the timed_ versions ?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 19, 2019

Author Contributor

We need chrono for ARMC5 first. That feels like a separate project.

If licensing permitted just lifting the ARMC6 implementation...?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 19, 2019

Author Contributor

This is one case where ARMC5 is a significant blocker. It would be easy to bring chrono in to use for the other 3 toolchains, but any attempt to keep ARMC5 building blocks it. Could make all relevant APIs conditional on ifndef __CC_ARM, but don't fancy that either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.