GCC bug or FreeRTOS bug?

After Forever · ‎2017-09-21

Posted on September 21, 2017 at 13:19

I spent 3 hours debugging this. I'm using STM32L051 with FreeRTOS (initial code generated by STM32CubeMX v4.1). GCC version is the one that's coming with Ubuntu 04 by default:

~$ arm-none-eabi-gcc --version

arm-none-eabi-gcc (15:4.9.3+svn231177-1) 4.9.3 20150529 (prerelease) Copyright (C) 2014 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I'm using the -Os optimization flag. I can't turn off the optimization because the binary ends up being bigger than my flash.

I faced strange hard fault errors with some values of my tasks' stack sizes. After some debugging I realized that the top of the stack address of some of my tasks was invalid. Some more debugging and I found the offending part.

Look at this line of the function prvInitialiseNewTask in tasks.c of FreeRTOS' code where the pxTopOfStack variable corrupts:

 {
 pxTopOfStack = pxNewTCB->pxStack + ( ulStackDepth - ( uint32_t ) 1 );
 pxTopOfStack = ( StackType_t * ) ( ( ( portPOINTER_SIZE_TYPE ) pxTopOfStack ) & ( ~( ( portPOINTER_SIZE_TYPE ) portBYTE_ALIGNMENT_MASK ) ) ); /*lint !e923 MISRA exception. Avoiding casts between pointers and integers is not practical. Size differences accounted for using portPOINTER_SIZE_TYPE type. */
 /* Check the alignment of the calculated top of stack is correct. */
 configASSERT( ( ( ( portPOINTER_SIZE_TYPE ) pxTopOfStack & ( portPOINTER_SIZE_TYPE ) portBYTE_ALIGNMENT_MASK ) == 0UL ) );
 }�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?

And the disassembly (with my comments):

; ulStackDepth == r2 == 0x00000200
; r5 == 0x2000085c
 807 [1] pxTopOfStack = pxNewTCB->pxStack + ( ulStackDepth - ( uint32_t ) 1 );
