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

Toolchain C++ headers can be included when libstdc++ is not selected #36644

Closed
stephanosio opened this issue Jun 30, 2021 · 6 comments · Fixed by #38476
Closed

Toolchain C++ headers can be included when libstdc++ is not selected #36644

stephanosio opened this issue Jun 30, 2021 · 6 comments · Fixed by #38476
Assignees
Labels
area: C++ bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Milestone

Comments

@stephanosio
Copy link
Member

stephanosio commented Jun 30, 2021

Status quo
The Zephyr build system does not specify -nostdinc argument when newlib is enabled (CONFIG_NEWLIB_LIBC=y):

if (NOT CONFIG_NEWLIB_LIBC AND
NOT COMPILER STREQUAL "xcc" AND
NOT ZEPHYR_TOOLCHAIN_VARIANT STREQUAL "espressif" AND
NOT CONFIG_NATIVE_APPLICATION)
set_compiler_property(PROPERTY nostdinc -nostdinc)
set_compiler_property(APPEND PROPERTY nostdinc_include ${NOSTDINC})
endif()

This has the effect of making the toolchain-provided standard headers (which includes newlib standard C library headers for the currently supported toolchains) available for inclusion.

When CONFIG_LIB_CPLUSPLUS=y, which semantically enables the use of libstdc++, this also has the intended effect since the toolchain-provided libstdc++ C++ standard library headers are available for inclusion when -nostdinc is not specified.

When CONFIG_NEWLIB_LIBC=y and CONFIG_LIB_CPLUSPLUS=n however, this has an unintended effect of making the libstdc++ headers available for inclusion even when it was never the developer's intention to make use of the toolchain-provided libstdc++ features, as observed in #36612 (comment).

The libstdc++, also known as the GNU C++ library, mainly consists of the standard C++ library header files, which include both definitions and implementations, and the standard C++ library object archive, which provides the implementations for the methods that do not come with implementation in the headers.

The current definition of CONFIG_LIB_CPLUSPLUS is "Link with STD C++ library":

config LIB_CPLUSPLUS
bool "Link with STD C++ library"

One may possibly argue that allowing the use of libstdc++ headers without linking the libstdc++ archive, alongside the minimal C++ runtime implemented by the Zephyr C++ subsystem (one might call this a "hybrid" scheme), is a both valid and intended scheme; but, in reality, this is simply not a correct thing to do for the following reasons and need to be classified as a bug:

  1. In traditional sense, headers are supposed to provide definitions and the object archive (.a file) is supposed to provide the implementations. In terms of C++, this separation often gets blurred and, as with the libstdc++, many small method implementations tend to get put in the header file. This does not mean that you can rely on the headers to provide the full implementation and making use of the headers without linking the relevant object files leaves the build success at the mercy of what gets put in the headers and source files on the libstdc++ side. This is non-deterministic and not a correct behaviour.
  2. libstdc++ headers are provided by the toolchain and its version may vary with the toolchain being used. Mixing libstdc++ headers with the minimal C++ library provided by what is currently known as the Zephyr C++ subsystem can lead to version-dependent compatibility issues as well as unexpected behaviours. We cannot afford to have this.
  3. gcc is currently the toolchain of choice for Zephyr and libstdc++ is shown as an example here for that reason. In the future, we will be adding support for other toolchains (e.g. Clang/LLVM), and this "hybrid" scheme, once again, makes that very non-deterministic and leaves things at the mercy of the toolchain type and version. We cannot afford to maintain such a non-deterministic scheme.

To be
As briefly noted in #36612 (comment):

  1. Disallow using libstdc++ headers when CONFIG_LIB_CPLUSPLUS=n by specifying -nostdinc++.
  2. Refactor C++ subsystem (subsys/cpp) such that the separation between the "minimal C++ runtime library" and the toolchain-specific C++ runtime library integration (glue/shim) is clear.
  3. Possibly downgrade "C++ subsystem" to "C++ library" (lib/cpp) and adopt the same scheme as the C library (lib/libc).
  4. Consider expanding the scope of "minimal C++ runtime library" such that basic standard C++ features are available in a toolchain-agnostic way (similar to the concept of minimal C library -- lib/libc/minimal).
  5. Consider adopting a BSD or Apache licensed C++ runtime library (e.g. Apache C++ Standard Library, LLVM C++ Standard Library, ...).
