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: 'register' storage class specifier is deprecated and incompatible with C++17 #12232

Closed
JojoS62 opened this issue Jan 9, 2020 · 19 comments
Closed

Comments

@JojoS62
Copy link
Contributor

JojoS62 commented Jan 9, 2020

Description of defect

Toolchain(s) (name and version) displaying this defect ?

gcc is complaining about using the register storage class as it is deprecated with c++11.
This issue has been reported for ARMC6 and been fixed there by suppressing the warning in
#10717
The same fix can be applied to the settings for gcc by using '-Wno-register', then it will compile also with -gnu++2a without warnings.

What version of Mbed-os are you using (tag or sha) ?

mbed-os-5.15.0

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

gcc 9.x

How is this defect reproduced ?

compile with custom profile and -std=g++17 or -std-g++2a enabled.

@0Grit
Copy link

0Grit commented Jan 9, 2020

@kjbracey-arm

@ciarmcom
Copy link
Member

ciarmcom commented Jan 9, 2020

Internal Jira reference: https://jira.arm.com/browse/MBOTRIAGE-2501

@kjbracey
Copy link
Contributor

Feel free to contribute a patch doing this - I'd have no objection.

Although we should be cleaning up the codebase. It's deprecated in C++14 and not actually in C++17 at all, so GCC would just be supporting it as an extension in that mode.

@jeromecoutant
Copy link
Collaborator

Hi

I agree I am very late to align ST FW driver in mbed with ST cube delivery
https://github.com/STMicroelectronics/STM32CubeF4

Work is on going.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 10, 2020

@JojoS62 Can you specify which files are generating this ? To know who can help fixing it in case there are 3rd party drivers involved (as @jeromecoutant mentioned ST cube).

@kjbracey
Copy link
Contributor

It will actually be almost exclusively 3rd party HAL code.

@JojoS62
Copy link
Contributor Author

JojoS62 commented Jan 10, 2020

yes, turning off the warning is just curing the symptoms, not really fixing the original problem.
in more recent CMSIS than in Mbed, this is fixed also in 2018 already:
ARM-software/CMSIS_5#345
ARM-software/CMSIS_5@f46ec37

I was told the deprication for 'register' was announced in 2009 already and is effective with c++11. But gcc or other compiler started to complain with c++17 setting.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 10, 2020

@JojoS62 We updated files in 2019 (the references are from 2018). I believe this should be fixed in CMSIS files in our code base. Please specify files, at least one we can have a look at entire module

@kjbracey
Copy link
Contributor

kjbracey commented Jan 10, 2020

I was told the deprication for 'register' was announced in 2009 already and is effective with c++11. But gcc or other compiler started to complain with c++17 setting.

You're right - I thought it was deprecated in C++14, but it was deprecated in C++11. It's removed in C++17, but it doesn't stop the compilers still accepting it - they're just required to start complaining because it's not valid C++17.

@JojoS62
Copy link
Contributor Author

JojoS62 commented Jan 10, 2020

@JojoS62 Can you specify which files are generating this ?

f.e. https://github.com/STMicroelectronics/STM32CubeF4/blob/b5abca20c9676b04f8d2885a668a9b653ee65705/Drivers/STM32F4xx_HAL_Driver/Inc/stm32f4xx_ll_adc.h#L1828
just a quick check, its still in the source in the link from Jerome.

@JojoS62
Copy link
Contributor Author

JojoS62 commented Jan 10, 2020

they're just required to start complaining because it's not valid C++17.

yes, but very haevy... And I think the compiler warning should remain, it makes sense to show that some part of the code is depracted.

@kjbracey
Copy link
Contributor

kjbracey commented Jan 10, 2020

Problem is that if it's like ARMC6 it won't just warn once, or once per file, it will be 100s of times in a header file included by every C++ file...

Just to clarify, it gnu++17 normally accepts it with a warning, but -Wno-register disables the warning, right?

@JojoS62
Copy link
Contributor Author

JojoS62 commented Jan 10, 2020

yes, thats correct. And that is how it was fixed for ARMC6, only the compile switch has different name. For ARMC6, it has been turned off already: "-Wno-deprecated-register" is in the profiles.

@JojoS62
Copy link
Contributor Author

JojoS62 commented Jan 10, 2020

There are thousands of warnings because 'register' is used in many headerfiles.

I'm currently compiling, it looks like affected headers are _ll_rtc.h, _ll_tim.h and _ll_adc.h.

@kjbracey
Copy link
Contributor

kjbracey commented Jan 10, 2020

For ARMC6, it has been turned off already

Because ARMC6 produces the warning for C++14 mode, so it needs the warning suppressed in the standard C++14 profile.

If you're making your own custom C++17 profile for GCC then you can obviously add the warning suppression in the same custom profile. But if you want to submit a patch to put it in the base profile, that's fine. I guess it makes it easier to switch to C++17 mode (possibly through a menu in an IDE export).

@JojoS62
Copy link
Contributor Author

JojoS62 commented Jan 10, 2020

yes, I can prepare the PR. But again, this is curing the symptoms. The ideal solution would be a clean HAL code. Removing the 'register' directives from the code makes no change in the compilation as the compilers ignore this for many years already.

I have also just compared the binaries, they have for gcc exactly the same size for c++14 and c++17, also regardless of the setting for warning/ no warning.

@JojoS62
Copy link
Contributor Author

JojoS62 commented Jan 10, 2020

If you're making your own custom C++17 profile for GCC then you can obviously add the warning suppression in the same custom profile

sure, this is possible. But the deprecated code is not a problem with Mbed, is there a chance that the HAL gets updated? Can I make a PR in the HAL repo?

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 10, 2020

@JojoS62 Yes you can make PR updating HAL.

I would like to triage this issue but as it is too generic, I am not certain where it belongs and who should fix it (teams). We should split this one to modules. We can help but first should find offenders there. Besides ST driver, is there any other?

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 6, 2020

@JojoS62 Lets simplify this issue. It won't move anywhere with the current scope

Besides ST driver, if there are any other, please create separate issues. I'll close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants