EABI Stack Alignment on ARM7

system · October 28, 2010, 12:44pm

johandc wrote on Thursday, October 28, 2010:

According to the new ARM Architecture Procedure Call Standard (AAPCS, 2.08, 16 october 2009) the stack pointer must be aligned to an 8 byte boundary.

In order to achieve this, it seems that FreeRTOS has a define called: portBYTE_ALIGNMENT. FreeRTOS then has a mechanism inside heap_1.c and heap_2.c that ensures that they return blocks aligned according to portBYTE_ALIGNMENT. However there is no guarantee that heap_3.c will return an 8 byte aligned block, but experience with newlib has shown that it does. So far so good. But…

Even though the stack area is allocated by malloc to a correct 8 byte address, FreeRTOS modifies the stack pointer in task.c:

pxTopOfStack = pxNewTCB->pxStack + ( usStackDepth - 1 );

Subtracting 1 from the pointer now makes it aligned to only 4 bytes. And this breaks floating point compatibility with all the latest GCC compilers using the EABI. (Such as Codesourcery G++ and Yagarto).

The fix can be done either by subtracting 2 from the pointer in task.c, or as it was done for the Cortex-M3 port between version 6.0.0 to 6.0.1, an offset can be added in portMacro.h:

portSTACK_TYPE *pxPortInitialiseStack( portSTACK_TYPE *pxTopOfStack, pdTASK_CODE pxCode, void *pvParameters ) {
	pxTopOfStack--;
        ....
   }

This has the same effect, and it works for Cortex-M3 and also ARM7 ports. Note: All of the official ARM7 ports in /portable/gcc/arm7* are broken using the new EABI compiler and needs this fix. So far Cortex-M3 and PIC32 seems to be the only ones that are 8 byte aligned!

As shown above, also the usStackDepth is affecting the alignment of a pointer. This means that the stack depth must always be dividable with 2 in order for 8 byte alignment to work. A more secure solution would be to check the portBYTE_ALIGNMENT at runtime and subtract from the pointer until it is properly aligned.

while ((portSTACK_TYPE) pxTopOfStack % 8)
		pxTopOfStack--;

This could be done in portmacro.h, but would require all ports to be modified, or the change could be made in task.c so it will always adhere to the portBYTE_ALIGNMENT before calling pxPortInitialiseStack…

rtel · October 28, 2010, 1:43pm

rtel wrote on Thursday, October 28, 2010:

This is quite a long post - but in summary I think you are probably trying to fix a problem that does not exist - but I’m happy to be proven wrong….

n order to achieve this, it seems that FreeRTOS has a define called: portBYTE_ALIGNMENT. FreeRTOS then has a mechanism inside heap_1.c and heap_2.c that ensures that they return blocks aligned according to portBYTE_ALIGNMENT. However there is no guarantee that heap_3.c will return an 8 byte aligned block, but experience with newlib has shown that it does. So far so good. But…

Relying on this alone does not guarantee the correct alignment because the alignment requirement is greater than sizeof( portSTACK_TYPE) .

Subtracting 1 from the pointer now makes it aligned to only 4 bytes. And this breaks floating point compatibility with all the latest GCC compilers using the EABI. (Such as Codesourcery G++ and Yagarto).

If you have a correctly aligned buffer to start with, then add an even number of sizeof( portSTACK_TYPE ) bytes (usStackSize is an even number), you will end up on a four byte boundary once the 1 has been subtracted. On the other hand, if you start with a correctly aligned buffer to start with, then add an odd number of sizeof( portSTACK_TYPE ) bytes (usStackSize is an odd number) you will end up on an eight byte boundary once the 1 has been subtracted.

The fix can be done either by subtracting 2 from the pointer in task.c

The situation you describe cannot be fixed by doing this, you just switch around whether it works with an odd or even usStackSize.

or as it was done for the Cortex-M3 port between version 6.0.0 to 6.0.1, an offset can be added in portMacro.h

Fell into that trap myself once - the fix for the cortex and the ARM7 are unfortunately not the same - the processors behave differently regarding the stack pointer on start up.

As shown above, also the usStackDepth is affecting the alignment of a pointer.

Exactly, hence what you do above that (how the buffer is aligned to start with) does not make that much difference.

This means that the stack depth must always be dividable with 2 in order for 8 byte alignment to work. A more secure solution would be to check the portBYTE_ALIGNMENT at runtime and subtract from the pointer until it is properly aligned.

while ((portSTACK_TYPE) pxTopOfStack % 8)
pxTopOfStack-;

My reasoning is that the line:

pxTopOfStack = ( portSTACK_TYPE * ) ( ( ( unsigned long ) pxTopOfStack ) & ( ( unsigned long ) ~portBYTE_ALIGNMENT_MASK  ) );

Corrects the alignment (assuming the stack grows down) once all the calculations have been complete - hence I don’t know why you would be seeing a problem. Are you seeing a problem?

The problem with the alignment correction code at the moment is that where sizeof( portSTACK_TYPE * ) is less than sizeof( unsigned long) by rights the compiler should output a warning. Remember this code has to work on all combinations of sizeof( portSTACK_TYPE * ) , big endian, little endian, 8 bit, 16 bit and 32 bit devices. Its quite tricky.

Regards.

system · October 28, 2010, 2:44pm

johandc wrote on Thursday, October 28, 2010:

I didn’t see that you fixed this issue already. The line you are suggesting is not in our older ver. 5.3.0. We have diffed all the portable files, and didn’t see any changes from our version to trunk. And didn’t actually foresee that you made a change in tasks.c, so i missed it. Maybe it was time that we rolled in a newer version.

I’m glad we come to the same conclusion. Of course the solution you state is much better suited to go in tasks.c because it allows for a different alignment mask based on architecture.

Thank you for using your time in explaining to us why i’ts important to stay up to date

system · November 2, 2010, 9:37am

johandc wrote on Tuesday, November 02, 2010:

In fact, the solution is not quite there yet:

pxTopOfStack = ( portSTACK_TYPE * ) ( ( ( unsigned long ) pxTopOfStack ) & ( ( unsigned long ) ~portBYTE_ALIGNMENT_MASK ) );

Gives a “Cast from pointer to integer of different size” on AVR8. Here a pointer is a 16 bit value, which is an int on avr8, and not a long. Would it make sense to add a new type called a portPOINTER_TYPE ?

In our case setting the cast to (unsigned int) instead of (unsigned long) fixes our issue, since we run only on ARM7 (where long = int = 32bit) and on AVR8 (where int = 16 bit).

rtel · November 2, 2010, 10:07am

rtel wrote on Tuesday, November 02, 2010:

In fact, the solution is not quite there yet:
<snip>
Gives a “Cast from pointer to integer of different size” on AVR8. Here a pointer is a 16 bit value, which is an int on avr8, and not a long.

…which is exactly what I meant by my comment in the previous post

The problem with the alignment correction code at the moment is that where sizeof( portSTACK_TYPE * ) is less than sizeof( unsigned long) by rights the compiler should output a warning. Remember this code has to work on all combinations of sizeof( portSTACK_TYPE * ) , big endian, little endian, 8 bit, 16 bit and 32 bit devices. Its quite tricky.

Would it make sense to add a new type called a portPOINTER_TYPE

Yes it would and that has been given some consideration. The problem is adding this type into the kernel would require every single port to be retested.

Regards.

system · April 11, 2011, 9:28am

anonymous wrote on Monday, April 11, 2011:

Unfortunately FreeRTOS V7.0.0 still does not fix this issue. Are there any plans to do so in future versions?

I am using this fix, but I am not sure if it works with any compiler:

pxTopOfStack = ( portSTACK_TYPE * ) ( ( ( uintptr_t ) pxTopOfStack ) & ( ( uintptr_t ) ~portBYTE_ALIGNMENT_MASK  ) );

Regards,
Thorsten Krohn

rtel · April 11, 2011, 9:38am

rtel wrote on Monday, April 11, 2011:

I am not aware of a problem that needs to be fixed. The alignment is checked when the buffer is returned from pvPortMalloc(), and after a pointer to the top of the stack is calculated prior to the stack being loaded with initial register values.

What is the problem you are referring to? Considering the solution has to work on big endian, little endian, CPUs, and … 8 bit, 16 bit, 20 bit, 24 bit and 32 bit architectures, and any combination thereof…

Regards.

system · April 11, 2011, 9:52am

anonymous wrote on Monday, April 11, 2011:

The problem is that this line of code in task.c (line 470, V7.0.0)

pxTopOfStack = ( portSTACK_TYPE * ) ( ( ( unsigned long ) pxTopOfStack ) & ( ( unsigned long ) ~portBYTE_ALIGNMENT_MASK  ) );

produces compiler warnings, if the used port has smaller pointers than (unsigned long). E.g. WinAVR uses 16 bit pointers. This results in two warnings:

tasks.c: In function 'xTaskGenericCreate':
tasks.c:470: warning: cast from pointer to integer of different size
tasks.c:470: warning: cast to pointer from integer of different size

As suggested previously, introducing portPOINTER_TYPE instead of uintptr_t could fix this, too.

Regards.

system · August 9, 2011, 12:22pm

johandc wrote on Tuesday, August 09, 2011:

Thanks for the fix thorsten, I can verify that this works with both AVR8, ARM and AVR32.

To Richard: There _IS_ a problem that needs to be fixed here.

Furthermore this also goes for the three other locations where portBYTE_ALIGNMENT_MASK is used. Specifically the assertions.