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

Set compilers to C++14 and C11 #10427

Merged
merged 5 commits into from May 28, 2019

Conversation

@kjbracey-arm
Copy link
Contributor

commented Apr 17, 2019

Description

  • ARMC6 and GCC are set to C++14 and C11.
  • ARMC5 is set to C++11 and C99, as it's the highest it can offer.

Pull request type

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

Release Notes

  • GCC and ARM toolchain profiles now select C++14 and C11, matching IAR, and these are the tested profiles. Codebase in this release should still work if profiles are set to C++98 and C99, although this is no longer tested.

Migration notes

As the default profiles now select C++14 and C11 for GCC and ARM toolchains, some applications may fail to compile because they use constructs that were valid in C++98, but not in C++14.

IAR 8 has always operated in C++14/C11 mode, so there are no new changes for IAR users who switched to IAR 8 for Mbed OS 5.12, but these notes apply for users migrating from older Mbed OS versions that used IAR 7.

Common compatibility issues

  • A space is required when a macro follows a string literal, for example in C99-style printf formats
    uint32_t val;
    printf("val = %"PRIu32, val); // Not valid in C++11
    printf("val = %" PRIu32, val); // OK

Without the space, C++11 interprets it as being a request for a user-defined "PRIu32-type" string literal.

  • Initializer lists cannot have implicit narrowing conversions:
    uint32_t x;
    uint8_t array1[] = { x }; // Not valid in C++11
    uint8_t array2[] = { 0xffff }; // Not valid in C++11
    uint8_t array3[] = { x & 0xff }; // Not valid in C++11
    uint8_t array4[] = { (uint8_t) x }; // OK
    uint8_t array5[] = { static_cast<uint8_t>(x) }; // OK
    uint8_t array6[] = { 0xffff & 0xff }; // OK (because it's a compile-time constant that fits)

These changes should be easy to make to existing codebases. A guide to other possible breakages in C++11 can be found at https://stackoverflow.com/questions/6399615/what-breaking-changes-are-introduced-in-c11. C++14 and C11 cause few extra issues.

Future compatibility issues

  • The register keyword is deprecated in C++14, and is removed in C++17. Some compilers issue warnings for register use in C++14, but this has been temporarily suppressed due to the prevalence of the keyword in target code. C++ code should be updated to remove the keyword as soon as possible - the warning will be reactivated once Mbed OS itself no longer triggers it.

Fallback

Mbed OS 5.13 releases should still work if compiled as C++98/C99, although this is not tested - if you have serious application compatibility issues, you should be able to switch the build profile back to the earlier language version as a temporary measure. This is likely to no longer be the case for Mbed OS 5.14.

ARM Compiler 5

Note also that ARM Compiler 5 has limited C++11 support and no C++14 or C11 support, and ARM Compiler 5 is no longer tested or officially supported. Nevertheless, ARMC5 builds have not been deliberately broken; ARMC5 build profiles select its C++11 mode in an attempt to match the other toolchains as much as possible, but the limited support may itself cause issues - attempts to check __cplusplus >= 201103 may activate code that ARMC5 cannot actually compile. Like the other toolchains, the profile can be switched back to C++98 if necessary for now.

Continued ARM C 5 support will limit the adoption of C++11 and C++14 features in the Mbed OS codebase, so it is quite possible that ARM Compiler 5 builds may stop working altogether in an upcoming release.

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Apr 17, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

@kjbracey-arm, thank you for your changes.
@ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

@0xc0170 0xc0170 requested review from ARMmbed/mbed-os-core and bulislaw Apr 17, 2019

@bridadan
Copy link
Contributor

left a comment

Very very exciting!!!

Do you know what this does to code size?

