Should xPendingReadyList be volatile?

xelus wrote on Friday, February 08, 2019:

Hello,
I am using FreeRTOS 0.9.0 and MSP430X_MSP430FR5969 port, and I am compiling it with TI 18.1 compiler.
I have found error, which causes device to hangs out. The error occurs when at least two task are inserted to xPendingReadyList in interrupts when scheduler is suspended.
When xTaskResumeAll function is call the microntroller only once evaluate listGET_OWNER_OF_HEAD_ENTRY macro value (before while loop), but this value is changed during moving list item to task ready list.
If I change the definition xPendingReadyList on volatile it works correctly

#define listGET_OWNER_OF_HEAD_ENTRY( pxList )  ( (&( ( pxList )->xListEnd ))->pxNext->pvOwner )

while( listLIST_IS_EMPTY( &xPendingReadyList ) == pdFALSE )
{
    pxTCB = ( TCB_t * ) listGET_OWNER_OF_HEAD_ENTRY( ( &xPendingReadyList ) );
    ( void ) uxListRemove( &( pxTCB->xEventListItem ) );
    ( void ) uxListRemove( &( pxTCB->xStateListItem ) );
    prvAddTaskToReadyList( pxTCB );

    /* If the moved task has a priority higher than the current
    task then a yield must be performed. */
    if( pxTCB->uxPriority >= pxCurrentTCB->uxPriority )
    {
        xYieldPending = pdTRUE;
    }
    else
    {
        mtCOVERAGE_TEST_MARKER();
    }
}

Should xPendingReadyList be volatile or it is compiler error?

rtel wrote on Friday, February 08, 2019:

Can you please try adding the following line to FreeRTOSConfig.h, then
report back as to whether it solves the problem for you:

#define configLIST_VOLATILE volatile

xelus wrote on Friday, February 08, 2019:

Thanks for tip, I was looking for such an option and somehow missed it. Unfortunatelly, adding volatile didn’t help, and even got worse. Now device hangs after first tick, because of error in:
vTaskSwitchContext —> taskSELECT_HIGHEST_PRIORITY_TASK —> listGET_OWNER_OF_NEXT_ENTRY

#define listGET_OWNER_OF_NEXT_ENTRY( pxTCB, pxList )\
{\
List_t * const pxConstList = ( pxList );\
	/* Increment the index to the next item and return the item, ensuring */\
	/* we don't return the marker used at the end of the list.  */\
	( pxConstList )->pxIndex = ( pxConstList )->pxIndex->pxNext;\
	if( ( void * ) ( pxConstList )->pxIndex == ( void * ) &( ( pxConstList )->xListEnd ) )\
	{\
		( pxConstList )->pxIndex = ( pxConstList )->pxIndex->pxNext;\
	}\
	( pxTCB ) = ( pxConstList )->pxIndex->pvOwner;\
}

In first line, pxIndex is overwritten by pxIndex->pxNext value. Next the address comparision is made, and at this point compiler gets new value of pxIndex. Because condition is met pxIndex is again overwritten by pxIndex->pxNext value, but this is the same value as it is in first line.

Asm code when volatile is enabled:

R14 is pxConstList
01f582:   0E3C 0002           MOVA    0x0002(R14),R12 ;R12 = ( pxConstList )->pxIndex
01f586:   1800 4CDE 0004 0002 MOVX.A  0x00004(R12),0x00002(R14) ;now pxIndex is updated
01f58e:   0ECF                MOVA    R14,R15
01f590:   00AF 0006           ADDA    #0x00006,R15
01f594:   1800 9FCE 0002      CMPX.A  R15,0x00002(R14)	;we using current ( pxConstList )->pxIndex value 
01f59a:   2004                JNE     (0xf5a4)
01f59c:   1800 4CDE 0004 0002 MOVX.A  0x00004(R12),0x00002(R14) ; at this point compiler are using old R12 value 
        $C$L2869:
01f5a4:   0E3F 0002           MOVA    0x0002(R14),R15
01f5a8:   1800 4FD2 000C 3AE4 MOVX.A  0x0000c(R15),&pxCurrentTCB
01f5b0:   4D82 3AEE           MOV.W   R13,&uxTopReadyPriority
01f5b4:   0110                RETA    

rtel wrote on Saturday, February 09, 2019:

Ok - I need to get a better understanding of the issue you are facing.
I will go back to your original post:

I am using FreeRTOS 0.9.0 and MSP430X_MSP430FR5969 port

I presume you mean FreeRTOS V9.0.0. Why use an old version?

and I am compiling it with TI 18.1 compiler.

Is that the latest compiler version?

I have found error, which causes device to hangs out. The error
occurs when at least two task are inserted to xPendingReadyList in
interrupts when scheduler is suspended. When xTaskResumeAll function is
call the microntroller only once evaluate listGET_OWNER_OF_HEAD_ENTRY
macro value (before while loop),

This bit I don’t understand. I think you are referring to the function
xTaskResumeAll(), which (for V9.0.0) can be seen here:
https://sourceforge.net/p/freertos/code/HEAD/tree/tags/V9.0.0/FreeRTOS/Source/tasks.c#l2017

The code you pasted starts on line 2041
https://sourceforge.net/p/freertos/code/HEAD/tree/tags/V9.0.0/FreeRTOS/Source/tasks.c#l2041

…but where is listGET_OWNER_OF_HEAD_ENTRY() being called in that
function before the while loop? I presume you mean it is being called
INSIDE the while loop (line 2043).

You say listGET_OWNER_OF_HEAD_ENTRY() is only evaluated once, which I
assume means the while() loop only executes once, which implies that,
even though xPendingReadyList contains two tasks, the line:

while( listLIST_IS_EMPTY( &xPendingReadyList ) == pdFALSE )

returns pdFALSE when there are two tasks in the list, but doesn’t return
false when there is only one task in the list (one task having been
removed during the first go around the loop).

Is that correct?

If I change the definition xPendingReadyList on volatile it works
correctly

Can you please show how you updated the code to make the list volatile
as my suggestion just made the members of the list volatile.

rtel wrote on Saturday, February 09, 2019:

I just tried the following mocked up test, but I’m not sure I’m testing the right thing.

First I created a function that suspended the scheduler and manually added two tasks to the pending ready list:

void AddTasksToPendingReadyList( void );
void AddTasksToPendingReadyList( void )
{
TaskHandle_t x1, x2;
tskTCB *y1, *y2;

	/* Create two tasks, doesn't matter what they are. */
	xTaskCreate(	prvIdleTask,
					"X1", configMINIMAL_STACK_SIZE,
					( void * ) NULL,
					( tskIDLE_PRIORITY | portPRIVILEGE_BIT ),
					&x1 );

	xTaskCreate(	prvIdleTask,
					"X2", configMINIMAL_STACK_SIZE,
					( void * ) NULL,
					( tskIDLE_PRIORITY | portPRIVILEGE_BIT ),
					&x2 );


	y1 = ( tskTCB * ) x1;
	y2 = ( tskTCB * ) x2;

	/* Manually add the tasks to the pending ready list while the scheduler
	is suspended. */
	vTaskSuspendAll();
	vListInsertEnd( &( xPendingReadyList ), &( y1->xEventListItem ) );
	vListInsertEnd( &( xPendingReadyList ), &( y2->xEventListItem ) );
}

Then I created a function that printed the number of items in the pending ready list:

void CheckTasksInList( void );
void CheckTasksInList( void )
{
	printf( "Tasks in list: %d\r\n", ( int ) xPendingReadyList.uxNumberOfItems );
	fflush( stdout );
}

I then called the following sequence from the idle task:

   taskENTER_CRITICAL(); // Prevent context switches.
	CheckTasksInList(); // Should print 0 tasks in list.
	AddTasksToPendingReadyList(); // Adds two tasks while scheduler suspended. */
	CheckTasksInList(); // Should print 2 tasks in list.
	xTaskResumeAll();
	CheckTasksInList(); // Should print 0 tasks in list again.

And sure enough I observed the expected behaviour, even without volatile defined, and with GCC optimization set to its most aggressive:

Tasks in list: 0
Tasks in list: 2
Tasks in list: 0

I also stepped through the while loop in xTaskResumeAll() to ensure two iterations were observed.

