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

GCC 9 support #2103

Merged
merged 14 commits into from
May 15, 2020
Merged

GCC 9 support #2103

merged 14 commits into from
May 15, 2020

Conversation

avtolstoy
Copy link
Member

@avtolstoy avtolstoy commented May 15, 2020

Description

Resolves various issues/warnings encountered when building with GCC9, switches over the CI to using GCC9.

Updates Workbench manifest with GCC 9 support as well.

Steps to Test

N/A

Example App

N/A

References

  • [CH42647]

Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

// Jump to system firmware
uint32_t addr = *(volatile uint32_t*)(ApplicationAddress + 4);
uint32_t stack = *(volatile uint32_t*)ApplicationAddress;
jump_to_system(addr, stack);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference: this is required because GCC9 now throws a deprecated error when calling CMSIS __set_MSP(), which clobbers sp, which is in fact not what is supposed to happen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quote from recent gcc docs:

Another restriction is that the clobber list should not contain the stack pointer register. This is because the compiler requires the value of the stack pointer to be the same after an asm statement as it was on entry to the statement. However, previous versions of GCC did not enforce this rule and allowed the stack pointer to appear in the list, with unclear semantics. This behavior is deprecated and listing the stack pointer may become an error in future versions of GCC.

@avtolstoy avtolstoy requested a review from sergeuz May 15, 2020 10:58
extern char link_interrupt_vectors_location;
extern char link_ram_interrupt_vectors_location;
extern char link_ram_interrupt_vectors_location_end;
extern uintptr_t link_interrupt_vectors_location[];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference: this is now required because of array bounds checks warnings in GCC9.

@@ -830,9 +830,3 @@ String String::format(const char* fmt, ...)
}
return result;
}

std::ostream& operator << ( std::ostream& os, const String& value ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference: this was a source of a major binary size bloat due to the usage of iostreams, which was bringing std strings, locales, exceptions and system_error stuff into the binary no matter what.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change that we ought to announce? What would be typical usage? Something like Serial << String("foo") ? Once that's clear I can check with Studios if they use this pattern in any application.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a breaking change. DeviceOS does not support iostreams in any form. I have no explanation why that code was there if only for unit tests. For the example you listed to work Serial would have to be a subclass of std::ostream, which it isn't and none of the other wiring classes are.

@@ -127,12 +127,20 @@ size_t Print::print(T n, int base)
size_t t = 0;
using PrintNumberType = typename PrintNumberTypeSelector<T>::type;
PrintNumberType val;
// FIXME: avoids 'comparison of constant '0' with boolean expression is always false'
#if __GNUC__ >= 9
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a separate bool specialization brings its own set of problems, so for now we are disabling this warning here specifically as a workaround.

@avtolstoy avtolstoy force-pushed the enhancement/bump_gcc_version/ch42647 branch from d226118 to 2c9237e Compare May 15, 2020 13:42
@avtolstoy avtolstoy changed the base branch from feature/mesh-deprecation to develop May 15, 2020 13:43
@avtolstoy avtolstoy requested review from DebbieGillespie and eberseth and removed request for DebbieGillespie May 15, 2020 14:39
@avtolstoy avtolstoy requested a review from monkbroc May 15, 2020 15:14
Copy link
Member

@monkbroc monkbroc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I verified the compilation on Workbench on Linux for all platforms.

@@ -830,9 +830,3 @@ String String::format(const char* fmt, ...)
}
return result;
}

std::ostream& operator << ( std::ostream& os, const String& value ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change that we ought to announce? What would be typical usage? Something like Serial << String("foo") ? Once that's clear I can check with Studios if they use this pattern in any application.

@avtolstoy avtolstoy added ready to merge PR has been reviewed and tested and removed needs review labels May 15, 2020
@avtolstoy avtolstoy merged commit 09439c9 into develop May 15, 2020
@avtolstoy avtolstoy deleted the enhancement/bump_gcc_version/ch42647 branch May 15, 2020 18:11
@elcojacobs
Copy link
Contributor

elcojacobs commented Aug 27, 2020

I'm very happy that the compiler has finally been upgraded!
Just compiled against 2.0.0-rc.1 and apart from the simple api deprecations changes, there seem to be no issues for my huge application.

Now that we run on gcc 9, can the makefiles for a local build be upgraded to use C++17 too? The still use std=gnu++14?

Changing the standard in lang-std.mk will give many warnings in STM32 includes about the deprecated register specifier:
ISO C++17 does not allow 'register' storage class specifier.

@avtolstoy
Copy link
Member Author

We aren't changing the C++ standard version for 2.x. We'll migrate most likely to C++20 in 3.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ready to merge PR has been reviewed and tested
Projects
None yet
5 participants