@@ -183,7 +183,7 @@ def format_flags(self):
flags['c_flags'] + flags['cxx_flags'] + flags['common_flags']
)
in_template = set(
["--no_vla", "--cpp", "--c99", "-MMD"] + config_option
["--no_vla", "--cpp", "--cpp11", "--c99", "-MMD"] + config_option

This comment has been minimized.

Copy link
@bridadan

bridadan Apr 17, 2019

Contributor

Are the --cpp and --cpp11 flags exclusive in this case?

Suggested change
["--no_vla", "--cpp", "--cpp11", "--c99", "-MMD"] + config_option
["--no_vla", "--cpp11", "--c99", "-MMD"] + config_option

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 17, 2019

Author Contributor

My reading of that was that that was just a list of options that would be eligible for passthrough in the code below. Can you check?

In which case both --cpp and --cpp11 want this behaviour, in case anyone wants to switch profile back to old version.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 17, 2019

Author Contributor

that was that that was

PS Don't report me to tech authors.

This comment has been minimized.

Copy link
@bridadan

bridadan Apr 17, 2019

Contributor

Oh gosh you're totally right, good eye! In my defense, I have not finished my first cup of coffee this morning 😄

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Do you know what this does to code size?

Nothing on its own, hopefully. Possibly might end up activating a few more printf formatters?

Once it's on and we start writing C++14-requiring code there will be language constructs like final, move semantics and constexpr we can use that could reduce code size.

@bridadan

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Nothing on its own, hopefully

Understood, though I think it'd be worth it to do a basic comparison with blinky or something as a sanity check.

@bridadan
Copy link
Contributor

left a comment

Very nice work!

@bridadan

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Note to self: when this goes in and we make a corresponding tools release, we'll need to update the online compiler.

@@ -6,7 +6,7 @@
<Compile>
<Option name="OptimizationLevel" value="4"/>
<Option name="UseFPU" value="0"/>
<Option name="UserEditCompiler" value="-fno-common; -fmessage-length=0; -Wall; -fno-strict-aliasing; -fno-rtti; -fno-exceptions; -ffunction-sections; -fdata-sections; -std=gnu++98"/>
<Option name="UserEditCompiler" value="-fno-common; -fmessage-length=0; -Wall; -fno-strict-aliasing; -fno-rtti; -fno-exceptions; -ffunction-sections; -fdata-sections; -std=gnu++14"/>

This comment has been minimized.

Copy link
@deepikabhavnani

deepikabhavnani Apr 17, 2019

Contributor

Question: How do we differentiate Armc5 / Armc6 profiles in exporters?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 17, 2019

Author Contributor

I don't know, but not sure I understand the question. This template must be for GCC or ARMC6, as -std is an option only for them.

There may be further exporter changes required for toolchains where the standard is specified via an XML option, which may not be visible on my search.

This comment has been minimized.

Copy link
@deepikabhavnani

deepikabhavnani Apr 17, 2019

Contributor

My bad, I thought this was default template for uVision didn;t see coide

This comment has been minimized.

Copy link
@bridadan

bridadan Apr 17, 2019

Contributor

Deepika's right though, we'll need to update the uvision template a bit more to play nicely with the C++ and C version. The language versions are selected by the IDE GUI.

@bulislaw
Copy link
Member

left a comment

Approved, if it doesn't inflate the size (please check couple of examples manually, as I can't see the code size checks run in the PR). Also that should go for 5.13 not a patch release.

@bridadan

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

These two lines control the language versions for uvision: https://github.com/ARMmbed/mbed-os/blob/master/tools/export/uvision/uvision.tmpl#L372-L373

The corresponding diff to go from gnu99 and gnu++98 to gnu11 and gnu++14 is as follows:

-            <v6Lang>4</v6Lang>
-            <v6LangP>2</v6LangP>
+            <v6Lang>6</v6Lang>
+            <v6LangP>6</v6LangP>

We'd need to make this conditional based on the --cpp flags.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

ARMC6 blinky showed ROM -4, RAM +8 - not sure whether there was anything real there or just padding shifting.

GCC blinky shows ROM +8.

ROM increase in GCC is coming from passing object sizes to delete operator in destructors, eg:

a:   f44f 7192       mov.w   r1, #292        ; 0x124
e:   f7ff fffe       bl      0 <_ZdlPvj>

Instead of:

a:   f7ff fffe       bl      0 <_ZdlPv>

New operator delete overloads passing object size are a C++14 extension, so there's a language cost.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

We'd need to make this conditional based on the --cpp flags.

Does it need to be conditional?

How much do we want to support retro-version language profiles? To the extent of "they'll probably work if you mess with the settings manually" or "we'll support old versions by making sure exporters and online compiler let you select old versions smoothly"?

Old language versions will be working for basically one release (5.13?) - the one where we haven't had time to start using the new language features, so not sure it's worth having too much polish.

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Does it need to be conditional?

Conditional or not does not really matter, (fine with any approach duplicate template file or python script updating the correct values) we just need to make sure the correct flags are set for uVision5 and uVision6 exporters

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

So you mean conditional on "ARMC5 or ARMC6", got it. I was reading that as conditional on "what language standard the JSON profile said", which seemed overkill.

Well, in that uVision case, that v6Lang option is presumably for ARMC6 specifically, so it could just always be set to the c++14 value.

But then where does the setting for ARMC5 C++11 go? Does it have an XML tag, or does it have to go in as a misc option?

@40Grit

This comment has been minimized.

Copy link

commented Apr 18, 2019

@team-embeddedPlanet

@farrenv, @trowbridgec, @AGlass0fMilk

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

uVision exporter may be an issue - it doesn't have an ARMC5 option for C++11, and doesn't have an ARMC6 option for "gnu++14".

Can't just put them into the misc options, as the misc options go to both C and C++, and it gets narked.

Exporter may have to lag behind until IDEs catch up.

<v6Lang>4</v6Lang>
<v6LangP>2</v6LangP>
<v6Lang>6</v6Lang>
<v6LangP>4</v6LangP>

This comment has been minimized.

Copy link
@bridadan

bridadan Apr 18, 2019

Contributor

The uvision template change has to be conditional because the online compiler needs to be able to export older projects that cannot use the newer language standards. The online compiler will use the latest copy of the tools but switch the profiles accordingly so the correct language version is used.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 25, 2019

Author Contributor

