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

Use REQUIRES_FULL_LIBC in tests and samples instead of NEWLIB_LIBC #63332

Merged
merged 4 commits into from
Oct 25, 2023
Merged

Use REQUIRES_FULL_LIBC in tests and samples instead of NEWLIB_LIBC #63332

merged 4 commits into from
Oct 25, 2023

Conversation

keith-packard
Copy link
Collaborator

To gain broader test coverage for the default C library, convert all samples and tests from specifying newlib to specifying REQUIRES_FULL_LIBC. Environments that require newlib will still use it, but otherwise these tests will build with picolibc.

@keith-packard keith-packard requested a review from a user September 29, 2023 22:25
@keith-packard
Copy link
Collaborator Author

keith-packard commented Sep 29, 2023 •
edited
Loading

This is stacked on top of the series that disables malloc-by-default in picolibc #63259

This series is now stand-alone.

@keith-packard keith-packard force-pushed the require-full-libc branch 2 times, most recently from 8664fd8 to 9a51795 Compare September 30, 2023 05:16
jukkar
jukkar reviewed Sep 30, 2023
View reviewed changes
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Networking related changes look ok, couple of minor nits about the commit messages.

subsys/net/lib/sockets/Kconfig Outdated Show resolved Hide resolved
samples/net/sockets/big_http_download/src/big_http_download.c Outdated Show resolved Hide resolved
@keith-packard keith-packard force-pushed the require-full-libc branch 3 times, most recently from 1573ad9 to 290d363 Compare October 9, 2023 22:20
@jhedberg
Copy link
Member

@keith-packard do you think this should go into the 3.5 release?

@keith-packard
Copy link
Collaborator Author

@keith-packard do you think this should go into the 3.5 release?

It's certainly nice to have the additional test coverage on the default library, and it doesn't change any application-visible code, so I'd say it's low-risk and good for validation of the picolibc transition. But, it also doesn't fix any bugs anywhere, so it might not be important enough to merge in during the RC phase.

@aescolar
Copy link
Member

aescolar commented Oct 13, 2023 •
edited
Loading

I think we need a new config flag for the second test here -- we need to know when the host C library is in use and filter tests in that case. I think that's equivalent to NATIVE_APPLICATION or (NATIVE_LIBRARY and not EXTERNAL_LIBC) -- arch_posix uses EXTERNAL_LIBC internally to drive selection of the host C library.

I'm in favor of the NATIVE_LIBC & NATIVE_LIBCPP you are introducing in this PR.
EXTERNAL_LIBC was introduced as a synonym of the host libC for the native_posix target, and in vanilla Zephyr that's all it was, but it has always been a misnomer, and somebody may have used it downstream to select an actual external C library for embedded targets.

Later we can correct all uses in tree of EXTERNAL_LIBC to be NATIVE_LIBC where appropriate.

@aescolar
Copy link
Member

@keith-packard thanks for the fixes. Note the 2 comments I left unresolved, I think those should still be fixed.

@aescolar
Copy link
Member

aescolar commented Oct 23, 2023 •
edited
Loading

@keith-packard how is this PR looking?
It would be nice to get this changes fixes in.
Specially 58f4b31 would help on other PRs.

@keith-packard
Copy link
Collaborator Author

@keith-packard how is this PR looking? It would be nice to get this changes fixes in. Specially 58f4b31 would help on other PRs.

I've rebased it this morning; there were conflicts with the removal of CONFIG_ZTEST_NEW_API which I needed to fix up. Other than that, I think it's ready to go. Let's see if CI is happy after the rebase.

@aescolar
Copy link
Member

@keith-packard
Thanks, but please don't forget https://github.com/zephyrproject-rtos/zephyr/pull/63332/files#diff-d9b05ecb84aaf9fb90748a2acbe37e8bf483c7a3c1370044dde783af45a1ab37

@keith-packard keith-packard force-pushed the require-full-libc branch 2 times, most recently from 70faf04 to e0c321c Compare October 23, 2023 16:59
aescolar
aescolar previously approved these changes Oct 24, 2023
View reviewed changes
fabiobaltieri
fabiobaltieri previously approved these changes Oct 24, 2023
View reviewed changes
@fabiobaltieri
Copy link
Member

@keith-packard could you rebase please?

@keith-packard
libc: Add REQUIRES_FLOAT_PRINTF to indirectly set printf support
5aafbad 
Instead of making applications use C library specific settings to enable
floating point support in printf, provide this indirect symbol which then
detects which C library is in use and selects the correct configuration for
each.