ldb wrote on Saturday, February 09, 2019:

The volatile is in the wrong place :slight_smile:

It is currently
ListItem_t * configLIST_VOLATILE pxIndex;

try
configLIST_VOLATILE ListItem_t * pxIndex;

(type * volatile ptr) means the thing at the pointer address is volatile
(volatile type * ptr) means the pointer itself is volatile

The compiler is correctly asssuming the pointer doesn’t change because it isn’t volatile the thing at the pointer is volatile … it is behaving exactly as it should.

If you ever get confused go for safety
(volatile type * volatile ptr)
Now the pointer and what it points to are both volatile :slight_smile:

xelus wrote on Saturday, February 09, 2019:

Thanks for your help.

I presume you mean FreeRTOS V9.0.0. Why use an old version?

Yes, I’m using 9.0.0 version, and I’m using this old version because this is old code, which is only bugfixed. I’m not sure what happens, when I will change to the newest Free RTOS version…

Is that the latest compiler version?

Yes, I have used in test the newest version

…but where is listGET_OWNER_OF_HEAD_ENTRY() being called in that
function before the while loop? I presume you mean it is being called
INSIDE the while loop (line 2043).

This macro is called inside loop, but in assembler code value is evaluated into microcontroller register before loop begins. Without any volatiles it looks like:

2041  	while( listLIST_IS_EMPTY( &xPendingReadyList ) == pdFALSE )
(1) 019e9c:   9382 3980           TST.W   &xPendingReadyList
(2) 019ea0:   2486                JEQ     (0x9fae)
(3) 019ea2:   0028 398A           MOVA    &0x0398a,R8
2043 pxTCB = ( TCB_t * ) listGET_OWNER_OF_HEAD_ENTRY( ( &xPendingReadyList ) );
(4) 019ea6:   0839 000C           MOVA    0x000c(R8),R9
(5) (.....)
2052    xYieldPending = pdTRUE;
(6) 019f74:   4392 3AEA           MOV.W   #1,&xYieldPending
2041  	while( listLIST_IS_EMPTY( &xPendingReadyList ) == pdFALSE )
(7) 019f78:   9382 3980           TST.W   &xPendingReadyList
(8) 019f7c:   2394                JNE     (0x9ea6)
2060    				if( pxTCB != NULL )

(1) - check if list isn’t empty
(2) - exit when list is empty
(3) - put into R8 register ( pxList )->xListEnd ))->pxNext (pointer value)
(4) - dereferencing ( (&( ( pxList )->xListEnd ))->pxNext->pvOwner
(7) - check if list isn’t empty (and it is not)
(8) - jump to line (4) if there is another task left in the list. In the meantime ( pxList )->xListEnd ))->pxNext was changed, but do not recalculate R8 register value
If there is only one task, then in line (8) it out of the loop and everything work fine.

Can you please show how you updated the code to make the list volatile
as my suggestion just made the members of the list volatile.
I added to FreeRTOSConfig.h line:

#define configLIST_VOLATILE volatile

I added your test function to the code and anduring execution it enters into infinite loop - just like in my case. listGET_OWNER_OF_HEAD_ENTRY always return X1 task.

(type * volatile ptr) means the thing at the pointer address is volatile
(volatile type * ptr) means the pointer itself is volatile

Isn’t it inversly?
(type * volatile ptr) means the pointer itself is volatile
(volatile type * ptr) means the thing at the pointer address is volatile

The compiler is correctly asssuming the pointer doesn’t change (…)

pxIndex is changed directly one line up, so I think, that compiler shouldn’t assuming that it isn’t changed, even if it isn’t set as volatile

I have temporarily added a configLIST_VOLATILE macro and changed the definitions:

/*
 * Definition of the only type of object that a list can contain.
 */