Reworked to parse the -std option. And I realised that we can actually work around uVision not knowing about gnu++14 - it's armclang's default and we can ask for <default>.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 25, 2019

Author Contributor

Except that workaround doesn't seem to work. Contrary to uVision documentation, if you put <default> in, it explicitly selects -std=gnu11 for C and -std=gnu++89 for C++.

(Those were the defaults for Compiler 6.9, I guess, but the documentation clearly says that it will use the compiler default by not passing an option).

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:new_standards branch 2 times, most recently from f08ddae to 5538e73 Apr 25, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

What's the situation with other exporters, w.r.t. being adaptive for old Mbed OS profiles?

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@bridadan Could you please re-review ?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

The earlier commit takes the approach of extracting the -std options and mapping to the GUI option.

This new commit redoes that and actually passes the -std option through as-is under "misc controls". This means separating out the C and C++ flags, putting the C flags at the top target level, and sticking the C++ flags on each C++ file individually.

Does this seem reasonable? If it works, it does avoid the GUI/IDE limitations.

@bridadan

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@kjbracey-arm Interesting strategy. However, what does that mean for the GUI option? Is it ignored/overridden? In general, I've been under the impression that people export to uVision specifically because they want to use the GUI options vs modifying command line flags.

@bridadan

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@kjbracey-arm if we don't have an immediate path forward on the uvision exporter it could always be revisited in a follow up PR

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:new_standards branch from 610b9f7 to 01e2cdd Apr 26, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Latest merge of MbedTester for FPGA caused a build error

kjbracey-arm added some commits Apr 17, 2019

Set compilers to C++14 and C11
* ARMC6 and GCC are set to C++14 and C11.
* ARMC5 is set to C++11 and C99, as it's the highest it can offer.
µVision export: Add ARMC6 `-std` handling
This is limited to ARMC6 because as of µVision V5.27 you can't set C++11
for ARMC5.

Also current µVision does not support gnu++14. We should be able to get
is as `<default>`, as it is the default for ARM Compiler 6.10-6.12,
but this option does not work as documented and actually requests
gnu++89 explicitly. So gnu++14 is mapped to gnu++11.
ARMC6: disable "reserved user-defined literal" warning
Clang warns about reserved user-defined literals by default. This
warning is not terribly helpful; compilers aren't normally in the
habit of warning about use of reserved identifiers. It can interfere
with, for example, deliberate emulation of a future standard
language feature.

The warning was promoted to an error in an mbed client build, due to a
non-C++11 "%s"name occurring in a macro. But the macro itself was never
invoked, so the misinterpretation as C++11 caused no problems other than
this warning. Killing the warning will let that code build on ARMC6.
The code already built on GCC and IAR.

If that macro ever was used, then a separate error about operator ""
name not being defined would be generated, on all 3 toolchains.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:new_standards branch from 613656b to b437d70 May 24, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 24, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented May 24, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 added needs: review and removed needs: CI labels May 26, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 26, 2019

Tests passed, needs finalizing reviews!

@0xc0170 0xc0170 requested review from bridadan and mark-edgeworth and removed request for bridadan May 27, 2019

@0xc0170 0xc0170 dismissed stale reviews from bridadan and mark-edgeworth May 27, 2019

Fixed

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 27, 2019

This should be now ready for integration

@0xc0170 0xc0170 merged commit cc49181 into ARMmbed:master May 28, 2019

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 Success
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 8957 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
@LMESTM

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@kjbracey-arm
Since merge of this PR we've got loads of warnings like below:

[DEBUG] Output: ./targets/TARGET_STM/TARGET_STM32L4/device\stm32l4xx_ll_usart.h:1959:3: warning: 'register' storage class specifier is deprecated and incompatible with C++17 [-Wdeprecated-register]
[DEBUG] Output: register uint32_t usartdiv = 0x0U;
[DEBUG] Output: ^~~~~~~~~
[DEBUG] Output: ./targets/TARGET_STM/TARGET_STM32L4/device\stm32l4xx_ll_usart.h:1960:3: warning: 'register' storage class specifier is deprecated and incompatible with C++17 [-Wdeprecated-register]
[DEBUG] Output: register uint32_t brrtemp = 0x0U;

How shall we cope with this ?
Any guidance would be welcome considering this was a bit unexpected ...

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

You can ignore it for now, as we're not actually compiling C++17. But the register keyword has been deprecated since C++11.

The fix is just to remove the register keyword - afaik it does nothing on its own in current compilers. If you believe there are old compilers where it does do something, so you want to retain it for them, you could have a compatibility macro in your code base.

The only case it does do something I'm aware of is the "named register" construct in the ARM Compiler (5 and 6): register int foo __asm("r2"). I would assume that you wouldn't get a warning from ARMC doing that, but haven't looked into it.

I guess we could add -Wno-deprecated-register to the profiles. But if it's not fixed now, it will bite you at some point in a C++17 update, so I think the warning is reasonable.

@kjbracey-arm kjbracey-arm deleted the kjbracey-arm:new_standards branch May 29, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

Thanks @kjbracey-arm for adding migration section 👍

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