0x80030d4 <+0x0018> 44 4e ldr r6, [pc, #272] ; (0x80031e8 <xTaskCreateStatic+300>) ; r6 == 0x3fffffff
 605 [1] pxNewTCB->pxStack = ( StackType_t * ) puxStackBuffer;
0x80030d6 <+0x001a> 25 63 str r5, [r4, #48] ; 0x30
 807 [2] pxTopOfStack = pxNewTCB->pxStack + ( ulStackDepth - ( uint32_t ) 1 );
0x80030d8 <+0x001c> 92 19 adds r2, r2, r6 ; r2 == 0x400001ff
0x80030da <+0x001e> 92 00 lsls r2, r2, #2 ; r2 == 0x000007fc
0x80030dc <+0x0020> aa 18 adds r2, r5, r2 ; r2 == 0x20001058
 808 [1] pxTopOfStack = ( StackType_t * ) ( ( ( portPOINTER_SIZE_TYPE ) pxTopOfStack ) & ( ~( ( portPOINTER_SIZE_TYPE ) portBYTE_ALIGNMENT_MASK ) ) ); /*lint !e923 MISRA exception. Avoiding casts between pointers and integers is not practical. Size differences accounted for using portPOINTER_SIZE_TYPE type. */
0x80030de <+0x0022> 07 25 movs r5, #7
0x80030e0 <+0x0024> aa 43 bics r2, r5
0x80030e2 <+0x0026> 01 92 str r2, [sp, #4]�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?

I added a cast to uint32_t and then back to a pointer in the first line and now it's fixed:

 {
 pxTopOfStack = ( StackType_t * ) ( ( uint32_t )pxNewTCB->pxStack + ( ulStackDepth - ( uint32_t ) 1 ) );
 pxTopOfStack = ( StackType_t * ) ( ( ( portPOINTER_SIZE_TYPE ) pxTopOfStack ) & ( ~( ( portPOINTER_SIZE_TYPE ) portBYTE_ALIGNMENT_MASK ) ) ); /*lint !e923 MISRA exception. Avoiding casts between pointers and integers is not practical. Size differences accounted for using portPOINTER_SIZE_TYPE type. */
/* Check the alignment of the calculated top of stack is correct. */
configASSERT( ( ( ( portPOINTER_SIZE_TYPE ) pxTopOfStack & ( portPOINTER_SIZE_TYPE ) portBYTE_ALIGNMENT_MASK ) == 0UL ) );
 }
�?�?�?�?�?�?�?

The disassembly:

; ulStackDepth == r2 == 0x00000200
; r5 == 0x2000085c
0x80030d4 <+0x0018> 01 3a subsr2, #1
 807 [1] pxTopOfStack = ( StackType_t * ) ( ( uint32_t )pxNewTCB->pxStack + ( ulStackDepth - ( uint32_t ) 1 ) );
0x80030d6 <+0x001a> aa 18 addsr2, r5, r2 ; r2 == 0x20000a5b
 605 [1] pxNewTCB->pxStack = ( StackType_t * ) puxStackBuffer;
0x80030d8 <+0x001c> 25 63 strr5, [r4, #48]; 0x30
 808 [1] pxTopOfStack = ( StackType_t * ) ( ( ( portPOINTER_SIZE_TYPE ) pxTopOfStack ) & ( ~( ( portPOINTER_SIZE_TYPE ) portBYTE_ALIGNMENT_MASK ) ) ); /*lint !e923 MISRA exception. Avoiding casts between pointers and integers is not practical. Size differences accounted for using portPOINTER_SIZE_TYPE type. */
0x80030da <+0x001e> 07 25 movsr5, #7
0x80030dc <+0x0020> aa 43 bicsr2, r5
0x80030de <+0x0022> 01 92 strr2, [sp, #4]
�?�?�?�?�?�?�?�?�?�?�?�?

So to sum up, pxNewTCB->pxStack is a pointer to uint32_t, casting it to uint32_t helped to fix the bug in the calculation process.

Is this a GCC bug or maybe mixing a pointer and uint32_t in an arithmetic expression is undefined behavior and then this is a FreeRTOS bug?

The funny thing is that after fixing this I'm facing another hard fault in another place. Gone debugging...

#gcc #freertos

Barry.Richard · ‎2017-09-21

Posted on September 21, 2017 at 21:13

And what is the type of those stack items?

StackType_t, which on the Cortex-M is unsigned 32-bit integer.

View solution in original post

Barry.Richard · ‎2017-09-21

Posted on September 21, 2017 at 15:14

I will try and replicate this and dig through the C books - in the mean time I notice you are not using the latest version of GCC, and the version you are using is marked as pre-release (i.e. not necessarily a stable release). Please try the same experiment with the latest stable GCC release (5.2?) and report back your findings. I will do likewise.

Barry.Richard · ‎2017-09-21

Posted on September 21, 2017 at 15:20

So the text I'm reading talks about adding and subtracting integers from pointers. The case above is using unsigned integers, which it doesn't say is not ok, but all the same also updating the original line to use in32_t casts instead of uint32_t.

waclawek.jan · ‎2017-09-21

Posted on September 21, 2017 at 16:55

This is basic C: the typecast removes the pointer arithmetics. I don't FreeRTOS but It appears to me that

ulStackDepth

is intended to be in bytes, in which case the pointer arithmetics was not appropriate at the first place.

JW

Barry.Richard · ‎2017-09-21

Posted on September 21, 2017 at 17:48

I just tried with the following compiler:

5.4.1 20160919 (release) [ARM/embedded-5-branch revision 240496]

and got the following output, which appears correct. I have annotated with comments within the code:

915 pxTopOfStack = pxNewTCB->pxStack + ( ulStackDepth - ( uint32_t ) 1 );

08002d30: ldr r3, [r4, #48] ; 0x30 // Sets r3 to 0x20000c48

08002d32: subs r5, #4 // Sets r5 to 4092 (as points to 4 byte types)

08002d34: add r5, r3 // Sets r3 to 0x20000c48. This is now the unaligned top of stack, 8-byte alignment is required.

919 pxTopOfStack = ( StackType_t * ) ( ( ( portPOINTER_SIZE_TYPE ) pxTopOfStack ) & ( ~( ( portPOINTER_SIZE_TYPE ) portBYTE_ALIGNMENT_MASK ) ) );

08002d40: bic.w r5, r5, #7 // Sets r5 from 0x20001c44 to 0x20001c40. This is then saved into the variable.

[edit] this was with -Os optimisation.

Barry.Richard · ‎2017-09-21

Posted on September 21, 2017 at 17:10

The pointer arithmetic is deliberate because the code is intended to run on all 40+ cores we support, some of which are 8-bit, some 16-bit, some 24-bit (ish), some 32-bit and at least one 64-bit. That is why there is a StackType_t definition.

Did you try a release version of GCC?

waclawek.jan · ‎2017-09-21

Posted on September 21, 2017 at 19:54

That is why there is a StackType_t definition.

Okay, but in

pxNewTCB->pxStack + ( ulStackDepth - ( uint32_t ) 1 );

it's the type of pxStack which governs the arithmetic - I don't know its definition but based on what was written above and the hungarian prefix I'd guess it's type is StackType_t* , rather than StackType_t . Is this what you intended?

------

08002d30: ldr r3, [r4, #48] ; 0x30 // Sets r3 to 0x20000c48 08002d32: subs r5, #4 // Sets r5 to 4092 (as points to 4 byte types) 08002d34: add r5, r3 // Sets r3 to 0x20000c This is now the unaligned top of stack, 8-byte alignment is required.

I don't get it. r3 is of course unaffected by the addition, why do you point that out?

Also, in OP,

 ulStackDepth == r2 == 0x00000200

while in your post (4092+4) / 4 = 0x400

What's the point of comparing apples to pears?

--------------

IMO the original code did what it was commanded to: it added (0x0200-1)*sizeof(pointertype) , as witnessed by the values in annotation.

; r5 == 0x2000085c

after addition

; r2 == 0x20001058

and if sizeof(

pxStack

) == 4 then 0x2000085c + 4 * (0x200 - 1) --> 0x20001058

JW

Barry.Richard · ‎2017-09-21

Posted on September 21, 2017 at 20:07

Again, have you tried this with a stable GCC release, rather than a

pre-release? We test with no optimisation and with full optimisation

and have not come across this issue before.

That is why there is a StackType_t definition.

Okay, but in

|pxNewTCB->pxStack + ( ulStackDepth - ( uint32_t ) 1 );|

it's the type of pxStack which governs the arithmetic

Yes, that is the point I was making. It has to be to be portable as the

pointer size is not always the same. Note that, as per the

documentation, ulStackDepth is specified as the number of stack items,

not the number of bytes.

- I don't know its

definition but based on what was written above and the hungarian prefix

I'd guess it's type is StackType_t* , rather than StackType_t . Is this

what you intended?

Yes, has forever been the case, and tested with 8, 16, 32 and 64 (plus

some oddballs in between) architectures with 20+ different compilers.

08002d30: ldr r3, ; 0x30 // Sets r3 to 0x20000c48

08002d32: subs r5, ♯ 4 // Sets r5 to 4092 (as points to 4 byte types)

08002d34: add r5, r3 // Sets r3 to 0x20000c48. This is now the unaligned top of stack, 8-byte alignment is required.

I don't get it. r3 is of course unaffected by the addition, why do you

point that out?

Because that is the value that gets aligned when the bottom bits get

cleared.

Also, in OP,

|ulStackDepth == r2 == 0x00000200|

while in your post (4092+4) / 4 = 0x400

What's the point of comparing apples to pears?

I don't understand that question. We have different cases because we

are passing in different sizes for the stack. That does not effect the

code generated by GCC - it does not know what is going to be passed in

when it is compiled.

IMO the original code did what it was commanded to: it added

(0x0200-1)*sizeof(/pointertype/) , as witnessed by the values in annotation.

|; r5 == 0x2000085c|

after addition

|; r2 == 0x20001058|

and if sizeof(|pxStack|) == 4 then 0x2000085c + 4 * (0x200 - 1) -->

0x20001058

I'm not sure if you are saying the original generated code was right or

wrong.

waclawek.jan · ‎2017-09-21

Posted on September 21, 2017 at 20:26

Note that, as per the documentation, ulStackDepth is specified as the number of stack items, not the number of bytes.

And what is the *type* of those *stack items*?

JW

Barry.Richard · ‎2017-09-21

Posted on September 21, 2017 at 21:13

And what is the type of those stack items?

StackType_t, which on the Cortex-M is unsigned 32-bit integer.