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

Zephyr does not define minimal C++ language standard requirement and does not track it #55204

Closed
aborisovich opened this issue Feb 27, 2023 · 16 comments
Assignees
Labels
area: C++ bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@aborisovich
Copy link
Collaborator

aborisovich commented Feb 27, 2023

Describe the bug
Zephyr defines C11 as minimal supported standard for C language in C language support - language standards.
Similar documentation article: C++ language support does not have "language standard" section. Roadmap #31281 also does not say a word about minimal requirements. Thus, minimal C++ version supported is unknown. This is a serious problem since:

  • C++ language standards have much more changes between releases than C language.
  • Custom toolchains for embedded systems often concentrate on C language support (due to its popularity) leaving C++ language with old standard (like dreadful C++98).

Concrete problem encountered:
SOF project uses Cadence Xtensa toolchain "xt-xcc" that is based on GNU 4.2.0 compiler for Intel Meteorlake audio firmware.
Meteorlake board has some C++ code that uses Zephyr #include <zephyr/sys/util.h> that provides C/C++ interropt helpers at the time of writing. "xt-xcc" is obsolete for some time and latest support language is C++98 standard.
Everything was working fine in our CI until PR #53405 , where author added constexpr keyword usage in commit deab09d .
Author @cfriedt in that PR claims (selective quote):

There should be a announcement for this and the code that suddenly requires new C++

As mentioned above, this is all in line with Zephyr's suggested ISO C and C++ standards (C11, C++11).

Proof of wrong:
Fuction template definition __z_log2_impl from include/zephyr/sys/util.h in some conditions returns value A and in other conditions value B. According to C++ standard ISO/IEC 14882:2011 §7.1.5.3:

The definition of a constexpr function shall satisfy the following constraints:

  • it shall not be virtual (10.3);
  • its return type shall be a literal type;
  • each of its parameter types shall be a literal type;
  • its function-body shall be = delete, = default, or a compound-statement that contains only
    • null statements,
    • static_assert-declarations
    • typedef declarations and alias-declarations that do not define classes or enumerations,
    • using-declarations,
    • using-directives,
    • and exactly one return statement;
  • every constructor call and implicit conversion used in initializing the return value (6.6.3, 8.5) shall be
    one of those allowed in a constant expression (5.19).

Also, cppreference.com - constexpr confirms this:
Quote marked "(until C++14)":

the function body must be either deleted or defaulted or contain only the following:

To Reproduce
Steps to reproduce the behavior:

  1. Open my minimal reproduction project with Compiler Explorer:
    https://godbolt.org/z/78sWY65v4
  2. Play around with compiler version and C++ standard.
    Note: compiler x86-64 gcc 12.2 compiles minimal example using C++11 without a warning!
    Change compiler to: x86-64 gcc 11.3 to receive an error (related to multiple return statements in constexpr function):

header.h:17:1: error: body of 'constexpr' function 'constexpr int __z_log2_impl(T) [with T = int]' not a return-statement
17 | }
| ^

  1. Change CMake line:
    set_property(TARGET output.s PROPERTY CXX_STANDARD 11) to
    set_property(TARGET output.s PROPERTY CXX_STANDARD 14)
    and watch how everything compiles successfully.

Expected behavior

  1. Zephyr defines minimal C++ language standard in the official documentation.
    Right now it is C++14 if we do not roll back PR lib: os: PoT utilities, hash function, hash map (hash table) #53405 (probably - remaining C++ code needs verification too!)
  2. Developers know, and check the C++ standard of the code they wright! Please note, that modern compilers GCC, Clang, MSVC tend to provide "backward compatibility" to old C++ standards what results in non-conforming code being compiled without an issue!
    If the compiler supports C++17 or C++20 it is possible that it will compile the code marked -std=c++11 that should not compile with that standard due to compiler extensions they provide! We need to watch the conformance carefully, maybe some static analysis tools?

Impact
SOF project has to urgently upgrade toolchain to new Cadence Xtensa "xt-clang" based on clang, that allows to use up to C++17 standard. If we do not do this, we are unable upgrade Zephyr revisions since our code will not build!
Luckily, we had been preparing for toolchain change in thesofproject/sof#7027 for some time and have only 2 .cpp source files at the time of writing.

