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.

Sign up for GitHub

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

Jump to bottom

Build: Build with newlib-nano c library #12735

Merged
merged 2 commits into from
May 15, 2019
Merged

Build: Build with newlib-nano c library #12735

merged 2 commits into from
May 15, 2019

Conversation

Benichou34
Copy link
Contributor

Add an option for building with newlib-nano library.
The newlib-nano library for ARM embedded processors is a part of the
GNU Tools for ARM Embedded Processors.

@codecov-io
Copy link

codecov-io commented Jan 25, 2019 •
edited
Loading

Codecov Report

Merging #12735 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12735      +/-   ##
==========================================
- Coverage    52.3%   52.26%   -0.04%     
==========================================
  Files         307      307              
  Lines       45408    45408              
  Branches    10504    10504              
==========================================
- Hits        23749    23733      -16     
- Misses      16874    16886      +12     
- Partials     4785     4789       +4
Impacted Files Coverage Δ
subsys/testsuite/ztest/include/ztest_assert.h 35% <0%> (-35%) ⬇️
subsys/testsuite/ztest/src/ztest.c 65.28% <0%> (-7.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fe3aea...fa4551a. Read the comment docs.

SebastianBoe
SebastianBoe requested changes Jan 28, 2019
View reviewed changes
Copy link
Collaborator

@SebastianBoe SebastianBoe left a comment

Choose a reason for hiding this comment

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

We need a test that makes sure that this does not regress.

@Benichou34
Copy link
Contributor Author

Do you want me to add a test to all /tests/kernel/directories ?

SebastianBoe
SebastianBoe requested changes Jan 29, 2019
View reviewed changes
CMakeLists.txt Outdated
@@ -191,6 +191,12 @@ zephyr_ld_options(
)
endif()

if(CONFIG_NEWLIB_LIBC_NANO)
zephyr_ld_options(
Copy link
Collaborator

Choose a reason for hiding this comment

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

zephyr_ld_options will test and skip the flag if it is not supported.

In this situation, we don't want the users configuration to be silently ignored, so we should use 'zephyr_link_libraries' instead.

CMakeLists.txt Outdated
@@ -191,6 +191,12 @@ zephyr_ld_options(
)
endif()

if(CONFIG_NEWLIB_LIBC_NANO)
zephyr_ld_options(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation.

CMakeLists.txt Outdated
zephyr_ld_options(
-specs=nano.specs
)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move this into libc/libc/newlib/CMakeLists.txt

since nano depends on newlib.

lib/libc/Kconfig Show resolved Hide resolved
tests/lib/mem_alloc/testcase.yaml Outdated
libraries.libc.newlibnano:
extra_args: CONF_FILE=prj_newlibnano.conf
arch_whitelist: arm
platform_exclude: qemu_x86_64 # No newlib
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't the 'arm' whitelist prevent qemu_x86_64 from being run already?

@SebastianBoe
Copy link
Collaborator

Do you want me to add a test to all /tests/kernel/directories ?

No, what we have now is enough.

SebastianBoe
SebastianBoe reviewed Jan 29, 2019
View reviewed changes
tests/lib/mem_alloc/testcase.yaml Outdated
@@ -8,3 +8,8 @@ tests:
arch_exclude: posix
platform_exclude: qemu_x86_64 # No newlib
tags: clib newlib
libraries.libc.newlibnano:
extra_args: CONF_FILE=prj_newlibnano.conf
arch_whitelist: arm
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you need a toolchain whitelist, I believe I observed that nano was not supported by the zephyr-sdk (but gnuarmemb does support it).

@Benichou34 Benichou34 force-pushed the newlibc_nano branch 2 times, most recently from 5f417e7 to 66d336d Compare February 20, 2019 16:07
aurel32
aurel32 reviewed Feb 21, 2019
View reviewed changes
lib/libc/newlib/CMakeLists.txt
-specs=nano.specs
)
endif()

Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably want to pass that flag also for compiling using zephyr_compile_options, so that the nano headers defined in the spec file are used instead of the default ones.

Note however that it is currently broken by PR #13538

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that " --specs=nano.specs" is only a linker option.
Please see Nano libraries usage

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't looked at the documentation, but at the content of nano.specs on my system

*cpp_unique_options:
-isystem /usr/include/newlib/nano %(nano_cpp_unique_options)

This adds a new system include path pointing to /usr/include/newlib/nano. This directory contains a single file newlib.h containing the newlib configuration. In the nano version a few options have been disabled. Some of those defines are used by other newlib headers changing the signature of some functions, so I believe this is something important to do.

Note however that the Zephyr SDK 0.10.0 provides a nano.specs file, but the include path added by this file is not present. It looks like a packaging issue, I'll investigate and fill a bug.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See also https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=801338

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the "zephyr_compile_options" into the "CMakeLists.txt"

@aurel32 aurel32 mentioned this pull request Feb 22, 2019
Closed
@zephyrbot
Copy link
Collaborator

zephyrbot commented Feb 25, 2019 •
edited
Loading

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

@Benichou34
Copy link
Contributor Author

Hello @SebastianBoe,
I think that all your change resquests was resolved.

galak
galak requested changes Apr 26, 2019
View reviewed changes
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Lets only expose the Kconfig option on the ARM embedded toolchain for now.

lib/libc/Kconfig Show resolved Hide resolved
Benichou34 added 2 commits May 15, 2019 06:53
@Benichou34 @galak
Build: Build with newlib-nano c library
bc4ca25 
Add an option for building with newlib-nano library.
The newlib-nano library for ARM embedded processors is a part of the
GNU Tools for ARM Embedded Processors.
Add mem_alloc tests with newlib nano.

Signed-off-by: Benoit Leforestier <[email protected]>
@Benichou34 @galak
codeowner: add codeowners for /test/lib
151f468 
Add codeowners for /test/lib

Signed-off-by: Benoit Leforestier <[email protected]>
@Benichou34 Benichou34 requested a review from andrewboie as a code owner May 15, 2019 11:54
@galak galak merged commit 3ef6308 into zephyrproject-rtos:master May 15, 2019
@Benichou34 Benichou34 deleted the newlibc_nano branch May 27, 2019 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aurel32 aurel32 aurel32 left review comments

@galak galak galak approved these changes

@andrewboie andrewboie andrewboie approved these changes

@SebastianBoe SebastianBoe SebastianBoe approved these changes

@nashif nashif Awaiting requested review from nashif

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Benichou34 @codecov-io @SebastianBoe @zephyrbot @andrewboie @aurel32 @galak