struct xLIST_ITEM
{
	listFIRST_LIST_ITEM_INTEGRITY_CHECK_VALUE			/*< Set to a known value if configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES is set to 1. */
	configLIST_VOLATILE TickType_t xItemValue;			/*< The value being listed.  In most cases this is used to sort the list in descending order. */
	struct xLIST_ITEM volatile * configLIST_VOLATILE pxNext;		/*< Pointer to the next ListItem_t in the list. */
	struct xLIST_ITEM volatile * configLIST_VOLATILE pxPrevious;	/*< Pointer to the previous ListItem_t in the list. */
	void * pvOwner;										/*< Pointer to the object (normally a TCB) that contains the list item.  There is therefore a two way link between the object containing the list item and the list item itself. */
	void * configLIST_VOLATILE pvContainer;				/*< Pointer to the list in which this list item is placed (if any). */
	listSECOND_LIST_ITEM_INTEGRITY_CHECK_VALUE			/*< Set to a known value if configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES is set to 1. */
};
typedef volatile struct xLIST_ITEM ListItem_t;					/* For some reason lint wants this as two separate definitions. */

struct xMINI_LIST_ITEM
{
	listFIRST_LIST_ITEM_INTEGRITY_CHECK_VALUE			/*< Set to a known value if configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES is set to 1. */
	configLIST_VOLATILE TickType_t xItemValue;
	struct xLIST_ITEM volatile * configLIST_VOLATILE pxNext;
	struct xLIST_ITEM volatile * configLIST_VOLATILE pxPrevious;
};
typedef volatile struct xMINI_LIST_ITEM MiniListItem_t;

/*
 * Definition of the type of queue used by the scheduler.
 */
typedef struct xLIST
{
	listFIRST_LIST_INTEGRITY_CHECK_VALUE				/*< Set to a known value if configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES is set to 1. */
	configLIST_VOLATILE UBaseType_t uxNumberOfItems;
	ListItem_t * configLIST_VOLATILE pxIndex;			/*< Used to walk through the list.  Points to the last item returned by a call to listGET_OWNER_OF_NEXT_ENTRY (). */
	MiniListItem_t xListEnd;							/*< List item that contains the maximum possible item value meaning it is always at the end of the list and is therefore used as a marker. */
	listSECOND_LIST_INTEGRITY_CHECK_VALUE				/*< Set to a known value if configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES is set to 1. */
} volatile List_t;

And now it’s work ok in both situations - during switiching task and during handling two tasks in xPendingReadyList list. Assembler look like:

2041 while( listLIST_IS_EMPTY( &xPendingReadyList ) == pdFALSE )
(1) 019e70:   9382 3980           TST.W   &xPendingReadyList
(2) 019e74:   2488                JEQ     (0x9f86)
2043 pxTCB = ( TCB_t * ) listGET_OWNER_OF_HEAD_ENTRY( ( &xPendingReadyList ) );
(3) 019e76:   002F 398A           MOVA    &0x0398a,R15
(4) 019e7a:   0F39 000C           MOVA    0x000c(R15),R9
(5) (...)
2052    xYieldPending = pdTRUE;
019f4c:   4392 3AEA           MOV.W   #1,&xYieldPending
(6) 2041	while( listLIST_IS_EMPTY( &xPendingReadyList ) == pdFALSE )
        $C$L38:
(7) 019f50:   9382 3980           TST.W   &xPendingReadyList
(8) 019f54:   2390                JNE     (0x9e76)

Now in line (8) it jumps to line (3), where pointer value is evaluated ( pxList )->xListEnd ))->pxNext

rtel wrote on Sunday, February 10, 2019:

@Leon - good debate, however…

If, in the following case (cut and paste from compiler manual):

volatile int attncount;
volatile int * acptr;

"declare the object attncount to be an integer whose value may be 
altered at any time (say by an asynchronous attention handler), and the 
object acptr to be a pointer to a volatile object of integer type."

then are you sure:

(type volatile ptr) means the thing at the pointer address is volatile
(volatile type ptr) means the pointer itself is volatile

is correct?

In the extract from the manual, ‘volatile int * acptr’ is in the format
(volatile type ptr), and it says the opposite, that the pointer is
pointing to a volatile object (not that the pointer itself is volatile).

Further,
https://barrgroup.com/Embedded-Systems/How-To/C-Volatile-Keyword says
the following two are equivalent:

volatile uint8_t * p_reg;
uint8_t volatile * p_reg;

One is in the format (volatile type ptr) and the other (type volatile ptr).

