Workaround for a bug in malloc() from newlib-nano 4.1.0 (in GCC 10.3) #15045
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@LDong-Arm, thank you for your changes. |
LDong-Arm
force-pushed
the
malloc_newlib_nano
branch
3 times, most recently
from
d158822 to
8a07824
Compare
The commit 84d0689 "Nano-malloc: Fix for unwanted external heap fragmentation" from newlib 4.1.0 introduced several optimizations, one of which is as follows: When the last chunk in the free list is smaller than requested, nano_malloc() calls sbrk(0) to see if the heap's current head is adjacent to this chunk, and if so it asks sbrk() to allocate the difference in bytes only and expands the current chunk. This doesn't work if the heap consists of non-contiguous regions. sbrk(0) returns the the current region's head if the region has any remaining capacity. But if this capacity is not enough for the second (non-trivial) call to sbrk() described above, allocation will happen from the next region if available. Expanding the current chunk won't work and will result in a segmentation fault. So this optimization needs to be reverted in order to bring back compatibility with non-contiguous heaps. Before the next version of newlib becomes available and gets updated in the GCC Arm Embedded Toolchain, we work around this issue by including the fix in Mbed OS. The linker prioritizes malloc() from the project to the one from the toolchain.
This reverts commit 6649e95. In the malloc test, `ALLOC_ARRAY_SIZE` defines the *capacity* of array that stores pointers to `malloc`'d buffers. It is *not* the number of allocations. In an ideal scenario, the test makes as many allocations as possible until the heap runs out and malloc() returns NULL. So revert the capacity of the array of pointers from 50 back to 100 so this array is less likely to run out before the heap does.
LDong-Arm
force-pushed
the
malloc_newlib_nano
branch
from
8a07824 to
8e41298
Compare
LDong-Arm
commented
Sep 1, 2021
|
|
||
| #include <newlib.h> | ||
|
|
||
| #if (__NEWLIB__ == 4) && (__NEWLIB_MINOR__ == 1) && (__NEWLIB_PATCHLEVEL__ == 0) |
There was a problem hiding this comment.
We are assuming the next release will have it fixed.
0xc0170
approved these changes
Sep 2, 2021
|
Nice work bisecting this and finding the cause. |
mergify
bot
added
needs: CI
needs: work
and removed
needs: review
needs: CI
needs: work
labels
Jenkins CI Test : ✔️ SUCCESSBuild Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
Once it's merged, it'd be good to update GCC in CI to 10.3 and do a whole run as soon as possible. |
|
@ARMmbed/mbed-os-core @donatieng Please review |
saheerb
approved these changes
Sep 3, 2021
donatieng
approved these changes
Sep 3, 2021
There was a problem hiding this comment.
Thanks a lot @LDong-Arm - really good investigation, and thanks for making a workaround :)
mergify
bot
added
release version missing
|
This PR does not contain release version label after merging. |
0xc0170
added
release-type: patch
Fixed |
mbedmain
added
release-version: 6.15.0
Release-pending
and removed
release-type: patch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
The GNU Arm Embedded Toolchain 10.3-2021.07 bundles the newlib 4.1.0 release.
The commit 84d0689 "Nano-malloc: Fix for unwanted external heap fragmentation" from newlib 4.1.0 introduced several optimizations, one of which is as follows:
When the last chunk of buffer in the free list (i.e. buffers recycled by
free()) is smaller than requested,nano_malloc()callssbrk(0)to see if the heap's current head is adjacent to this chunk, and if so it askssbrk()to allocate the difference in bytes only and expands the current chunk.This doesn't work if the heap consists of non-contiguous regions.
sbrk(0)returns the the current region's head if the region has any remaining capacity. But if this capacity is not enough for the second (non-trivial) call tosbrk()described above, allocation will happen from the next region if available. Expanding the current chunk won't work and will result in a segmentation fault.So this optimization needs to be reverted in order to bring back compatibility with non-contiguous heaps. Before the next version of newlib becomes available and gets updated in the GCC Arm Embedded Toolchain, we work around this issue by including the fix in Mbed OS. The linker prioritizes
malloc()from the project over the one from the toolchain.Meantime, we try to upstream the fix to newlib so the next release will have correctly behaving
malloc().Note: The Mbed OS implementation of
sbrk()for split-heap is here.Fixes: #15023
Impact of changes
This workaround ensures
malloc()works correctly on targets with two heap regions (i.e.MBED_SPLIT_HEAP) such as K64F and DISCO_L475VG_IOT01A when using GCC 10.3-2021.07.Migration actions required
Documentation
None.
Pull request type
Test results
The CI currently uses GCC 9 because this bug prevented us from upgrading it. I tested this workaround locally with GCC 10.3. After this PR gets in, we can update the CI to use GCC 10.3.
Reviewers
@ARMmbed/mbed-os-core @mbed-os-test @donatieng @kjbracey-arm