Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CF should not use bitfields #67

Closed
jphickey opened this issue Nov 23, 2021 · 1 comment · Fixed by #103
Closed

CF should not use bitfields #67

jphickey opened this issue Nov 23, 2021 · 1 comment · Fixed by #103

Comments

@jphickey
Copy link
Contributor

Use of bitfields is discouraged by many coding standards (including GSFC) because the C standard does not specifically dictate how they are packed into the underlying integer type. CF uses them in several internal structures, for example:

CF/fsw/src/cf_cfdp.h

Lines 159 to 165 in 2ca7f97

typedef struct {
unsigned q_index : 3; /* which Q is this in? */
unsigned ack_timer_armed : 1;
unsigned suspended : 1;
unsigned canceled : 1;
unsigned crc_calc : 1;
} flags_all_t;

If these truly need to be bit fields, then they should be implemented explicitly using shifts and masks. However, initial inspection of the code would suggest they do not need to be bit fields, they can be made into separate fields. While this may increase the memory footprint somewhat (struct is likely to be 5 bytes in the example instead of 4) this is probably a reasonable trade, because separate fields can be simply read/written directly rather than requiring a read-modify-write etc.

Note that when using bit fields, the assembly instructions to do shifts and masks will still be generated by compiler, even though the C syntax "looks" simple - it is hiding it all. So it may be considerably less efficient than accessing separate memory locations. This of course depends on hardware architecture, caching, optimization by the compiler, etc but in general bitfields will always be less efficient, due to the extra shifting and masking.

jphickey added a commit to jphickey/CF that referenced this issue Dec 8, 2021
Bit field behavior is platform-specific, bits are not
specified to be in any particular order. Furthermore,
unions of bitfields are likely undefined behavior.

This removes the bitfields and replaces with normal
fields.
@jphickey jphickey added the bug label Dec 8, 2021
@jphickey
Copy link
Contributor Author

jphickey commented Dec 8, 2021

This is actually a portability bug because the bit flags in "all" are not guaranteed to be the same actual bits as the flags in the rx/tx union members. So when crossing between the two union members, undefined behavior may result. The linked PR addresses this.

jphickey added a commit to jphickey/CF that referenced this issue Dec 9, 2021
Bit field behavior is platform-specific, bits are not
specified to be in any particular order. Furthermore,
unions of bitfields are likely undefined behavior.

This removes the bitfields and replaces with normal
fields.
astrogeco added a commit that referenced this issue Dec 9, 2021
Fix #67, remove use of bitfields in CF
@skliper skliper added this to the Draco milestone Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants