openbmc / libpldm

Apache License 2.0
9 stars 12 forks source link

C library hygiene: Avoid use of bitfields #4

Open amboar opened 2 years ago

amboar commented 2 years ago

WG14/N1256 (C99 draft standard) section 6.7.2.1 paragraphs 10 and 11 state:

An implementation may allocate any addressable storage unit large enough to hold a bitfield. If enough space remains, a bitfield that immediately follows another bit-field in a structure shall be packed into adjacent bits of the same unit. If insufficient space remains, whether a bit-field that does not fit is put into the next unit or overlaps adjacent units is implementation-defined. The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined. The alignment of the addressable storage unit is unspecified.

A bit-field declaration with no declarator, but only a colon and a width, indicates an unnamed bit-field.108) As a special case, a bit-field structure member with a width of 0 indicates that no further bit-field is to be packed into the unit in which the previous bitfield, if any, was placed.

with the following footnote:

108) An unnamed bit-field structure member is useful for padding to conform to externally imposed layouts.

Essentially, the implementation defined bitfield order leads to duplicate struct member definitions such as https://github.com/openbmc/libpldm/blob/9c76679224cf4b1655323a1e5e561ad2cb493ba2/include/libpldm/base.h#L112-L122

Further, as multiple bitfields can be packed into a single storage unit, write-on-write ordering of read-modify-write sequences can stomp on values in concurrent environments (e.g. via signal handling or multi-threading).

IMO we should avoid use of bitfields and prefer use of explicitly sized types and use of macros for named shift and mask constants to encode and decode sub-unit fields. The consequence of this is we're always operating above the behaviour of endianness, which removes the need to double-define struct members.