C++ New allocates memory from kernel heap

[vanwinkeljan]

Describe the bug

The current Zephyr C++ new implementation allocates memory from the kernel heap instead of using the same heap used by malloc.

In case a system would be configured with none, or a small, kernel heap and a large libc heap the call to new will fail even when sufficient memory is available in the libc heap.

To Reproduce
Steps to reproduce the behavior:

• Problem was hit on private project.
• Todo: reproduce with libcxx tests

Expected behavior

new/delete should use same memory pool as malloc/free

Impact

None useable system due to memory allocation failure.

Environment

• OS: Linux
• Toolchain: Zephyr
• f9c94d4 (2.0-rc3)

Additional context

PR (#18464) was created to provide the option to disable Zephyr new/free implementation and falback to libcxx implementation which uses malloc and free internally.

One open question from the code review on PR #18464 is why zephyr provides its own implementation of new/free that specifically depends on kernel heap

[vanwinkeljan]

I'm not sure this is the right way to solve that

I agree here, a solution would be to use malloc/free instead of k_malloc/k_free so that at least the memory is acquired from the same pool for both new and malloc.

The next step could be to make the behaviour of malloc/free configurable to use one of the following scenarios:

1. kernel heap (k_malloc/k_free)
2. dedicated memory pool in kernel space
3. dedicated memory pool in user space
4. libc heap

Originally posted by @vanwinkeljan in #18464 (comment)

[pabigot]

Actually my hope was that Zephyr doesn't need to override new and delete at all. The default implementation from libstdc++, by inspection, uses malloc.

I've confirmed that simply removing cpp_new works fine with #18991 when newlib is used. We know you need a Zephyr malloc heap for things like std::vector. Turn's out that probably because Zephyr isn't providing an implementation of operator new[] so we're getting the one from libc++. I don't like mixing providers, and I don't like the idea of replacing API that's not clearly documented as something that can be replaced (as _sbrk is, that being the supplier for malloc()). We have no idea what sort of internal hackery might be affected by replacing pieces, nor reason to believe we can find all the pieces that have to be replaced (since we didn't.)

So I'd rather that for non minimal libc configurations Zephyr defer to the libstdc++ provided implementation and exclude cpp_new.

If we go that way the problem disappears for newlib. It's still there for minimal libc, where it looks like operator new/delete aren't found if cpp_new is not built, probably because they come from libstdc++ and CONFIG_LIB_CPLUSPLUS is incompatible with minimal libc.

A case could be made that the right answer there is "yep, too bad" and we leave things as they are, which is half-implemented. As I noted earlier somewhere, if you're doing dynamic memory allocation with C++ you probably won't be using minimal libc.

Alternatively you could add configuration of the back-end use in the Zephyr implementation of operator new to the options in your list. I don't think you'd get much traction on changing how malloc() works, and you probably shouldn't: it's too core.

Thoughts?

[vanwinkeljan]

Actually my hope was that Zephyr doesn't need to override new and delete at all. The default implementation from libstdc++, by inspection, uses malloc.

I'm also wondering why zephyr is providing its own implementation as I came to the same conclusion that libc++ is using malloc. One explanation could be the overhead of the exception handling code that gets pulled in.

So I'd rather that for non minimal libc configurations Zephyr defer to the libstdc++ provided implementation and exclude cpp_new.

Agree and that is what I tried to achieve with the PR; with the exception that the behaviour could be changed via Kconfig, you never know if somebody wants it the other way around.

It's still there for minimal libc, where it looks like operator new/delete aren't found if cpp_new is not built

For this case I would rather change the behaviour of cpp_new from k_malloc to malloc as malloc and new are in the same family. Now even for minimal libc they go to different memory pools.

Thoughts?

Use implementations of libc++ if the library is enabled and not only for new and it variants but also __cxa_pure_virtual and the type info in the vtables.

Further cleanup cpp_new to use malloc so at least malloc and new come from the same pool (for the minimal libc case)

[pabigot]

@vanwinkeljan Can this be closed now that #18464 has been merged? Or do you feel there's more work to be done (and if so, is it still medium priority)?

[andrewboie]

Hi,

I'm also wondering why zephyr is providing its own implementation as I came to the same conclusion that libc++ is using malloc. One explanation could be the overhead of the exception handling code that gets pulled in.

I think all this stuff should simply be malloc/free based. I don't think there's any strong need to preserve the status quo.

If we have everything derived by malloc(), then we have freedom to implement the malloc heap in a way that makes sense for that C library. minimal libc currently does it with a sys_mem_pool(), newlib has its own logic which derives from a memory region implemented in lib/libc/newlib/libc-hooks.c

kernel heap (k_malloc/k_free)
dedicated memory pool in kernel space

Neither of these are appropriate, they won't work with user mode.
We should keep this simple and just derive everything from malloc(), the implementations of malloc for both newlib and minimal libc are user-mode friendly, and we can make any extensions there

[vanwinkeljan]

@pabigot question is if we want to keep this one open to update zephyrs new implementation to use malloc instead of k_malloc.

[andrewboie]

@pabigot question is if we want to keep this one open to update zephyrs new implementation to use malloc instead of k_malloc.

It should not be using k_malloc(), such memory is never accessible to user mode

[vanwinkeljan]

@andrewboie problem is that minimum libc can be build without heap support (and as such without malloc/free) and I have the feeling this is the reason why k_malloc is used. Of Course it is an option to put a restriction in place that a libc with heap is required for c++.

[andrewboie]

I have the feeling this is the reason why k_malloc is used

No, that's not why this is the case.
Please, derive all this C++ memory allocation stuff from malloc()/free()
If you don't have a malloc() heap, you don't get a C++ heap either.

[pabigot]

The k_malloc stuff came in through #10792; I can't tell why that was used except perhaps that (a) looked like CONFIG_HEAP_MEM_POOL_SIZE would be the appropriate place to get memory from the heap, and (b) at the time the PR was started the C++ allocation functions were inline function defined in kernel.h.

I have no problem with this being converted to malloc/free.

@vanwinkeljan This was assigned to me and it's a medium priority bug two weeks before release, so it needs to be worked. Please let me know if you are working or intend to work on it, otherwise I'll knock it out in the next day or two.