Type safe handles

hawk777 wrote on Friday, February 14, 2014:

I was wondering if there was some reason why FreeRTOS’s handles aren’t type safe? Right now, every handle (TaskHandle_t, QueueHandle_t, EventGroupHandle_t, etc.) is just void*. That means that any pointer at all can be passed where such a handle is needed—you can pass a QueueHandle_t where a TaskHandle_t is needed, for example, or perhaps easier to do, you can pass &handle where handle is needed, because void** is implicitly convertible to void*.

This could all be avoided if the handles were typedefs of “struct <something> *”, where “struct <something>” is perhaps even the real struct behind the handles, but the struct itself is left incomplete (not fully defined) in the header files to prevent applications from poking at hidden data—thus the handles remain opaque, as desired. Then the compiler would give errors for conversion between incompatible pointer types.

rtel wrote on Friday, February 14, 2014:

The reason is historic really in that when there were less features all the types of queue, semaphore, etc. were in fact pointers to the same structure. Making the types pointers to different (dummy) structures then requires casting within the FreeRTOS source files themselves when one API or private function calls another API function - which also upsets the lint checking as you cast from type to type.

That said, now more features handle types exist (timers, queue sets, event groups, etc.), and because we are always wanting to make continuous improvement, it is probably time to rethink this. I don’t think it is something we would do for the imminent V8 release though, its too close and I would have to find a way of keeping lint happy and check things with the C++ users. In the mean time please add a feature request through SourceForge so we can come back to it.

Regards.

hawk777 wrote on Sunday, February 16, 2014:

Ah, I will create a feature request, but would like to clarify one thing first. I was suggesting that the incomplete structs have the exact same name as the FreeRTOS-internal complete structs. Then the typedef would be completely accurate: QueueHandle_t could actually be a typedef for Queue_t* or struct QueueDefinition*, just that to applications, struct QueueDefinition would be incomplete. This would not require any casting, because within FreeRTOS, QueueHandle_t already IS the type to which you would otherwise be casting! Outside the kernel, it’s the same type, just that the struct is incomplete so the handle type ends up being opaque. Is there any reason not to do this?

Of course I realize this isn’t the best timing. Unfortunately I only just thought of this idea.

rtel wrote on Sunday, February 16, 2014:

Sorry for the brief reply - I’m just heading out the door. Please provide an example few lines of code to demonstrate how this would look inside the header file, given that the structure is actually defined in in the C file. We have had compiler warning/lint problems in the past where the same name takes two different types inside and outside of the C file (which used to be done by, for example, not including queue.h from queue.c to avoid name conflicts). I think you are suggesting something else though, which I would be happy to try. I’m not sure of the effect of having an incomplete structure in the header that is then included from the C file that has the complete structure, but we will see.

Regards.

hawk777 wrote on Sunday, February 16, 2014:

I would do this in the header file:

struct QueueDefinition;
typedef struct QueueDefinition *QueueHandle_t;

BaseType xQueueGenericSend(QueueHandle_t xQueue, /* more params */) PRIVILEGED_FUNCTION;
// more stuff

Then, in the source file, this:
struct QueueDefinition {
int8_t *pcHead;
/* more fields */
};
typedef struct QueueDefinition Queue_t;

BaseType xQueueGenericSend(QueueHandle_t xQueue, /* more params */) {
/* access field foo as xQueue->foo */
}

In the application, only the header file is included, so QueueDefinition is an incomplete struct and QueueHandle_t is a complete pointer to an incomplete type. In the source file, QueueDefinition is completed, so QueueHandle_t is a pointer to a complete type, so its members can be accessed directly.

Unfortunately I can’t tell you whether this will cause Lint to complain, as I don’t have it installed. “gcc -Wall -Wextra -ansi -pedantic” had no comment about such code.

(Message edited to add backslash escapes before the asterisks)

richard_damon wrote on Sunday, February 16, 2014:

Lint should not complain about making a typedef to a pointer of an incomplete type that is elsewhere made a typedef to a pointer to the same type but complete.

I remember years ago asking about the same question, and the answer was that at that point, some of the cross compilers used for some older systems would complain about making an typedef to an incompletely defined struct. Perhaps in the intervening years those compilers have made it up to the level of supporting this.

hawk777 wrote on Sunday, February 16, 2014:

It seems that the standard specifies that making a typedef to an incomplete struct is valid (“An incomplete type may only by used when the size of an object of that type is not needed. It is not needed, for example, when a typedef name is declared to be a specifier for a structure or union, or when a pointer to or a function returning a structure or union is being declared.”), and of course defining pointers to incomplete types is also legitimate.

As to whether there are compilers out there that fail to implement this, I don’t know; I have only GCC to test on.

rtel wrote on Sunday, February 16, 2014:

Unfortunately, when you support 18+ compilers, including some that could be described as obscure, what the standard says is of not much consequence.

I have actually experimented with this change today. I actually really like it because lint insists I remove lots of casting which makes the code much nicer. However, I have started to test with kernel aware debuggers, and the change breaks one of the two I have tested so far. I’m trying to get information from the vendor on why that is, but due to shortness of time it is likely the changes will get backed out prior to the release.

Regards.

rtel wrote on Monday, February 17, 2014:

Unfortunately, due to a bug in an elf parser used by a kernel aware plug-in, we can’t keep this change in for the release. When the parser is fixed it can be re-instated.

Regards.

hawk777 wrote on Wednesday, February 19, 2014:

Ah, OK. If it’s just about the name, would it be an option to use a different name for the incomplete and complete structs? You would still have to use a cast inside the FreeRTOS functions, but at least applications would get type safety for handles.

rtel wrote on Wednesday, February 19, 2014:

The files have already been packed up for release.

Regards.