Fix stack aligment.

[perkristian]

Stack should be 8 byte aligned on ARM.
Fix the automatic correction of the alignment in rt_init_stack,
and make sure that all stacks are aligned by the compiler.

[perkristian]

I have been experimenting with mbed-rtos on a LPC1768 chip and have had problems getting anything to run at all. I would get HardFault crashes on bootup. Even just compiling the rtos_basic minimal example in the online compiler would crash.

I finally had some time to look more closely into this. The first thing I noticed was that when compiling in debug mode the system would boot up. So it seems that unoptimized code runs fine. Then I noticed that rt_init_stack (HAL_CM.c) was passed stack pointers that were not 8 byte aligned. There is code there that is supposed to align the stack, but it is not correct. I fixed the alignment code and added alignment attributes to other stacks I could find.

I can now successfully boot rtos_basic and see the leds flashing.

[adamgreen]

If the size variable ends up being odd then won't the stack pointer become unaligned after your change? The old code used to 8-byte align the top of the stack, which is what matters since that is what SP will actually be initialized to and will then grow downwards to the bottom.

[perkristian]

Yes, maybe it would also be appropriate to protect agains odd stack sizes. Should absolutely everything be allowed, or should there be some expectation on what is reasonable?

[0xc0170]

Isn't an attribute non generic, gcc specific ?
I haven't seen any reports, neither experienced any errors with rtx in this regard, thus I am sceptical about this PR.

How to reproduce the problem? Rtos basic example for lpc1768, built via tools scripts will show this behaviour?

[perkristian]

I was very surprised to see this problem. As I said, even rtos_basic with the online compiler will crash on my device which is a mbed LPC1768. Hence I did a local build for debugging with openocd/gdb. I have compiled with both gcc-arm-none-eabi-4_7-2012q4 and gcc-arm-none-eabi-4_9-2014q4 with the same result.

[perkristian]

There are some generic alignment macros in libraries/net/lwip/lwip-sys/arch/cc.h.
Any suggestion for a common file where this would fit?

[adamgreen]

I can't reproduce any problems on my mbed LPC1768 with the rtos_basic sample when using GCC_ARM or the mbed online compiler. I imported the rtos_basic sample from http://developer.mbed.org/teams/mbed/code/rtos_basic/ for my testing.

I suspect that you are hitting some other issue that needs to be further investigated. The only thing that I could see really going wrong on the online compiler side is maybe somehow picking up an old version of the sample that had some other issue or picking a target other than LPC1768 when performing the build.

Your stack alignment change in rt_init_stack() looks wrong to me as it now makes no attempt to align the top of the stack which is what the task's stack pointer is actually set to. There is no real reason to align the bottom of the stack.

[perkristian]

The odd stack happens with osTimerThread. In my build the stack as allocated by the linker is odd. The array is of type unsigned char, so I assume it is appropriate for the linker to give it an odd start
address. I have seen the problem come and go, so I am sure you can get lucky, which is probably what happens with the online compiler builds.
In my build:
(gdb) print &os_thread_def_stack_osTimerThread
$20 = (unsigned char (*)[2048]) 0x1000029d

So when rt_init_stack() runs
p_TCB->stack is 0x1000029d.
p_TCB->priv_stack is 2048,
size is set to 512.
And by simple arithmetic stk becomes 0x10000a9d

Then the test ((U32)stk & 0x04) is true, but this does not help much since stk-- sets stk to 0x10000a99 which is still odd.

So my conclusion is that the code expects the stack to be 4 byte aligned and only tries to adjust the stack to be 8 byte aligned in that case. If the stack passed is completely unaligned the alignment
code is not sufficient.

[adamgreen]

Thanks for the extra information. That makes things a lot clearer.

As you say, the underlying problem is that rt_init_stack() expects p_TCB->stack to be 4-byte aligned. It definitely seems like the current osThreadDef() macro definition breaks that assumption.

A few more comments:

• I think the correct way to round a value up to the closest 8-byte aligned value is (address + 7) & ~7. Your current code (address & ~7) + 8 will even round-up when the address is already 8-byte aligned.
• The bottom of the stack only needs to be 4-byte aligned. This makes sure that the 4-byte MAGIC_WORD check is aligned properly and that the existing 8-byte alignment code for the top of the stack works correctly.
• The smaller change to fix the issue that you are encountering is to leave everything as is except for the
  osThreadDef() macro. Just 4-byte align the stack that it allocates. A more compiler agnostic approach may be to allocate the stack as an array of 32-bit values.

diff --git a/libraries/rtos/rtx/TARGET_CORTEX_M/cmsis_os.h b/libraries/rtos/rtx/TARGET_CORTEX_M/cmsis_os.h
index f19a723..da63205 100644
--- a/libraries/rtos/rtx/TARGET_CORTEX_M/cmsis_os.h
+++ b/libraries/rtos/rtx/TARGET_CORTEX_M/cmsis_os.h
@@ -335,9 +335,9 @@ int32_t osKernelRunning(void);
 extern osThreadDef_t os_thread_def_##name
 #else                            // define the object
 #define osThreadDef(name, priority, stacksz)  \
-unsigned char os_thread_def_stack_##name [stacksz]; \
+unsigned int os_thread_def_stack_##name [stacksz / sizeof(unsigned int)]; \
 osThreadDef_t os_thread_def_##name = \
-{ (name), (priority), (stacksz), (os_thread_def_stack_##name)}
+{ (name), (priority), (stacksz), (unsigned char*)(os_thread_def_stack_##name)}
 #endif

 /// Access a Thread definition.

[0xc0170]

@perkristian ,what do you think above Adam's comments above?

[perkristian]

I agree with Adam's comments.

I have pushed a new commit which changes the relevant interfaces to be uint32 pointers. There is still the issue of the Thread interface, but I assume that cannot be changed since it is external.

[0xc0170]

What issue in the interface? That Thread ctor defines stack parameter as pointer to unsigned char?

[perkristian]

Yes, any stack parameters pointing to unsigned char may be unaligned and we the crashes will be random depending on the build.

[0xc0170]

The similar check you proposed in the beginning, can be applied in the ctor there, to align the stack pointer if it's not NULL.

[adamgreen]

I like @perkristian current proposal.

@0xc0170 I wonder how many people would actually be impacted if the constructor was changed to take a uint32_t* instead. I suspect most people just pass in NULL. I would think that the compiler error would be pretty clear as to what they need to change if it does impact them.

[0xc0170]

Thanks guys. I'll merge this.