Also, in the case of xTaskResumeAll(), the code in question is inside a
critical section, so no values can be altered outside of the compiler’s
knowledge (an interrupt can’t change the value, another task can’t
change the value, and the value is not mapped to a hardware register
that can change at any time) - so as the compiler can see all the code
is the ‘volatile’ qualifier necessary at all?

rtel wrote on Sunday, February 10, 2019:

My last post is a couple up from here, see
https://sourceforge.net/p/freertos/discussion/382005/thread/4edb6fb4b4/#1f4d/ee7d

ldb wrote on Sunday, February 10, 2019:

The guy who wrote the article said
Volatile pointers to non-volatile data are very rare (I think I've used them once), but I'd better go ahead and give you the syntax:

In your post above you gave only 2 options there are actually 3
volatile uint8_t * p_reg;
uint8_t volatile * p_reg;
uint8_t * volatile p_reg;

I should say that answer folows this
https://stackoverflow.com/questions/9935190/why-is-a-point-to-volatile-pointer-like-volatile-int-p-useful

Which is fine except
http://www.cs.utah.edu/~regehr/papers/emsoft08-preprint.pdf

However as per above I suspect 2 & 3 are compiler dependant I know a lot of embedded compilers that don’t interpret it that way suggested as the standard. From his code output I would say the volatile is being silently ignored.

As even your post stated volatile in that form are rare and I would suggest best avoided.
He fixed it by simply dragging it out and typecasting it a volatile pointer which makes it very clear to the compiler (every compiler unddrstands that one).

ldb wrote on Sunday, February 10, 2019:

It works now because you have a proper volatile pointer that the compiler recognizes. As I said above I think it is a case of compiler silently dropping the volatile.

xelus wrote on Sunday, February 10, 2019:

Also, in the case of xTaskResumeAll(), the code in question is inside a
critical section, so no values can be altered outside of the compiler’s
knowledge (an interrupt can’t change the value, another task can’t
change the value, and the value is not mapped to a hardware register
that can change at any time) - so as the compiler can see all the code
is the ‘volatile’ qualifier necessary at all?

Macro listGET_OWNER_OF_HEAD_ENTRY use pxList->xListEnd->pxNext pointer value, which is changed indirectly in uxListRemove function. Compiler doesn’t know, where pvContainer points, and by the pvContainer pointer we change ( pxList )->xListEnd ))->pxNext value.

I made a test with pointers declarations.

int * volatile ptr;

volatile int *ptr1;
volatile int * volatile ptr2;

int volatile *ptr3;
int volatile * volatile ptr4;

volatile int volatile *ptr5;
volatile int volatile * volatile ptr6;

The compiler reports warning for the ptr5 and ptr6. So it’s treats
volatile uint8_t * p_reg;
uint8_t volatile * p_reg;
as the same.

In my case, it looks like the compiler treats pxIndex as a pointer to volatile data, because it’s read again from memory pxIndex->pvOwner in the second iteration. If the compiler would treat the pointer as volatile, it should also read pxIndex value from memory before dereference.

ldb wrote on Monday, February 11, 2019:

Yeah it is as I thought the compiler does not know how to make a volatile pointer with that syntax which is not unusual.

As it is always accessed from the pointer below make the pointer below volatile which is the normal format of volatile which every compiler understands.

struct xMINI_LIST_ITEM
{
    listFIRST_LIST_ITEM_INTEGRITY_CHECK_VALUE          
    configLIST_VOLATILE TickType_t xItemValue;
   volatile  struct xLIST_ITEM * pxNext;   / * now a normal volatile ptr * /
    volatile struct xLIST_ITEM * pxPrevious; / * now a normal volatile ptr * /
};
typedef volatile struct xMINI_LIST_ITEM MiniListItem_t;

/*
 * Definition of the type of queue used by the scheduler.
 */
typedef struct xLIST
{
    listFIRST_LIST_INTEGRITY_CHECK_VALUE 
    configLIST_VOLATILE UBaseType_t uxNumberOfItems;
    volatile ListItem_t *  pxIndex;           / * now a normal volaile pointer * /
    MiniListItem_t xListEnd;              
    listSECOND_LIST_INTEGRITY_CHECK_VALUE               
} volatile List_t;

