[TOOLCHAIN_GCC_ARM] Using __get_MSP() in _sbrk()

[betzw]

@0xc0170 @bcostm

Yesterday I was giving a closer look to file libraries/mbed/common/retarget.cpp and especiallaly to the lines from line 474 to line 492 where function __sbrk() gets defined for TOOLCHAIN_GCC_ARM, which looks like this:

// Dynamic memory allocation related syscall.
extern "C" caddr_t _sbrk(int incr) {
    static unsigned char* heap = (unsigned char*)&__end__;
    unsigned char*        prev_heap = heap;
    unsigned char*        new_heap = heap + incr;
https://github.com/mbedmicro/mbed/blob/master/libraries/mbed/common/retarget.cpp#L482
#if defined(TARGET_ARM7)
    if (new_heap >= stack_ptr) {
#elif defined(TARGET_CORTEX_A)
    if (new_heap >= (unsigned char*)&__HeapLimit) {     /* __HeapLimit is end of heap section */
#else
    if (new_heap >= (unsigned char*)__get_MSP()) {
#endif
        errno = ENOMEM;
        return (caddr_t)-1;
    }

    heap = new_heap;
    return (caddr_t) prev_heap;
}

My point here refers to line 484 where the availability of further heap space is checked by comparing the new_heap end with the position of the current stack pointer (__get_MSP()).
In my eyes, this is not a very secure/good way to check the availability of further heap space as in case at the moment of the call we are in the situation of low stack usage the check might pass and the memory added to the heap, while further on the stack might grow beyond its current value (i.e. as we are growing down __get_MSP() might return a minor value) and such invade the already allocated heap space ...
Furthermore, even if less important, the usage of __get_MSP() as heap limit implies a certain memory layout in which the heap and stack must be placed in the same memory bank and where the heap must be located immediately before the stack region, which on some platforms (e.g. STM32L476RG) limits the possible choices.

A solution to this issue might be to implement __sbrk() as it is done for TARGET_CORTEX_A (see line 482) also in case of a Cortex-M, i.e. using __HeapLimit to delimit the heap space. But this requires a review of all linker scripts for the GNU toolchain for most likely all platforms.

What do you think?

[bcostm]

Thanks for this analysis. I don't know why today we can't use the __HeapLimit as done on the Cortex_A? I am not an expert of this. Maybe ARM can help ? If a fix is found for one or two platforms, for example the F401 and L476, we can help afterwards to deploy this change to all other ST platforms.

[0xc0170]

cc @adamgreen

[adamgreen]

The current version is probably done for maximum flexibility since it doesn't require the user to figure out beforehand how much memory they want to dedicate to the stack and heap. The obvious cost of this flexibility is that the stack can grow downward and collide with the heap and it requires the heap and stack grow towards each other in the same area of memory.

Having a HeapLimit forces the user to figure out beforehand how big they want their stack and heap to be. What does the user have to modify to grow the heap/stack limits in your plan? The linker script?

For most platforms just adding a HeapLimit doesn't really solve anything anyway since it only limits heap growth and does nothing to stop the stack from growing down into the heap. To really make it work, you would want to pre-allocate space for the stack as well and place it somewhere in memory where it could never grow down into the heap or anything else.

The best solution might be to pull sbrk() out of retarget.cpp and place it in its own sbrk.o object file so that it can be replaced completely by any platform that wants to use a different memory model. Could also have a DEVCE_* macro in device.h which can disable the inclusion of the one in retarget.cpp for a particular platform.

@0xc0170 What does the online compiler do?

[betzw]

Well, the HeapLimit would also lower the probability that the heap gets corrupted by the stack as its growth will be limited. Maybe not by chance in mbedOS the memory layout has been changed to have the stack being the very first region (assuming that the vector tables will also be moved further up in future)!?!

[adamgreen]

Yes, if the HeapLimit is quite small but if you make that the default then people will start complaining about their programs now having allocation failures when currently they don't. I don't like setting HeapLimits because I don't know what they should be. Each project will have a different split between stack and heap usage.

I do agree that allowing some flexibility in placing the heap in a different (or even across several) bank(s) might be nice on some platforms.

[betzw]

No doubt about that limiting the size of the heap might create problems for some applications, as for other applications the current approach might not work out at all (e.g. when allocating a lot of memory initially, which then gets partially freed, before entering computations which use recursive functions) ...
Not sure, if there is a "right" solution for mbed-classic. I believe it is a trade-off which has to take into account all the design goals of mbed-classic and their respective priorities.
As far as I understand, e.g. in mbed-os you need to specify only a stack limit and the heap will catch all the rest of the memory which has been left by the linker, which might be a good compromise.
(Also in mbed-os there is no support for multiple non-consecutive memory banks.)

[adamgreen]

If you set HeapLimit too low, the user will get alloc failures when there is still memory available. If you set it too high then they are in the same situation as today where a recursive function could collide with the heap. The correct value depends on how much stack the developer needs.

Putting a defined stack at the bottom of valid RAM is one way to catch such stack overflows. Another is to use the MPU (on CPUs which have one) to place a guard block of 32-bytes between the stack and other areas of memory so that a stack overflow generates a memory fault. But that is something that the developer should do themselves as they might have other uses for the MPU where they would need to add this into the mix.

[betzw]

Btw, as far as I know the online compiler uses limits at least with respect to the heap.
@0xc0170 might confirm.

[adamgreen]

At least for the LPC1768 target that doesn't appear to be the case:
https://github.com/mbedmicro/mbed/blob/master/libraries/mbed/targets/cmsis/TARGET_NXP/TARGET_LPC176X/TOOLCHAIN_ARM_STD/sys.cpp
https://github.com/mbedmicro/mbed/blob/master/libraries/mbed/targets/cmsis/TARGET_NXP/TARGET_LPC176X/TOOLCHAIN_ARM_MICRO/sys.cpp

[betzw]

Well, as far as I know at least in TOOLCHAIN_ARM_MICRO __user_setup_stackheap() gets never called and therefore is removed from the binary by the linker.
Anyway, I have the impression that there is no real general guideline regarding the memory layout in mbed-classic. Pls. correct me if I am wrong!

cc @nikapov-ST

[sg-]

The right solution for the mbed SDK has been letting the stack and heap collide. https://developer.mbed.org/handbook/Memory-Model

[0xc0170]

The right solution for the mbed SDK has been letting the stack and heap collide. https://developer.mbed.org/handbook/Memory-Model

👍

[betzw]

@siorpaes

[betzw]

So my question now becomes: considering that the future of mbed should be mbed-os, is it really worthwhile to align all mbed-classic platforms a/o tool-sets which do currently not implement this memory model to become compliant with it (e.g. #1554)?

[sg-]

Yes - All mbed Classic platforms should implement this memory model. https://developer.mbed.org/handbook/Memory-Model

We need to create a test for this. Anyone interested??