compilationDate_t not handled correctly

[jason-sachs]

The use of compilationDate_t is handled incorrectly in several ways. This is a typedef:

typedef const struct compilationDate_type{
 uint8_t date[11];
 uint8_t time[8];
}compilationDate_t;

This is a 19-byte structure containing date and time info. It is used by application code as follows (

X2Cscope_library_make/interface/X2Cscope.c

Line 61 in 6bac2d8

compilationDate_t compilationDate = {__DATE__, __TIME__};

)

compilationDate_t compilationDate = {__DATE__, __TIME__};

void X2Cscope_Init(void)
{
    X2Cscope_HookUARTFunctions(sendSerial, receiveSerial, isReceiveDataAvailable, isSendReady);
    X2Cscope_Initialise((void*)X2CscopeArray, X2CSCOPE_BUFFER_SIZE, X2CSCOPE_APP_VERSION, compilationDate);
}

and internally the implementation does this:

X2Cscope_library_wrapper.c:

void X2Cscope_Initialise(void* scopeArray, uint16_t scopeSize, const uint16_t appVersion, compilationDate_t compilationDate) {
    ...
    initVersionInfo(TableStruct, appVersion, compilationDate);
    ...
}

and in VersionInfo.c

void initVersionInfo(volatile tTableStruct* tblStruct, const uint16 appVersion, compilationDate_t compilationDate)
{
	tblStruct->framePrgVersion = appVersion;
	tblStruct->framePrgCompDateTime = (uint8*)&compilationDate;
}

where tTableStruct is defined in CommonFcts.h:

typedef struct tTableStruct tTableStruct;
struct tTableStruct {
    ...
    uint16 framePrgVersion;
    uint8* framePrgCompDateTime;
    ...
};

This implementation has several problems.

1. __DATE__ and __TIME__ are string constants with 11 characters and 8 characters, respectively. But C uses null-terminated strings, so the storage space needs 12 characters and 9 characters, respectively. Also, a char and a uint8_t are not necessarily synonymous. The correct application-level declaration and use should be:

   typedef struct compilationDate_type {
     char date[12];
     char time[8];
   } compilationDate_t;
   
   ...
   
   const compilationDate_t compilationDate = {__DATE__, __TIME__};
   doSomethingWith(&compilationDate);
   

   The library is welcome to define its own internal type that has a non-null-terminated array of uint8 to store the content (assuming sizeof char == sizeof uint8_t, which is true on most embedded processors but not all -- notably TI C2800 DSP which has a 16-bit char), but initializing a uint8_t[11] with __DATE__ is problematic.

2. uint8 and uint16 are not standard data types. Code should use uint8_t and uint16_t as declared in <stdint.h>. Adding additional types and mixing/matching use of uint8_t / uint8 has the potential to cause confusion. (Entered as Use stdint.h types such as uint8_t, and do not create nonstandard aliases; X2cDataTypes.h does not exist #10.)

3. The const in const uint16_t appVersion is useless for callers as a function signature; C passes all arguments by value. It does mean that the implementation of initVersionInfo cannot change the value in its implementation, but that is of questionable value. The const keyword has its primary use when defining storage for a variable (const SOMETYPE x = ...) and when using in a function signature that accepts pointers (void somefunc(const SOMETYPE *px);) to prevent the compiler from modifying the content.

4. compilationDate_t is passed by value rather than by pointers when it's not necessary to do so, and it is stored as a pointer when it is not safe to do so.

   • The compiler is free to allocate storage on the stack for the function argument compilationDate in both X2Cscope_Initialise() and initVersionInfo(), and copy the 19-byte data structure twice, once for each function call. (It may be able to optimize this out, but I am not sure of this.)
   • Furthermore, it is stored in framePrgCompDateTime as a uint8_t * based on &compilationDate in initVersionInfo(), which may be on the stack. Safe programming practices should copy this structure into an internal buffer, rather than depend on storing a typecast pointer to an external struct.

5. If framePrgCompDateTime is going to use a pointer rather than an internal buffer, it should at least be a const uint8_t * (or const compilationDate_t *) so that the compiler knows it is not safe to modify the pointer data.

[jason-sachs]

Just as an example that demonstrates what happens with the pass-by-value / pass-by-pointer, see https://gist.github.com/jason-sachs/795ec49009362915e01f9f8a82490b06

If you look at what happens, the datetime_t struct is copied onto the stack, and then a pointer to that data on the stack is stored for later use, which is not valid.

[MarkWendler]

We are limited here in some level with the X2C core implementation that is not using pointer for the compilation date. Definitely the structure and the wrapper can be improved. I will ceck if changing the whole story to piner is feasible or not.
Performance is not impacted significantly as the init is running just once.