Signed-off-by: Keith Packard <keithp@keithp.com>
@keith-packard
Kconfig.zephyr: Add NATIVE_LIBC and NATIVE_LIBCPP
c5610f6 
These are set when building against the host C or C++ libraries. This
allows filtering tests that require use of a C library that sits atop
Zephyr OS APIs.

Signed-off-by: Keith Packard <keithp@keithp.com>
@keith-packard
tests: Switch from NEWLIB_LIBC to REQUIRES_FULL_LIBC
0890502 
Instead of forcing use of NEWLIB_LIBC, select any available complete C
library implementation. Add CONFIG_REQUIRES_FLOAT_PRINTF where needed.

Signed-off-by: Keith Packard <keithp@keithp.com>
@keith-packard
samples: Switch from NEWLIB_LIBC to REQUIRES_FULL_LIBC
9e01681 
Instead of forcing use of NEWLIB_LIBC, select any available complete C
library implementation. Add CONFIG_REQUIRES_FLOAT_PRINTF and adjust
CONFIG_LIBC_MALLOC_ARENA_SIZE as needed.

Signed-off-by: Keith Packard <keithp@keithp.com>
@aescolar aescolar merged commit 1e5c46d into zephyrproject-rtos:main Oct 25, 2023
20 checks passed
@kartben kartben mentioned this pull request Nov 13, 2023
Closed
kartben added a commit to kartben/zephyr that referenced this pull request Nov 13, 2023
@kartben
samples: lz4: reduce malloc arena size
bb5098d 
PR zephyrproject-rtos#63332 introduced a change to the LZ4 sample that set the malloc
arena to an unnecessarily large size.
After testing on native_posix, qemu_m3, and esp32s3_devkitm_appcpu, it
would appear 24K is a much more reasonable (and sufficient) size.

Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
@kartben kartben mentioned this pull request Nov 13, 2023
Merged
aescolar pushed a commit that referenced this pull request Nov 14, 2023
@kartben @aescolar
samples: lz4: reduce malloc arena size
fc4967e 
PR #63332 introduced a change to the LZ4 sample that set the malloc
arena to an unnecessarily large size.
After testing on native_posix, qemu_m3, and esp32s3_devkitm_appcpu, it
would appear 24K is a much more reasonable (and sufficient) size.

Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
Chenhongren pushed a commit to Chenhongren/zephyr that referenced this pull request Nov 17, 2023
@kartben
samples: lz4: reduce malloc arena size
11b39b1 
PR zephyrproject-rtos#63332 introduced a change to the LZ4 sample that set the malloc
arena to an unnecessarily large size.
After testing on native_posix, qemu_m3, and esp32s3_devkitm_appcpu, it
would appear 24K is a much more reasonable (and sufficient) size.

(cherry picked from commit fc4967e)

Original-Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
GitOrigin-RevId: fc4967e
Change-Id: I7b7072e6537ae69169ddaa724d0bac68cfc4a630
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5030150
Reviewed-by: Keith Short <keithshort@chromium.org>
Tested-by: Keith Short <keithshort@chromium.org>
Tested-by: ChromeOS Prod (Robot) <chromeos-ci-prod@chromeos-bot.iam.gserviceaccount.com>
Commit-Queue: Keith Short <keithshort@chromium.org>
yunheQin pushed a commit to yunheQin/zephyr that referenced this pull request Dec 6, 2023
@kartben @yunheQin
samples: lz4: reduce malloc arena size
161344b 
PR zephyrproject-rtos#63332 introduced a change to the LZ4 sample that set the malloc
arena to an unnecessarily large size.
After testing on native_posix, qemu_m3, and esp32s3_devkitm_appcpu, it
would appear 24K is a much more reasonable (and sufficient) size.

Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
@aasinclair aasinclair mentioned this pull request Jan 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jukkar jukkar jukkar left review comments

@aescolar aescolar aescolar approved these changes

@jhedberg jhedberg jhedberg approved these changes

@fabiobaltieri fabiobaltieri fabiobaltieri approved these changes

@nashif nashif Awaiting requested review from nashif

@rlubos rlubos Awaiting requested review from rlubos

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@stephanosio stephanosio Awaiting requested review from stephanosio

@Vudentz Vudentz Awaiting requested review from Vudentz

@sjanc sjanc Awaiting requested review from sjanc

@microbuilder microbuilder Awaiting requested review from microbuilder

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@ceolin ceolin Awaiting requested review from ceolin

@d3zd3z d3zd3z Awaiting requested review from d3zd3z

@galak galak Awaiting requested review from galak

@ioannisg ioannisg Awaiting requested review from ioannisg

Assignees

@rlubos rlubos

Labels
None yet
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.
6 participants
@keith-packard @jhedberg @fabiobaltieri @aescolar @jukkar @rlubos