Logs and console output
Compiling with xt-xcc: xt-xcc.log
Compiling with xt-clang -std=c++11: xt-clang C++11.log

Environment (please complete the following information):

  • OS: Windows, Linux

Additional context
N/A

@aborisovich
Copy link
Collaborator Author

For example MSVC compiler has /permissive- flag. Pity gcc does not something similar. I mean there is -fpermissive flag that is supposed to convert errors related to non-conformant code to warnings (that code was compiled using compiler extensions). However, my example in https://godbolt.org/z/78sWY65v4 proves that they do not take this flag seriously. Maybe we should consider some external tool for the conformance tests?

@cfriedt
Copy link
Member

cfriedt commented Feb 27, 2023

@aborisovich - documentation only goes so far.

Why not contribute a PR? Perhaps a test so you are not inconvenienced the same way in the future.

It will probably amount to only a few lines of code.

@aborisovich
Copy link
Collaborator Author

aborisovich commented Feb 27, 2023

@aborisovich - documentation only goes so far.

Why not contribute a PR? Perhaps a test so you are not inconvenienced the same way in the future.

It will probably amount to only a few lines of code.

Subject is certainly not easy and we need some tool that would run as part of Zephyr CI. And it will certainly not be only a few lines of code. I'm looking for potential solutions. Maybe something like cppcheck added to Compliance Checks workflow would be good for checking standard conformance...
EDIT:
Looks like cppcheck does not have conformance validation checks.
The easiest solution I can think of right now is just trying to compile not only with GCC but also with Clang.
It looks like newest x86-64 clang 15.0.0 issues the warnings correctly when we compile with -std=C++11:

Step cmake returned: 0
-- The CXX compiler identification is Clang 15.0.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /opt/compiler-explorer/clang-15.0.0/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /app/build
Step build returned: 0
[ 50%] Building CXX object CMakeFiles/output.s.dir/main.cpp.o
[100%] Linking CXX executable output.s
[100%] Built target output.s
In file included from /app/main.cpp:1:
header.h:5:2: warning: use of this statement in a constexpr function is a C++14 extension [-Wc++14-extensions]
if (sizeof(x) < sizeof(unsigned int)) {
^
header.h:16:2: warning: multiple return statements in constexpr function is a C++14 extension [-Wc++14-extensions]
return -1;
^
header.h:6:3: note: previous return statement is here
return ((x) < 1) * (-1) +
^
header.h:9:3: note: previous return statement is here
return ((x) < 1) * (-1) +
^
2 warnings generated.

(referencing my minimal reproduction example).
It looks like Clang actually performs some standard conformance testing. Quote from Clang docs

The LLVM bug tracker uses the "c++" label, as well as mode-specific labels such as "c++11", "c++14", and so on, to track known bugs with Clang's language conformance.

@cfriedt
Copy link
Member

cfriedt commented Feb 28, 2023

@aborisovich - can you extract just the relevant portion of your error from the log and put it into a markdown code block, so that the error is a bit more obvious / searchable?

It's a bit difficult to pinpoint where the actual error is and it would help if the issue were more concise. This would help others try to fix your issue without having access to your compiler.

Can you provide a way to reproduce using west or twister?

Your issue can likely be solved relatively easily with a simple code change. Additional static analysis would not solve any aspect of the actual problem. It would just describe it differently.

And please, again, critique the code, not the author :-) You're not here to discredit me, right? You're here to hopefully get help to solve your problem.

@nashif nashif added the priority: medium Medium impact/importance bug label Feb 28, 2023
@aborisovich
Copy link
Collaborator Author

@cfriedt I never, ever meant to discredit anyone. I assume that as obvious thing, that we developers work with mutual respect.
Bugs, unspecified behaviors are constant part of our work. Moreover, I strongly approve what you do - C++ is a very needed language in Zephyr and I hope it will be replacing or complimenting C in future. When I wrote "you are doing something wrong" I meat it as a quote, not as an expression targeting you directly. By this quote I meant "Zephyr project systems providing API compatibility". I wanted to indicate importance of the problem I described above and potentially severe impact, so we can harden the processes in Zephyr.

@cfriedt
Copy link
Member

cfriedt commented Feb 28, 2023

@aborisovich - no harsh feelings. I'm glad that we can work together to make things better.