@stephanosio stephanosio added bug The issue is a bug, or the PR is fixing a bug area: C++ labels Jun 30, 2021
@stephanosio stephanosio added this to the v2.7.0 milestone Jun 30, 2021
@stephanosio stephanosio self-assigned this Jun 30, 2021
@stephanosio
Copy link
Member Author

@yperess
Copy link
Collaborator

yperess commented Jun 30, 2021

Just to make sure (I saw that you self assigned this issue) I'll just update my PR to match the appropriate C++ standard and then wait for this bug to be resolved? I don't want to accidentally duplicate work.

@stephanosio
Copy link
Member Author

stephanosio commented Jun 30, 2021

Just to make sure (I saw that you self assigned this issue) I'll just update my PR to match the appropriate C++ standard and then wait for this bug to be resolved? I don't want to accidentally duplicate work.

@yperess As far as #36612 is concerned, I was expecting the new header files to be updated to comply with the C++ standard. If you would like to look into this issue as well, please let me know and I will update the assignment.

@yperess
Copy link
Collaborator

yperess commented Jun 30, 2021

I've update #36612 to match the C++ style modeling it after the GNU cstdint file. It passes CQ now as well as our build with the custom toolchain. I'll let you know if I have some free cycles to work on this issue (I'm very interested in ensuring good support for C++).

@github-actions
Copy link

github-actions bot commented Sep 5, 2021

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Sep 5, 2021
@stephanosio stephanosio removed the Stale label Sep 5, 2021
@stephanosio
Copy link
Member Author

NOTE: For 2.7, we will keep the existing structure and only provide a fix with the minimum amount of changes.

In the future, the C++ support will be completely re-architected to support multiple toolchains (further details to be discussed and finalised in the Toolchain WG meetings, alongside the C library integration refactoring).

stephanosio added a commit to stephanosio/zephyr that referenced this issue Sep 21, 2021
The TensorFlow Lite module makes use of the features provided by the
standard C++ library (e.g. `#include <functional>`), so the standard
C++ library config must be enabled for it.

This used to work without `CONFIG_LIB_CPLUSPLUS=y` due to the bug
described in the issue zephyrproject-rtos#36644.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
nashif pushed a commit that referenced this issue Sep 21, 2021
The TensorFlow Lite module makes use of the features provided by the
standard C++ library (e.g. `#include <functional>`), so the standard
C++ library config must be enabled for it.

This used to work without `CONFIG_LIB_CPLUSPLUS=y` due to the bug
described in the issue #36644.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
Rushybrook pushed a commit to Rushybrook/zephyr that referenced this issue Oct 21, 2021
The TensorFlow Lite module makes use of the features provided by the
standard C++ library (e.g. `#include <functional>`), so the standard
C++ library config must be enabled for it.

This used to work without `CONFIG_LIB_CPLUSPLUS=y` due to the bug
described in the issue zephyrproject-rtos#36644.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
@stephanosio stephanosio mentioned this issue May 19, 2022
8 tasks
williamtcdns added a commit to williamtcdns/zephyr that referenced this issue Sep 23, 2024
C++ examples such as `samples/cpp/cpp_synchronization` do not build
on xtensa because some toolchain headers such as <limits.h> which is
a C header turns out to depend on a C++ header when included in C++
mode.

This change preserves the fix for issue zephyrproject-rtos#36644 while selectively
enabling the use of C++ standard includes.

Signed-off-by: William Tambe <williamt@cadence.com>
williamtcdns added a commit to williamtcdns/zephyr that referenced this issue Sep 23, 2024
C++ examples such as `samples/cpp/cpp_synchronization` do not build
on xtensa because some toolchain headers such as <limits.h> which is
a C header turns out to depend on a C++ header when included in C++
mode.

This change preserves the fix for issue zephyrproject-rtos#36644 while
selectively enabling the use of C++ standard includes.

Signed-off-by: William Tambe <williamt@cadence.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

Successfully merging a pull request may close this issue.

3 participants