Type safety and improved handling of CFE_SB_MsgId_t values

[skliper]

In 6.6, as we move to supporting MsgId's, MsgKey's, RouteIdx, and other types, we should move away from using native C types and wrapping the types in a struct to prevent accidentally using the wrong type in assignments and function calls.

This will, of course, require re-tooling any existing code that expects the type to be a simple type...

For example, instead of:

typedef uint16 CFE_SB_MsgId_t;

uint32 SomeFunction(CFE_SB_MsgId_t MsgId) {
    /* ... */
    return MsgId; /* this will work fine, even though it's bad behavior! */
}

Use:

typedef uint16 CFE_SB_MsgId_Atom_t;
typedef struct
{
    CFE_SB_MsgId_Atom_t __MsgId; /* private, do not touch */
} CFE_SB_MsgId_t;

uint32 SomeFunction(CFE_SB_MsgId_t MsgId) {
    /* ... */
    return MsgId; /* will now give a compiler error */
}

[skliper]

Imported from trac issue 214. Created by cdknight on 2017-10-24T13:48:15, last modified: 2019-07-03T12:58:32

[skliper]

Trac comment by cdknight on 2017-10-24 14:03:11:

Note that the struct should read (sorry the double-underscore got interpreted by the trac system):

{{{
typedef struct
{
CFE_SB_MsgId_Atom_t __MsgId; /* private, do not touch **/
} CFE_SB_MsgId_t;
}}}

[skliper]

Trac comment by cdknight on 2017-10-24 14:04:04:

Curious what the performance impact will be for this, particularly for frequently-used types.

One option would be to have a compile-time flag that enables the wrappers so you can do compiler type checking, then disable for performance.

A-la:
{{{
#ifdef BUILD_WRAPPERS

typedef uint16 CFE_SB_MsgId_Atom_t;
typedef struct
{
CFE_SB_MsgId_Atom_t __MsgId; /* private, do not touch */
}

#else

typedef uint16 CFE_SB_MsgId_t;

#endif
}}}

[skliper]

Trac comment by glimes on 2017-10-24 14:05:16:

I have the power to edit the Description for you (this may
be one of those silly "admin only" things).

[skliper]

Trac comment by cdknight on 2017-10-24 14:09:44:

Replying to [comment:4 glimes]:

I have the power to edit the Description for you (this may
be one of those silly "admin only" things).

yes, you silly admin. :D

[skliper]

Trac comment by glimes on 2017-10-24 14:10:46:

Before you make the code complicated in the name of making it faster, please prototype and measure. You may find that there is no runtime cost.

Generally, I've found that

• copy structure by assignment
• pass structure to function by value
• return structure from function by value
  tend to be just as fast as an int (or a double), for structures
  that are as small as an int (or a double).

[skliper]

Trac comment by cdknight on 2017-10-24 14:11:47:

Note also I filed ticket #241 suggesting creating a type for status/return codes so that it's clear when functions are returning a status. I have a first pass at the ES patch demonstrating the code change impact.

[skliper]

Trac comment by cdknight on 2017-10-24 15:00:38:

whoop, not 6.6

[skliper]

Trac comment by cdknight on 2017-10-27 13:24:21:

also note we may want to wrap void *'s wherever possible/needed (see ticket #247 for an example where we got bit by a void *.)

[skliper]

Trac comment by cdknight on 2017-10-27 14:26:57:

see also ticket #172

[skliper]

Trac comment by jphickey on 2018-05-14 12:36:48:

Upvote this ticket to be addressed in the next CFE release.

See also #100. We should use the same approach for CFE Pipe IDs as I'm seeing some potential (mis)use of pipe IDs as array indices.

In every reasonable compiler out there, passing a struct by value is just as efficient as passing an integer of the same size (provided the structure size is less than or equal to sizeof(long)). So wrapping an integer value in a struct should have no effect on performance at all.

I would advocate using this approach everywhere we have an "id" type that is numeric in nature. It ensures that it cannot be mixed with other integer types haphazardly, and also helps preserve architectural integrity by forcing one to treat it as an opaque value, only going through the correct API to manipulate the values.

[skliper]

Trac comment by jphickey on 2019-03-27 11:23:51:

Branch trac-214-msgid-type-safety, commit [changeset:d61f598] implements this concept for the CFE_SB_MsgId_t type.

This was originally implemented in #206 but not included in CFE 6.6 due to the API compatibility concerns at the time of review. I have rebased it to the current development branch and associated it with this ticket instead.

[skliper]

Trac comment by jhageman on 2019-07-03 12:58:32:

Moved unfinished 6.7.0 tickets to next release

[jphickey]

Submitted tickets and pull requests in all app repos to be compatible going forward:

• App should treat CFE_SB_MsgId_t values as opaque sample_app#53
• App should treat CFE_SB_MsgId_t values as opaque ci_lab#45
• App should treat CFE_SB_MsgId_t values as opaque to_lab#28
• App should treat CFE_SB_MsgId_t values as opaque sch_lab#37

[astrogeco]

@skliper I was having a conversation about message IDs and remembered this ticket. I think based on the discussion in #592 and #602 this issue shouldn't have been closed.

[skliper]

@astrogeco - right. Only partially addressed.