OSAL "common_types.h" is not completely reliable

[skliper]

On some systems (particularly 64-bit) the types defined in OSAL "common_types.h" file do not always match their expected widths.

There is currently a very helpful compile-time assert to catch this if it does go wrong but we need it to not go wrong in the first place.

The best way to solve this is to leverage the C "stdint.h" file - this has been standard since C99. For any C library that does not have this header it can fall back to using the existing defines.

[skliper]

Imported from trac issue 1. Created by jphickey on 2014-12-16T21:25:05, last modified: 2015-11-20T16:22:16

[skliper]

Trac comment by jphickey on 2014-12-16 21:53:03:

Pushed branch "trac-1-common_types-fix" to address these issues

[skliper]

Trac comment by glimes on 2014-12-17 11:03:43:

We can hotlink from tickets to changesets with ![changeset:baa27c0] which gives [changeset:baa27c0].

I wish we could deprecate ALL of the types defined in "common_types.h" in favor of using the ones that were common enough 25 years ago that the standards committee nailed them into INCITS ISO/IEC 9899:1999 (E) "The C Programming Language" Section 7.18 "Integer types" ...

[skliper]

Trac comment by jphickey on 2014-12-18 16:00:47:

Agreed - would be nice to not reinvent the types at all but now you are talking about asking people to run something to the effect of s/uint32/uint32_t/g against all of their code.

But doing it this way has the advantage of making uint32_t and uint32 interchangeable for the time being, so one day in the future after everyone has moved to the OSAL with this change, we can run that big sed script on CFE/CFS and be done with it.

[skliper]

Trac comment by glimes on 2015-01-29 14:51:20:

Click [changeset:79ca34a here] to see the changeset.

[skliper]

Trac comment by glimes on 2015-01-29 15:42:04:

I have reviewed this change, and recommend ACCEPT with the following minor caveats carried forward.

• A significant block of source is within the _HAVE_STDINT_ preprocessor conditional. When implementing the build system that provides this symbol, we should take a moment to cross check that the block is being used, and that relevant tests pass.

• This may break code that conflicts with the <stdint.h> header.

• This may break code that uses the boolean type and depends on common_types.h defining it.

• This may break code that contains cpusize or cpussize typedefs, not previously defined in this header.

Any code broken by any of the above effects should be trivial to repair, and I would expect few external projects to see any impact.

[skliper]

Trac comment by jphickey on 2015-01-30 11:58:57:

Greg - Thanks for the feedback -- some good points there.

Accepting this change by itself should not have much effect on existing missions //unless// they happen to already define {{{HAVE_STDINT}}} in their build system (unlikely). In the case of an existing build that does //not// define that symbol, the only effective change here is the addition of cpusize, cpussize, and osalbool types. Specifically note that in this case {{{<stdint.h>}}} is also not included because it is inside the conditional, so this reduces the chance of any inadvertent name clash with something else in "stdint.h"; one must explicitly tell their build system to use the stdint.h file before it will be included.

One easy thought -- I could change the {{{HAVE_STDINT}}} conditional to be {{{OSAL_HAVE_STDINT}}} to even further reduce the likelihood that this is inadvertently defined in an existing build.

Other thoughts on the points you made:

• The amount of code inside the {{{#ifdef HAVE_STDINT}}} conditional block is approximately the same as all the other {{{#elif}}} branches. In an ideal world the automated tests would have to separately build with EACH branch to verify it (e.g. "arm", "i386", "ppc", etc). But I expect those {{{#elif}}} branches might never actually be used because we find out that EVERYONE has a stdint.h file (it is standardized, after all).

• boolean / osalbool: compatibility with old code that uses "boolean" is maintained by the extra typedef at the bottom (line 301). I experienced a real-world name conflict with "boolean" in the json-c library causing it to not compile together with OSAL. RTEMS 4.11 already deprecated their "boolean" type likely for the same reason. This is only intended to take the first baby step toward avoiding this name clash, and I suggest using "osalbool" instead for all new code. This might be useful to put on a preprocessor conditional as well but turning it off today will break a lot of CFE files.

• cpusize / cpussize: these are defined but //NOT// used anywhere because I didn't want to break compatibility with older OSAL versions. Like osalbool these are a suggestion, but we can leave them out for now if someone suspects any conflicts. Currently the OSAL/PSP use uint32 for sizes; the only problem is on 64-bit processors where one actually could have a size greater than 2^32^ but this is unlikely in a small embedded system.

[skliper]

Trac comment by mlogan on 2015-01-30 18:02:44:

I have reviewed changes per this ticket and ACCEPT as well. I also had concerns about type "boolean" as they are used in LADEE and RP missions code bases. I believe these are mitigated by the developer.

[skliper]

Trac comment by dheater on 2015-02-03 17:44:49:

Could you also define standard int types for systems fail to do so?
For example:
typedef short int int16; /* Legacy type /
typedef int16 int16_t; / Standard type */

That would mean that new code could use the standard int types instead of the locally defined ones.

[skliper]

Trac comment by jphickey on 2015-02-05 13:41:22:

FYI - I have done some more testing with the various permutations of compiler toolchains that I have available to me, and I had to make a couple minor changes to this commit.

Issue 1: The "size_t" type is not always available via {{{<stdint.h>}}}. It is officially defined in {{{<stddef.h>}}}, so the change is modified to include both stdint.h and stddef.h when compiling with the HAVE_STDINT flag. (On some toolchains you got it implicitly with stdint.h, on others you did not, so this makes it work on all.)

Issue 2: The "ssize_t" is actually not in either of those files, but actually in {{{<unistd.h>}}} and/or possibly {{{<stdio.h>}}} (seems to vary). It seems this may also be a POSIX extension that became de-facto standard for read/write calls.

The main use-case for the C library "ssize_t" is to support a "-1" error return code for functions that need to return a size when successful (i.e. read/write). However (particularly with trac #35 fixed) the OSAL will not actually need anything like this.

The more likely use-case for application code is to express offsets or distance between objects in memory. In this sense I have renamed the type to be "cpudiff" as an alias to the C-standard type "ptrdiff_t" which is defined in {{{<stddef.h>}}} (along with size_t) for this exact purpose.

With respect to the earlier comment about aliasing the C99-standard type names to the legacy type names, I like this idea. However my thought would be to just get this changeset merged in first, then I'll add that as another separate commit on top of this.

The updated changeset is now [changeset:b70692d]

[skliper]

Trac comment by glimes on 2015-02-05 16:54:42:

I've updated the Change Set link in the wiki page I'm using
to track this set of things to review, [wiki:ReviewMerge20150202].

I have reviewed Joe's updated edits and continue to recommend ACCEPT.

Interesting points about size_t and ssize_t.
I need to read my INCITS ISO/IEC 9899:1999 again.

[skliper]

Trac comment by acudmore on 2015-02-11 14:05:14:

I think it is a good idea to move to the standard types.
I checked a few systems that I have available:

• vxWorks 6.4 defines the stdint types, but does not have a stdint.h header.
• RTEMS does seem to have the correct header and types.
• The CubeSat mission I work on which uses GCC/Newlib/FreeRTOS has the header and types

Can anyone check to see if recent vxWorks has the stdint.h header? If it does, then most of the code should use these standard types.

One nit about the comment on 129:

• If the host system has a ISO C99 standard stdint header file, prefer it.
  We want to make sure we are not using a "host" header ( for example Linux 64 bit intel, if we are cross compiling to a 32 bit cold fire )
  Maybe change comment to:
• If the target toolchain …

On systems that have stdint, do we have an equivalent of the _STUCT_HIGH/LOW_BIT_FIRST macro? Is that needed anymore?

In ospi-os-core.h, does the use of OS_PRINTF bypass the OS_printf function?
The OS_printf function is useful on some targets ( essential on others )

[skliper]

Trac comment by jphickey on 2015-02-11 14:54:34:

Alan - I agree with your thoughts on the comment line - it is definitely the TARGET toolchain that needs to have the header. I can fix this up to be clear.

Regarding the STRUCT_HIGH_BIT/LOW_BIT_FIRST: My vote is that this is not needed any more and should be deprecated. There is no code that I am aware of that is actually using it. But more importantly, simply knowing the architecture (i.e. ARM or PPC) does not necessarily imply endianness. For sure there are little and big endian variants of ARM architectures.

In general we should always prefer code that is endian neutral and make all efforts to have code that will work the same on either type of system. If code MUST be endian-specific, then I prefer to do a runtime check i.e. check which byte is first after storing a uint16 with value "0x0102". If even that is not feasible, then the #define can go in the specific BSP/PSP as a compile-time option.

[skliper]

Trac comment by glimes on 2015-02-12 14:58:04:

Accepted by #41 for integration candidate.

[skliper]

Trac comment by glimes on 2015-03-04 14:54:06:

Confirmed that the fix from [changeset:b70692d] is present in the
current development branch of OSAL, [changeset:5aae372].
Closing this ticket as resolved; if we want to do further refinements,
a new ticket would be appropriate.