Do you notice the inconsistancy above in one case it uses ListItem_t which is bizzarely marked volatile (on your compiler I doubt it does what they expect) and everywhere else they used a struct pointer.

xelus wrote on Thursday, April 04, 2019:

Hi,
if someone has similiar problem with MSP430 compiler, then TI has confirmed that this is compiler bug (“Compiler may lose volatile qualifier in A->B->C when B and C are both volatile”) - ticket no CODEGEN-5943

rtel wrote on Thursday, April 04, 2019:

Appreciate you taking the time to report back.

cobusve wrote on Friday, April 05, 2019:

Looks to me like there are 2 different bugs here. There should be no need to use volatile for that sequence because none of the values are changed from another context, so the compiler should track the side-effects and should evaluate the expression on the second pass without the need to add volatile (as per section 5.1.2.3 of C99).

It seems that there is also a second compiler bug that at some point volatile is also dropped silently, so the workaround for the 1st bug fails because of a second bug.

From the description from Mateusz it seems like the second bug is confirmed but not the first?

xelus wrote on Monday, April 08, 2019:

I did not report the first bug to TI. Maybe I do not quite understand it, but is it possible to trace the side effect of such an operation at all? I think, that to achieve this, the only method would be to not optimize any function that writes to memory by the pointer.

cobusve wrote on Monday, April 08, 2019:

Any optimization which can potentially break seamtically correct code is just a bug, as section 5.1.2.3 of the standard specifically states that

The semantic descriptions in this International Standard describe the behavior of an abstract machine in which issues of optimization are irrelevant.

also

An actual implementation need not evaluate part of an expression if it can deduce that its value is not used and that no needed side effects are produced (including any caused by calling a function or accessing a volatile object).

This means that the compiler is allowed to omit evaluation of the expression IF AND ONLY IF it can guarantee that no side effect could possibly have changed the value. If the compiler loses track or cannot fully trace side effects is obliged to assume that the variable could have changed and it is no longer valid to make the optimization of emitting the evaluation.

It would be a disaster if compiler optimizations broke code using pointers randomly.

ldb wrote on Tuesday, April 09, 2019:

I think the second bug has nothing to do with the volatile it is the const … look at this crazy thing which all gets hidden because it’s a stupid macro.

List_t * const pxConstList = ( pxList );

Again what does that really mean … I am not sure I know how a C compiler is going to read that it is such a weird syntax. Is that a pointer to a const and we follow the same rules as volatile then anything pointed to by the pointer is also a const … so now you can’t change any fields on pxConstList or is only the pointer a const.

However we also declared a thing in the List_t struct as a volatile … so now is the thing at the pointer a const volatile or a const pointer to a volatile or perhaps it throws it’s hand in the air like me and goes WTF.

Basically the code is structural C junk. There is a good reason we don’t like using macros because you get this sort of thing and no one seemed to realize you have a mixture of consts and volatiles on the same pointers. I was trying to follow it and I was OMG is the pointer a const or a volatile now.

On the reference above you are doing this
https://embeddedgurus.com/barr-code/2012/01/combining-cs-volatile-and-const-keywords/

The moral of the story is writing silly C code does not always compile the same way on different compilers. I don’t see it as a compiler bug I see it as stupid code that is easily avoided.

I am with Cobus van Eeden there isn’t a context switch the whole point of the volatiles seems to be to combat the const and we are calling what the compiler does a bug.

richard_damon wrote on Tuesday, April 09, 2019:

List_t* const pxConstList = (pxList);

Is very well defined code, even if List_t has a volatile as part of its definition. It says that pxConstList has a constant value, that of pxList at that point in time, and that value can not change (but the List_t that it points to can). Perhaps the compiler has a bug that it thinks that const pointers imply that the objects they point to are constant (which they aren’t, they are allowed to change). The POINTER is constant, the POINTEE is volatile, and are very different objects.

As an example, you sell your house and new people move in. The contents of the house have changed, but a letter with a constant address, used to refer to you, but now referers to the new owners. pxConstList is like that letter, it knows exactly which List_t it is refering to, and which one can’t change, but the value of that List_t can change all over the place (including the pointers within it)