Skip to content

cpu/esp32: fix malloc thread safety #18209

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
Merged
merged 1 commit into from
Jun 16, 2022
Merged

cpu/esp32: fix malloc thread safety #18209

merged 1 commit into from
Jun 16, 2022

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Jun 15, 2022 •
edited
Loading

Contribution description

This PR fixes the thread safety of heap allocation functions for ESP32.

Heap allocation functions in newlibc-nano for ESP32 (malloc/free/...) seem not to be thread save. tests/malloc_thread_safety fails either with

Testing: malloc()/free()
EXCEPTION!! exccause=28 (LoadProhibitedCause) @800d2b28 excvaddr=fffffffc

or with

Testing: malloc()/free()
Not all blocks were correctly freed!
mallinfo().uordblks before test: 4308, after test: 3224625688
Testing: realloc()/free()
Not all blocks were correctly freed!
mallinfo().uordblks before test: 4308, after test: 3224625688
TEST FAILED

Therefore, if heap allocation functions from newlibc-nano are used (module esp-idf-heap is not enabled) module malloc_thread_safe has to be used to make heap allocation thread safe. Heap allocation functions in ESP-IDF (module esp-idf-heap) that are used with SPI RAM or esp-wifi are already thread safe.

Testing procedure

Use

BOARD=esp32-wroom-32` make -j8 -C tests/malloc_thread_safety flash test

With this PR the test has to work, without this PR the test crashes or fails.

Issues/PRs references

Found while testing PR #18202.

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Jun 15, 2022
@gschorcht gschorcht added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 15, 2022
@gschorcht gschorcht changed the title cpu/esp32: fix malloc_thread_safety cpu/esp32: fix malloc thread safety Jun 15, 2022
@maribu maribu enabled auto-merge June 15, 2022 10:24
@gschorcht
Copy link
Contributor Author

gschorcht commented Jun 15, 2022 •
edited
Loading

Strange, there are sporadic compilation errors in CI due to different hashes in this PR, other PRs and for different applications, which I can't reproduce locally log

@maribu
Copy link
Member

maribu commented Jun 15, 2022

@kaspar030 traced it down to differences in the debug information of the elf files - the hash of stripped binaries matches again. I was able to reproduce the issue with BUILD_IN_DOCKER=1.

@gschorcht
Copy link
Contributor Author

@kaspar030 traced it down to differences in the debug information of the elf files - the hash of stripped binaries matches again. I was able to reproduce the issue with BUILD_IN_DOCKER=1.

Do you mean this time or when we already had this problem two weeks ago? I remember that @kaspar030 said at the time that the debugging information differ, but in fact it was an actual difference in the bootloader binary.

@gschorcht
Copy link
Contributor Author

gschorcht commented Jun 15, 2022 •
edited
Loading

@kaspar030 traced it down to differences in the debug information of the elf files - the hash of stripped binaries matches again. I was able to reproduce the issue with BUILD_IN_DOCKER=1.

The question is what could be the cause that the debug information is different, why this only happens in the RIOT docker and why this only happens with one app and one board? The generated code is the same, so Kconfig generates the same dependencies as the Makefiles. The compiler options are the same except for the TEST_KCONFIG variable. Nevertheless, each object file has exactly one additional .debug_macro which makes the difference.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 15, 2022
@gschorcht gschorcht mentioned this pull request Jun 16, 2022
Merged
@maribu maribu added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 16, 2022
@maribu
Copy link
Member

maribu commented Jun 16, 2022

The full Murdock run was recent enough and the only failure was a hash mismatch due to different .debug sections. No need to wast days of CPU times due to that, so I skip compile tests this time.

@gschorcht
Copy link
Contributor Author

The full Murdock run was recent enough and the only failure was a hash mismatch due to different .debug sections. No need to wast days of CPU times due to that, so I skip compile tests this time.

Yes, but we should still find out the reason. However, I myself have no idea how.

@maribu maribu merged commit 6db97e2 into RIOT-OS:master Jun 16, 2022
@gschorcht
Copy link
Contributor Author

@maribu Thanks for reviewing and merging

@gschorcht gschorcht deleted the cpu/esp32/fix_malloc_thread_safety branch June 20, 2022 18:30
@gschorcht gschorcht mentioned this pull request Jul 26, 2022
Merged
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.
4 participants
@gschorcht @maribu @chrysn @benpicco