Please let me know if you need any help.

@jwakely
Copy link

jwakely commented Mar 1, 2023

Using -std=c++11 -pedantic should get you similar warnings with GCC. I'm not sure which backwards compatibility extensions you're worried about, generally GCC and clang will warn (or error) if you use language features that are not supported by the standard dialect selected with -std. You just need to use the option to ask for warnings about extensions, which is -pedantic.

@teburd
Copy link
Collaborator

teburd commented Mar 1, 2023

Zephyr docs don't explicitly say C11 or even C99 is required. C11 is still on the table as a pending requirement in RFC

@andyross
Copy link
Contributor

andyross commented Mar 1, 2023

FWIW: as of #55290 the current functional status of the tree is something like: Zephyr supports building C++ applications (provided toolchain support) with any issued C++ standard equal to or later than C++98. Support for specific standard versions needs to be configured via one of the CONFIG_STD_CPPxx choice values.

That sounds about right to me. We aren't a C++ project. We shouldn't be relying on C++ features to build our OS. I don't know necessarily that ancient standards are important per se, as out of tree apps move on (and in particular as SOF moves to xt-clang) I don't see why we can't bump that version in the future. But for now it's not a big hardship to maintain C++98 support.

andyross added a commit to andyross/zephyr that referenced this issue Mar 1, 2023
We weren't explicit about what we mean by "C++ support" as it relates
to standards versions, and it's been causing some friction in the tree
vs. some of our cruftier toolchains.  Add a paragraph to clarify
things.

Fixes zephyrproject-rtos#55204

Signed-off-by: Andy Ross <andyross@google.com>
@andyross
Copy link
Contributor

andyross commented Mar 1, 2023

