Types in `byteorder.h` need a cleanup

[maribu]

Description

The types in byteorder.h like be_uint64_t look like this:

/**                                                                              
 * @brief          A 64 bit integer in big endian aka network byte order.        
 * @details        This is a wrapper around an uint64_t to catch missing conversions
 *                 between different byte orders at compile time.                
 */                                                                              
typedef union __attribute__((packed)) {                                          
    uint64_t u64;       /**< 64 bit representation */                            
    uint8_t u8[8];      /**< 8 bit representation */                             
    uint16_t u16[4];    /**< 16 bit representation */                            
    uint32_t u32[2];    /**< 32 bit representation */                            
    be_uint16_t b16[4]; /**< big endian 16 bit representation */                 
    be_uint32_t b32[2]; /**< big endian 32 bit representation */                 
} be_uint64_t; 

• Why does the type contain __attribute__((packed))? This will for the compiler to assume all accesses are unaligned. This reduces performance and increases ROM size when accessing it.
  • When used in a some foo_hdr_t structure, that structure should have the __attribute__((packed)) attribute instead. This would prevent unaligned accesses to incorrectly be assumed to be aligned when casting a uint8_t *-buffer to a header struct. But at the same time, be_uint64_t used outside of headers won't be effected by the performance penalty for no reason.
• Why does it contain both uint16_t u16[4] and be_uint16_t b16[4]?
  • Especially with the poor member documentation, this could easily mislead people to assume the u16 member contains host byte order and the b16 member contains network byte order.

Versions

Up to current master

[maribu]

Suggestion

Simplify all types to look like this

/**                                                                              
 * @brief          A 64 bit integer in big endian aka network byte order.        
 * @details        This is a wrapper around an uint64_t to catch missing conversions
 *                 between different byte orders at compile time.                
 */                                                                              
typedef union {                                          
    uint64_t u64;       /**< 64 bit representation */                            
    uint8_t u8[8];      /**< 8 bit representation */                             
} be_uint64_t; 

[kaspar030]

1. This would need checking all users for possibly unaligned access...

2. Can we drop the array representation, and contain it to the conversion functions through casting?

[maribu]

1. This would need checking all users for possibly unaligned access...

It is not needed for all users. Only when a member of an packed (__attribute___((packed))) structure is either be_uint64_t (and friends) or its alias network_uint64_t (and friends), all accesses to those should be checked.

In order to cause unaligned access without a be_uint64_t in a packed structure, one would have to do something like:

    be_uint64_t beu64;
    uint32_t *foo = (uint32_t *)&beu64.u8[2];
    *foo = 1;

But that would be an unaligned access even with __attribute__((packed)) being added to be_uint64_t. One would have to also add that attribute to foo for this to be working, but in that case it would again work regardless of whether be_uint64_t is also packed or not.

Btw.: It seems that almost all instances of using these types are "standalone" (not as struct member). All those instances could profit from not using __attribute__((packed)). And only the remaining users need to be carefully reviewed.

2. Can we drop the array representation, and contain it to the conversion functions through casting?

Thinking about it, I agree that the array representation is usually quite odd. If it is an integer after all, why not just represent it as such? (The EUI-64 use case would be a counter example, though. IMO, the EUI-64 should just have never been defined as an integer in the first place.)

[kaspar030]

It is not needed for all users.

I mean, all users need to be checked if they're instances of sth like be_uint32_t foo = (be_uint32_t *) pktpos;, which is valid when be_uint32_t is packed, invalid otherwise, depending on pktpos.

[maribu]

I mean, all users need to be checked if they're instances of sth like be_uint32_t foo = (be_uint32_t *) pktpos;, which is valid when be_uint32_t is packed, invalid otherwise, depending on pktpos.

But let's convert those to byteorder_bebuftohl() and friends, regardless of the outcome of this discussion.

[maribu]

ipv6_addr_t is also pretty complex and this time without __attribute__((packed)) does have indeed an alignment requirement of 64 bit :-/

[maribu]

ipv6_addr_t is also pretty complex and this time without __attribute__((packed)) does have indeed an alignment requirement of 64 bit :-/

As a result, I now have to re-implement ipv6_addr_is_multicast(), rather than using it.

[maribu]

I wanted to see how much it breaks to make the IPv6 a simple byte array and created a PR to let Murdock have a look: https://github.com/RIOT-OS/RIOT/pull/14759

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.