rework newlib directory prioritization to fix include processing for non-Zephyr toolchains #18242
Conversation
zephyrbot
added
area: C Library
|
Background information: To understand the details you need to know what the system header search path is for a given compiler. Get this with: With By adding The contents of the zephyr toolchain path ending in With By adding Note that this build does not have |
|
All checks are passing now. Review history of this comment for details about previous failed status. |
Consistently place C++ use of extern "C" after all include directives, within the negative branch of _ASMLANGUAGE if used. Background from issue zephyrproject-rtos#17997: Declarations that use C linkage should be placed within extern "C" so the language linkage is correct when the header is included by a C++ compiler. Similarly #include directives should be outside the extern "C" to ensure the language-specific default linkage is applied to any declarations provided by the included header. See: https://en.cppreference.com/w/cpp/language/language_linkage Signed-off-by: Peter Bigot <[email protected]>
pabigot
force-pushed
the
nordic/20190813b
branch
from
2fb258d to
7033b2a
Compare
pabigot
removed
area: API
|
@pfalcon Please check this branch for the use case underlying #15627. It essentially reverts a change from #14312 that you used to justify #15937, i.e. using That change was incorrect for newlib, but is arguably appropriate for minimal libc. I don't have a test case for #15627 but if your solution depended on the newlib use of |
@pabigot: As a quick "first thing which comes to mind", based on wording above and "Commits 3 Files changed 11", I'd say that this is too late for 2.0. I'll try however to look in detail as time permits. |
|
@pfalcon Only one commit, and a change to two files, fixes this bug, which I thought was in scope for this stage of the release process. The other two commits are (1) a test case to avoid regressions, and (2) additional cleanup(bug) fixes queued in #18243 necessary to do unit tests on C++ code. However, I'm not adamant about merging this, as long as it's OK to leave C++ broken in 2.0.0 with non-Zephyr toolchains. |
So, testcases are includes in a PR that fixed that issue, #15937 , specifically, a40cc0d . I.e., with my patch, POSIX-using apps no longer need to do As can be seen, CI is green for this PR, so hopefully teh handling above is not broken. Well, I'd still hope to try it manually myself to be double-sure (tomorrow, perhaps). But otherwise, I'd leave it to @galak, the author of #14312, to vote this PR up/down. |
|
https://reviews.llvm.org/D32411 includes a commit that documents a design to support Are there non-clang/gnuc toolchains we need to worry about? |
pabigot
force-pushed
the
nordic/20190813b
branch
from
7033b2a to
60bd61f
Compare
The solution from zephyrproject-rtos#14312 of using -isystem to prioritize the position of the libc directory bypasses the effect of -ffreestanding with respect to libc symbols expected to be present in a non-hosted environment. Further, it breaks C++ with the ARM Embedded toolchain as the system fails to find the right file with #include_next. Use a more fine-grained solution that explicitly includes the underlying newlib header required for <inttypes.h> support before moving on to include the next available one, whether system or non-system. Closes zephyrproject-rtos#17564 Signed-off-by: Peter Bigot <[email protected]>
Confirms build (and run) of C++17 applications that make use of STL containers and other features. Signed-off-by: Peter Bigot <[email protected]>
pabigot
force-pushed
the
nordic/20190813b
branch
from
60bd61f to
26bedf0
Compare
zephyrbot
added
the
area: Test Framework
|
Ok, manually tested tests/posix/common, tests/posix/fs - all's ok (yeah, that should be covered by CI, but as we know there's a bit randomization and subsetting in it, so some tests aren't run all platforms, and given a trouble it took to lead POSIX-related changes into the mainline, I wanted to have an explicit checkpoint here). So, I'd be happy to +1 this, just giving some time to @galak to review too. |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport v1.14-branch
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick 654d2df47a80501a4ab5077362f0224cc361a31d 3aee7b8c96592f489617aba8873982fbcc32166b 26bedf08be2e10fa941df0353ed6902009f55b79
# Create a new branch with these backported commits.
git checkout -b backport-18242-to-v1.14-branch
# Push it to GitHub.
git push --set-upstream origin backport-18242-to-v1.14-branch
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backportThen, create a pull request where the |
This work closes #17564 and related C++ failures.
Zephyr builds with
-ffreestandingmeaning that__STDC_HOSTED__is false. Consequently the toolchain standard<stdint.h>is a minimal header that routes to<stdint-gcc.h>which defines integer types without defining the marker macro__int64_t_definedthat signals newlib<inttypes.h>to provide support forPRIu64(#14310). In hosted builds<stdint.h>instead sets some flags then delegates to the host-provided (newlib) implementation of<stdint.h>.In support of the desire to have support for integer types that aren't part of the freestanding system #14312 placed the newlib include headers before the system ones, specifically to make hosted features available in a non-hosted system.
This decision breaks building C++ applications that use standard C++ include files when using a non-Zephyr toolchain like
gnumarmemb. The C++ header<cstdlib>(referenced from<array>in #17564) wants to#include_next <stdlib.h>, but the include priority has placed the directory that provides it earlier in the search path, where it can no longer be found.The workaround succeeded with the Zephyr toolchain only because the Zephyr SDK uses a cross-compiler infrastructure technique that leaves behind and references a second copy of the base include directory that is normally needed only when building the compiler.
<stdint.h>is then found at that second location.This PR switches to a solution based on updating the include path to include an intercepting header that provides the required defines to satisfy the use case of #14310 without completely breaking the toolchain standard include processing. This should work on GCC and clang.
Note that the test case requires a patch from #18243 to support ztest with C++.