(and having said that it's clear I should just put that text in the docs and mark it as fixing this bug)

andyross added a commit to andyross/zephyr that referenced this issue Mar 1, 2023
We weren't explicit about what we mean by "C++ support" as it relates
to standards versions, and it's been causing some friction in the tree
vs. some of our cruftier toolchains.  Add a paragraph to clarify
things.

Fixes zephyrproject-rtos#55204

Signed-off-by: Andy Ross <andyross@google.com>
andyross added a commit to andyross/zephyr that referenced this issue Mar 1, 2023
We weren't explicit about what we mean by "C++ support" as it relates
to standards versions, and it's been causing some friction in the tree
vs. some of our cruftier toolchains.  Add a paragraph to clarify
things.

Fixes zephyrproject-rtos#55204

Signed-off-by: Andy Ross <andyross@google.com>
andyross added a commit to andyross/zephyr that referenced this issue Mar 1, 2023
We weren't explicit about what we mean by "C++ support" as it relates
to standards versions, and it's been causing some friction in the tree
vs. some of our cruftier toolchains.  Add a paragraph to clarify
things.

Fixes zephyrproject-rtos#55204

Signed-off-by: Andy Ross <andyross@google.com>
@stephanosio
Copy link
Member

Zephyr defines C11 as minimal supported standard for C language in C language support - language standards.

That is not necessarily true. The documentation says that most parts of Zephyr only make use of the features available in C99 with some additional components requiring C11 -- i.e. the "minimum" compiler C standard support requirement varies depending on which component(s) you require.

The documentation does not designate a specific "minimum supported" (or "maximum allowed" for that matter) C standard version because some components may benefit from the features available in a more recent version of the standard; nevertheless, the recommendation is to stick with C99 in general and C11 at most.

Expected behavior

  1. Zephyr defines minimal C++ language standard in the official documentation.

For the same reason described above, I would refrain from defining a hard limit; especially for C++ because newer C++ standards have many appealing features.

The documentation should only provide the general recommendation (and the de facto requirement for the core components of Zephyr) for the C++ standard conformance, which would be to stick with C++11 in general. It would be preferable to allow a more recent version of the C++ standard for specific components and modules.

andyross added a commit to andyross/zephyr that referenced this issue Mar 1, 2023
We weren't explicit about what we mean by "C++ support" as it relates
to standards versions, and it's been causing some friction in the tree
vs. some of our cruftier toolchains.  Add a paragraph to clarify
things.

Fixes zephyrproject-rtos#55204

Signed-off-by: Andy Ross <andyross@google.com>
@cfriedt cfriedt closed this as completed in 10b9aab Mar 2, 2023
@aborisovich
Copy link
Collaborator Author

aborisovich commented Mar 3, 2023

I reopen this issue because it looks to me that you may missed the key point I am trying to show here.
This statement:

For the same reason described above, I would refrain from defining a hard limit; especially for C++ because newer C++ standards have many appealing features.

Is not a resolution to problem with using C++. I agree that new language feature are appealing, but please note what I wrote in the issue description:

Custom toolchains for embedded systems often concentrate on C language support (due to its popularity) leaving C++ language with old standard (like dreadful C++98).

What if the zephyr application that uses some custom toolchain X to build embedded projects does not have a compiler supporting C++11 or newer? You basically block them from using your services (Zephyr APIs).
@andyross, filling up documentation with "We support C++ standard whatever found in the given file you compile" is not a solution either. I would rather call it "discrediting a problem" than solving it.
This problem is not related to C, because Zephyr does great job of hiding C language features under #ifdef macros.
Example: What would happen to all zephyr applications if you suddenly add to very commonly used header (like some util header) alignof keyword (supported since C11).
All zephyr applications using C99 would stop to work and would be forced to use C11 compiler right?
You basically do the same for C++ right now, and cover the issue with a blanked - do not do this.

@aborisovich aborisovich reopened this Mar 3, 2023
@andyross
Copy link
Contributor

andyross commented Mar 3, 2023

Hm... as of right now we guarantee that all the OS headers build correctly with C++98 compilers (specifically gcc and clang with -std=c++98), and we're testing this along with all the other . It's true it was a hole, but I'm pretty sure it's been plugged. Obviously the discussion as to when to deprecate C++98 will be contentious, but for now at least that's the requirement.

Which is to say: if you push a PR right now using alignof or rvalue references or whatever inside an #ifdef __cplusplus, it will fail in CI. I think that's what you want, no?

(Note that there are other issues with C++ support at the toolchain level, like "how to link in the standard library" that are quite a bit farther behind. So while the OS has support lots of platforms won't quite build C++98 correctly even if their compilers technically support it; xcc and xt-clang are both in this category for now.)

@aborisovich
Copy link
Collaborator Author

aborisovich commented Mar 3, 2023

Maybe I need to make it simple and more concise.
There are 2 problems:

  1. Commit deab09d should not even compile without setting Kconfig option CONFIG_STD_CPP14=y but it does (using CONFIG_STD_CPP11=y) because the compilers you use does not check standard conformity correctly.
  2. Approach "we add whatever we want with whatever standard we want" is not a good one if you intent to keep other projects using Zephyr RTOS. At least provide the minimal standard information in the sources that you had modified.
    Eg. You create a include/zephyr/sys/util.h file that works fine with C++98. Somebody adds there consteval keyword not wrapped under #ifdefs C++20. Suddently zephy rapplications using include/zephyr/sys/util.h may not use it anymore.
    But worst, they have to discover why because you do not announce anything in the file!

@andyross
Copy link
Contributor

andyross commented Mar 3, 2023

Yeah, those problems are fixed as of #55290

Please open a new bug if you find a way to add non-C++98 code to an OS header without doing proper version detection.

@andyross andyross closed this as completed Mar 3, 2023
@aborisovich
Copy link
Collaborator Author

aborisovich commented Mar 3, 2023

Thanks @andyross . Did not notice a whole PR just a single commit modifying docs. Sorry for the hussle and thank you for addressing the issue.

Using -std=c++11 -pedantic should get you similar warnings with GCC. I'm not sure which backwards compatibility extensions you're worried about, generally GCC and clang will warn (or error) if you use language features that are not supported by the standard dialect selected with -std. You just need to use the option to ask for warnings about extensions, which is -pedantic.

Indeed adding a -pedantic flag to newest gcc also resolves the issue.

nordicjm pushed a commit to nordicjm/zephyr that referenced this issue Mar 8, 2023
We weren't explicit about what we mean by "C++ support" as it relates
to standards versions, and it's been causing some friction in the tree
vs. some of our cruftier toolchains.  Add a paragraph to clarify
things.

Fixes zephyrproject-rtos#55204

Signed-off-by: Andy Ross <andyross@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: C++ bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

No branches or pull requests

7 participants