gcc: Provide _sbrk implementation compatible with RTX

[adamgreen]

I verified that the hang issue I was seeing when building and running
the mbed official networking tests with GCC_ARM was related to this
issue reported on the mbed forums:
http://mbed.org/forum/mbed/topic/3803/?page=1#comment-18934

If you are using the 4.7 2013q1 update of GCC_ARM or newer then it
will have a _sbrk() implementation which checks the new top of heap
pointer against the current thread SP, stack pointer.
See this GCC_ARM related thread for more information:
https://answers.launchpad.net/gcc-arm-embedded/+question/218972
When using RTX RTOS threads, the thread's stack pointer can easily
point to an address which is below the current top of heap so this
check will incorrectly fail the allocation.

I have added a _sbrk() implementation to the mbed SDK which checks the
heap pointer against the MSP instead of the current thread SP. I have
only enabled this for TOOLCHAIN_GCC_ARM as this is the only GCC based
toolchain that I am sure requires this.

[bogdanm]

It might be a good idea to align the new_heap pointer, as it happens for example here:

https://github.com/mbedmicro/mbed/blob/master/libraries/mbed/targets/cmsis/TARGET_NXP/TARGET_LPC176X/TOOLCHAIN_GCC_CS/sys.cpp

Not sure if this is a hard requirement, but it seems to be a bit safer.

[adamgreen]

Good catch, I can add this alignment but I don't think it is required. Both newlib and newlib-nano contain code to 8 byte align the pointers that they return from their respective malloc() implementations (MALLOC_ALIGN for newlib-nano and MALLOC_ALIGNMENT for newlib). 8-byte aligning the addresses returned from _sbrk() doesn't necessarily align the pointer finally returned from malloc() since the heap code inserts chunk headers at the beginning of its allocations. That means that only malloc() really controls the final alignment of the pointer returned to the application.

I don't know if that Code Sourcery implementation is a good example to use anyway since I think it has a few flaws:

• The _sbrk() routine is expected to return -1 on error and not NULL.
• The _sbrk() routine should really set the global errno to ENOMEM on error. I do see code in newlib-nano's malloc() implementation which will set errno appropriately if it sees the -1 returned from _sbrk(). I didn't see the same in the standard newlib implementation.
• I think it would be best if _sbrk() just returns if the heap and stack would collide and not still advance the heap pointer past the stack pointer as seen in the CS routine.
• The unsigned int type for the incr parameter won't cause any bugs but it would imply the value will never have a negative value. This isn't true with the standard newlib which passes in negative values to free space back to the system.

As I am reading through this code, I do wonder if it would be better to perform an extra check at the beginning of _sbrk() to see if the stack pointer is already lower than the heap pointer and call abort() if it is since there is already likely to be stack/heap corruption and continuing will only worsen the problem?

[bogdanm]

You're right that malloc() should automatically align the pointer that it returns, adding some alignment code here might just make the code a bit safer. That said, I agree that this is a bit on the paranoid side of things, so we can leave the code as-is.
Regarding your idea to check _sbrk against the stack pointer, I believe it should work, but it must be tested, especially with the RTOS enabled. I didn't have a close look at our RTOS internals yet and I don't know how it would cope with such a change.

[adamgreen]

Ok, at this point I plan to leave this pull request as is.

[adamgreen]

Thanks!