Skip to content

[C++] Use malloc/free instead of k_malloc/k_free in operator new/delete #20678

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

Merged
merged 2 commits into from
Nov 26, 2019

Conversation

vanwinkeljan
Copy link
Member

Use malloc/free instead of k_malloc/k_free in operator new/delete and extended test cases to cover new/delete.

Fixes: #18990

@vanwinkeljan vanwinkeljan requested a review from nashif as a code owner November 13, 2019 23:31
@zephyrbot zephyrbot added the area: Tests Issues related to a particular existing or missing test label Nov 13, 2019
@@ -1,5 +1,5 @@
tests:
misc.app_dev.cpp:
arch_exclude: posix
build_only: true
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this excluded?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test case to cover new/delete and this onde can run in CI, before this change the test case did not contained any runnable test and was only build.

Copy link
Contributor

Choose a reason for hiding this comment

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

right but why did you exclude qemu_x86_coverage

Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason qemu_x86_coverage hangs when running with c++.

Copy link
Contributor

Choose a reason for hiding this comment

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

need a bug filed on this, since this is an indicator that gathering code coverage will fail

Copy link
Member Author

Choose a reason for hiding this comment

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

created bug report #20729, looks like our own coverage code ends up in a endless loop

@aescolar
Copy link
Member

@vanwinkeljan What is the motivation for doing this change?

@vanwinkeljan
Copy link
Member Author

@aescolar please see issue #18990 for more background

@pabigot
Copy link
Collaborator

pabigot commented Nov 14, 2019

This isn't what I expected. Please check my understanding:

  • Currently ZEPHYR_CPLUSPLUS is defaulted off, and applies only when MINIMAL_LIBC is disabled and LIB_CPLUSPLUS is enabled. AFAICT ZEPHYR_CPLUSPLUS would be desirable only when using a libc that doesn't support C++ even though it provides a libc++. (?? I'm not sure how that would even work, but I think it was a path to retaining old behavior, so fine.) Its effect is to support new/delete and virtual functions.
  • Proposed ZEPHYR_NEW_DELETE is defaulted on when LIBCPLUSPLUS is disabled, requires MINIMAL_LIBC is disabled or supports malloc, and is allowed when LIB_CPLUSPLUS is enabled. It only overrides new/delete, and overriding virtual support is always done with LIB_CPLUSPLUS is disabled.

At this point I have no clue of how all the different C++ and libc Kconfig features interact. I'm particularly concerned here that it might become easy for somebody to replace new/delete in a newlib-based libc++ (which already implements them using malloc), which would be gratuitous and risky.

That mess pre-dates this PR, but I'm uncomfortable with changing it at this point. For the purposes of settling the basic need in #18990 to get rid of using kernel memory for C++ objects is it not sufficient to just replace the k_malloc(free) preprocessor-protected calls in cpp_new.cpp with unprotected malloc(free) calls? And leave the configuration infrastructure unchanged?

@vanwinkeljan
Copy link
Member Author

I'm particularly concerned here that it might become easy for somebody to replace new/delete in a newlib-based libc++ (which already implements them using malloc), which would be gratuitous and risky.

We could just drop the option of using the zephyr version of new/delete if a libc++ is used (it uses malloc internally). Actually this is maybe the most correct and clean way forward and actually the Kconfig could be dropped in this way.

Shall I update the PR to get a feeling how it would look like?

@vanwinkeljan
Copy link
Member Author

@pabigot @andrewboie Another surprise is the failing CI, it seems that virtual destructors are calling operator delete. With the current proposal this requires a libc heap.

@andrewboie
Copy link
Contributor

We could just drop the option of using the zephyr version of new/delete if a libc++ is used (it uses malloc internally).

Yes, why would we not want to do this?

Maybe this is historical cruft but we should not be providing our own versions of these functions if the C library already provides them!

Use malloc/free instead of k_malloc/k_free in operator new/delete
implementation or use libstdc++ implementation when available.

Further updated cpp_synchronization sample to enable minimal libc heap
as virtual destructor requires operator delete which depends on free.

Signed-off-by: Jan Van Winkel <[email protected]>
Extended c++ test case to cover the zephyr implementation of new and
delete.

Signed-off-by: Jan Van Winkel <[email protected]>
@vanwinkeljan
Copy link
Member Author

@dleach02 do you want this PR in 2.1? It has a medium prio bug associated.

@dleach02
Copy link
Member

@vanwinkeljan, yes we should merge this to clear the medium bug.

@vanwinkeljan vanwinkeljan added this to the v2.1.0 milestone Nov 26, 2019
@dleach02 dleach02 merged commit 1182956 into zephyrproject-rtos:master Nov 26, 2019
@vanwinkeljan vanwinkeljan deleted the cpp_new_malloc branch November 27, 2019 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: C++ area: Samples Samples area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C++ New allocates memory from kernel heap
6 participants