Problems using PC-Lint lock semantic checking with FreeRTOS

Kernel
1

philpem wrote on Wednesday, September 16, 2015:

Hi,

I’m trying to set up PC-Lint to check for correct releasing of mutexes in FreeRTOS. This is normally done by specifying semantics for the functions which perform the locking and unlocking operations, specifically:

-sem(xSemaphoreTake, thread_lock, 1p)
-sem(xSemaphoreGive, thread_unlock, 1p)
-sem(xTaskCreate, thread_create(1), 1p)

The problem is that xSemaphoreTake and xSemaphoreGive are implemented as macros:

:::c
#define xSemaphoreGive( xSemaphore )		xQueueGenericSend( ( QueueHandle_t ) ( xSemaphore ), NULL, semGIVE_BLOCK_TIME, queueSEND_TO_BACK )

This prevents PC-Lint from applying the semantics as the macro is expanded out to a call to xQueueGenericSend. There’s also a side issue relating to mutexes and semaphores being fed to the same lock and unlock functions and thus one being misinterpreted as the other, but I’ll leave that aside for the moment.

I could reimplement xSemaphoreGive as follows (and do the same to xSemaphoreTake), which allows PC-Lint to detect the lock and unlock operations:

:::c
static inline BaseType_t xSemaphoreGive( SemaphoreHandle_t xSemaphore )
{
	return xQueueGenericSend( ( QueueHandle_t ) xSemaphore, NULL, semGIVE_BLOCK_TIME, queueSEND_TO_BACK );
}

This also has the benefit of improving type safety, e.g. preventing a mistyped variable name from passing the wrong thing into xSemaphoreGive. I’ll note that I’ve seen a bug like that at work – a variable protected by a mutex was accidentally passed into the Give function, which caused said function to crash quite spectacularly. The variable names in question were something like “varName” and “varNameLock”; a developer used the autocomplete function in their editor and forgot to add the “lock” suffix. In our case, even the C compiler’s own basic type checking would have likely caught that a struct was being passed in place of a semaphore (and if not, Lint’s strict type checking could have been configured to do so).

Back to the topic at hand – the problem I have is that my suggested modification would require a lot of changes to the FreeRTOS source code.

Is there any way I can do something like this without radically altering the FreeRTOS headers?

Is there a reason FreeRTOS was implemented using macros instead of static inline functions?

Thanks,
Phil.

2

rtel wrote on Wednesday, September 16, 2015:

All the design choices in FreeRTOS have a reason behind them. Some date back to the original versions of FreeRTOS, which were intended to be as small as possible. Hence, the queue is the fundemental primitiave that has all the event management built in, and semaphores were (originally) implemented by macros without adding anything to the code size - that is not quite true now as mutexes in particular are treated differently in the code from binary semaphores. Likewise error detection is done by assert macros that trap errors during development, but can be completely removed once code is tested, keeping size and run-time minimal. SafeRTOS on the other hand makes lots of error checks directly in the code, returns error codes for all anomalies, and therefore requires more code to implement equivalent functionality.

These days the original design choices still have benefits as the smaller amount of code means there is less code to test. However, newer features, such as event groups and direct to task notifications, are not based on queue data structures or functionality. Direct to task notifications can often be used in place of semahores (but not mutexes) and are smaller and much faster than semaphores.

FreeRTOS source code has always avoided using any features or syntax introduced by newer C standards, as it is compiled with more than 20 compilers, and the more obscure compilers only support the older standards. Hence inline functions have never been used in the core of FreeRTOS. However, inline functions are being used in the newer FreeRTOS components (TCP and FAT) as it is unlikely these newer components would not be used on really small processors anyway.

We did once make a type safe version of FreeRTOS, but had to back the changes out, I think (if I recall correctly) because of bugs in elf parsers used by some debuggers meant the debugger veiws were corrupted.

3

philpem wrote on Thursday, September 17, 2015:

Thanks for the explaination. I sat and thought about it last night, and realised I hadn’t factored in old compilers… I guess I’ve been spoiled by IAR EWARM and GCC, both of which are C99 compliant.

Incidentally, I’ve found a way to add type safety by wrapping the macros in functions. This involves creating a new header file which is included after the FreeRTOS headers. Said header contains a series of functions like this:

:::c
//lint -sem(xSemaphoreTake, thread_lock, 1p)
static inline BaseType_t (xSemaphoreTake)(SemaphoreHandle_t xSemaphore, const TickType_t xBlockTime)
{
	return xSemaphoreTake(xSemaphore, xBlockTime);
}
#undef xSemaphoreTake

The preprocessor is prevented from substituting the macro for the function name by surrounding the function name in brackets; this has no effect on the function definition. Then we simply insert the relevant macro, and undefine the macro after exiting. A few Lint comments are added to the file to inform PC-Lint of the function semantics, e.g. locking and unlocking.

This has the advantage that all the strong type checking can be disabled by wrapping the type-checking wrappers with an “#if TYPE_CHECKING” guard, and only define this when Lint is being run (or simply “#if defined(_lint)”). As the code has no side effects, it could even be added to debug builds.

To go further (I’m working on this!), an additional “MutexHandle_t” typedef could be defined based on SemaphoreHandle_t. Functions which accept mutexes could then be modified to accept this type, and PC-Lint can then be configured to enforce strong type checking upon it. That is to say, any function which only accepts a mutex will only accept a mutex (in Lint at least).

And all this because of someone hamming the name of a variable…

I’ll have a look at direct-to-task notifications, but I seem to recall there was some reason we could’t use them. One common pattern we use is that a function kicks off an interrupt-driven operation (e.g. A/D sampling), the interrupt raises a semaphore, and another function waits on the semaphore. The semaphore itself is not exposed outside of the code module in which it resides. The rationale there is that different tasks (or at least different functions running in a given task context) may make use of the A/D at different times.

Thanks,
Phil.

4

philpem wrote on Thursday, September 17, 2015:

As a side note, I think I’ve found a bug – no impact at runtime, but causes an Unreachable Code warning from Lint:

Line 1687 of task.h:

:::c
#define xTaskNotifyGive( xTaskToNotify ) xTaskNotify( ( xTaskToNotify ), 0, eIncrement );

Should be:

:::c
#define xTaskNotifyGive( xTaskToNotify ) xTaskNotify( ( xTaskToNotify ), 0, eIncrement )

Note the trailing semicolon at the end of the macro.

5

rtel wrote on Thursday, September 17, 2015:

Thanks for pointing this out - it looks like it has been changed already:

http://sourceforge.net/p/freertos/code/HEAD/tree/trunk/FreeRTOS/Source/include/task.h#l1711

Regards.

6

rtel wrote on Thursday, September 17, 2015:

One common pattern we use is
that a function kicks off an interrupt-driven operation (e.g. A/D
sampling), the interrupt raises a semaphore, and another function waits
on the semaphore. The semaphore itself is not exposed outside of the
code module in which it resides. The rationale there is that different
tasks (or at least different functions running in a given task context)
may make use of the A/D at different times.

If I understand this scenario correctly, then you can use a task
notification by saving the handle of the task to notify in a variable,
then the interrupt service routine clears the variable back to NULL once
it has used it to notify the task.

Ideally the variable would be inside a structure used by the driver to
describe the peripheral being used.

Regards.