-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
libc/picolibc: Don't enable malloc by default #63259
libc/picolibc: Don't enable malloc by default #63259
Conversation
5667746
to
9fc000e
Compare
9fc000e
to
54d501d
Compare
I've hacked this up to add a new config entry, |
54d501d
to
6bab185
Compare
And I think this PR should add such dependency to the likes of Thrift, TfLite, etc rather than doing it at the sample level, no? |
It probably won't be perfect, but yeah, it would be better to at least get started with this. |
6bab185
to
931dc4a
Compare
066ad46
to
1d7105b
Compare
rebased to fix conflict. |
@@ -6,6 +6,7 @@ | |||
menuconfig NET_SOCKETS | |||
bool "BSD Sockets compatible API" | |||
select FDTABLE | |||
select REQUIRES_MALLOC if DNS_RESOLVER || NET_IP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically, getaddrinfo and freeaddrinfo could maybe use a slab if malloc is unavailable. What do you think @rlubos ? Not a change required here, but maybe a good future enhancement. Even malloc-less systems should be able to resolve.
@@ -7,6 +7,7 @@ config ZEPHYR_THRIFT_MODULE | |||
menuconfig THRIFT | |||
bool "Support for Thrift [EXPERIMENTAL]" | |||
select EXPERIMENTAL | |||
select REQUIRES_MALLOC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One day, It would be cool to not require a global allocator for Thrift
Seems like this might uncover more malloc dependencies. Having malloc exist but always fail (with a zero-sized arena) seems like a user trap to me? |
Now that picolibc is the default C library, tests which want to check the minimal library need to explictly select it. Signed-off-by: Keith Packard <[email protected]>
Make sure these tests don't run when the underlying target requires a full C library, and hence won't end up using the minimal C library anyways. Signed-off-by: Keith Packard <[email protected]>
Instead of making applications use C library specific settings to enable malloc support, 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 <[email protected]>
Instead of making applications use C library specific settings to specify the desired minimum malloc arena size, 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 <[email protected]>
Most C++ applications will end up using various language features which involve allocating memory, so most applications will need malloc. Applications not needing it may still set REQUIRES_MALLOC=n if they don't. Signed-off-by: Keith Packard <[email protected]>
The getaddrinfo function uses calloc when either a DNS resolver or an IP stack is available. Indicate that by selecting REQUIRES_MALLOC when necessary. Signed-off-by: Keith Packard <[email protected]>
This module uses various malloc APIs internally. Signed-off-by: Keith Packard <[email protected]>
These modules use malloc APIs internally. Add REQUIRES_MALLOC to their Kconfig settings to ensure the C library provides an implementation. Signed-off-by: Keith Packard <[email protected]>
This module uses malloc in the common code. Signed-off-by: Keith Packard <[email protected]>
The da1469x_pd driver uses calloc when reading trimv information. Signed-off-by: Keith Packard <[email protected]>
These hash_map tests all use malloc directly to prepare various test elements. Signed-off-by: Keith Packard <[email protected]>
This sample uses malloc for a local buffer. Signed-off-by: Keith Packard <[email protected]>
strftime deals with timezones and the picolibc (and newlib) implementations of that code currently uses malloc. That will get fixed upstream eventually, but for now, we need malloc to use it. Signed-off-by: Keith Packard <[email protected]>
1d7105b
to
78d37d3
Compare
These tests allocate output buffers at runtime. Signed-off-by: Keith Packard <[email protected]>
This test uses malloc for temporary buffers. Signed-off-by: Keith Packard <[email protected]>
This test uses malloc for output buffers. Signed-off-by: Keith Packard <[email protected]>
Instead of relying on the default setting of `MINIMAL_LIBC_MALLOC` to pull in the common malloc implementation, enable it whenever the application indicates that it needs malloc through the REQUIRES_MALLOC setting. Signed-off-by: Keith Packard <[email protected]>
When malloc is included in the build, the heap initialization code will be pulled in even if the application doesn't actually use malloc. This increases the size of executables by a few hundred bytes and delays system startup. Select COMMON_LIBC_MALLOC only when applications indicate that they require malloc by setting REQUIRES_MALLOC. Signed-off-by: Keith Packard <[email protected]>
78d37d3
to
6a901af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM
I've just posted PR #65434 which is an alternative to this PR that avoids including the malloc initialization code by not using |
Ok, I think #65434 is the right way forward here -- applications do not have to declare whether they use malloc or not. When the don't, the malloc initialization code will not be included in the application. |
The common malloc implementation uses a heap which is initialized at system startup time. That gets linked into the application even if there are no references to malloc. This wastes memory and slows application startup.
When used with the minimal C library, this is "fixed" in the default configuration by setting the common malloc arena size to zero, removing the arena initialization code and providing stubs for malloc and free which are no-ops.
Picolibc adopted a different plan where a default malloc arena was provided when malloc was enabled. This ensures that malloc will be usable when it is available, which avoids a user trap where the application links but fails as soon as it tries to allocate any memory. However, that means that any application using picolibc will include the malloc arena initialization even if it doesn't use malloc. That costs a few hundred bytes of memory and a bit of startup time.
As an alternative, this patch series disables malloc entirely in the default configuration; appplications needing it will need to explicitly enable it in their configuration. All of the samples and tests which use malloc have their configurations updated.
Subsystems, drivers, libraries and modules which use malloc internally now signal their need for an allocator where necessary by selecting the
REQUIRES_MALLOC
Kconfig parameter.