zephyr: build failures with gcc 9.x

Describe the bug warnings with new compilers that turn into errors when building using sanitycheck

In file included from /home/nashif/Work/zephyrproject/zephyr/subsys/net/l2/ethernet/arp.c:16:
/home/nashif/Work/zephyrproject/zephyr/include/net/net_pkt.h: In function ‘net_pkt_set_src_ipv6_addr’:
/home/nashif/Work/zephyrproject/zephyr/include/net/net_pkt.h:824:9: error: taking address of packed member of ‘struct net_ipv6_hdr’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
  824 |         &NET_IPV6_HDR(pkt)->src);
      |         ^~~~~~~~~~~~~~~~~~~~~~~
/home/nashif/Work/zephyrproject/zephyr/subsys/net/l2/ethernet/arp.c: In function ‘arp_hdr_check’:
/home/nashif/Work/zephyrproject/zephyr/subsys/net/l2/ethernet/arp.c:537:32: error: taking address of packed member of ‘struct net_arp_hdr’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
  537 |      net_ipv4_is_addr_loopback(&arp_hdr->src_ipaddr)) {
      |                                ^~~~~~~~~~~~~~~~~~~~
/home/nashif/Work/zephyrproject/zephyr/subsys/net/l2/ethernet/arp.c: In function ‘net_arp_input’:
/home/nashif/Work/zephyrproject/zephyr/subsys/net/l2/ethernet/arp.c:590:9: error: taking address of packed member of ‘struct net_arp_hdr’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
  590 |         &arp_hdr->src_ipaddr,
      |         ^~~~~~~~~~~~~~~~~~~~
/home/nashif/Work/zephyrproject/zephyr/subsys/net/l2/ethernet/arp.c:602:30: error: taking address of packed member of ‘struct net_arp_hdr’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
  602 |       net_ipv4_is_addr_mcast(&arp_hdr->src_ipaddr)) {
      |                              ^~~~~~~~~~~~~~~~~~~~
/home/nashif/Work/zephyrproject/zephyr/subsys/net/l2/ethernet/arp.c:608:42: error: taking address of packed member of ‘struct net_arp_hdr’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
  608 |   addr = if_get_addr(net_pkt_iface(pkt), &arp_hdr->dst_ipaddr);
      |                                          ^~~~~~~~~~~~~~~~~~~~
/home/nashif/Work/zephyrproject/zephyr/subsys/net/l2/ethernet/arp.c:635:8: error: taking address of packed member of ‘struct net_arp_hdr’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
  635 |        &arp_hdr->src_ipaddr,
      |        ^~~~~~~~~~~~~~~~~~~~
/home/nashif/Work/zephyrproject/zephyr/subsys/net/l2/ethernet/arp.c:655:27: error: taking address of packed member of ‘struct net_arp_hdr’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
  655 |   if (net_ipv4_is_my_addr(&arp_hdr->dst_ipaddr)) {
      |                           ^~~~~~~~~~~~~~~~~~~~
/home/nashif/Work/zephyrproject/zephyr/subsys/net/l2/ethernet/arp.c:657:8: error: taking address of packed member of ‘struct net_arp_hdr’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
  657 |        &arp_hdr->src_ipaddr,
      |        ^~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[3]: *** [zephyr/subsys/net/l2/ethernet/CMakeFiles/subsys__net__l2__ethernet.dir/build.make:63: zephyr/subsys/net/l2/ethernet/CMakeFiles/subsys__net__l2__ethernet.dir/arp.c.obj] Error 1

To Reproduce

Build for native_posix on Fedora 30 with new compiler

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 18 (16 by maintainers)

Commits related to this issue

compiler: no warnings on address-of-packed-member With gcc 9.1.x on fedora 30 we are getting new warnings that turn into errors when running sanitycheck. Disable those while the issues are being addr... — committed to nashif/zephyr by nashif 5 years ago
compiler: no warnings on address-of-packed-member With gcc 9.1.x on fedora 30 we are getting new warnings that turn into errors when running sanitycheck. Disable those while the issues are being addr... — committed to zephyrproject-rtos/zephyr by nashif 5 years ago
net: Fix alignment when accessing network packet headers Avoid direct access to __packed struct elements. The structs are typically cast to data that is received from the network. Fixes #16587 Sign... — committed to jukkar/zephyr by jukkar 5 years ago
include: net: remove __packed from network structs GCC 9 produces a warning about taking the address of a member of a packed structure. Since the networking headers should all use unaligned accesses ... — committed to cfriedt/zephyr by cfriedt 3 years ago

Most upvoted comments

@rlubos @cfriedt @jukkar @tbursztyka

I’ve investigated this a bit.

First, adding __packed to a struct does not pack the members that are structs themselves, nor it guarantees the correct alignment for them, hence the warning. So, for example, you could have a struct in_addr placed at an odd address, which is incompatible with the fact that internally it has a uint32_t member of the union. See this link for more info.

The header structs must be packed since they reflect directly what came on the wire.

It only triggers with struct in6_addr and struct in_addr because those happen to have alignment requirements (i.e. they are no just structs that hide a simple byte array, but have 16-bit and 32-bit words in them). That’s why the warning doesn’t trigger with, for example, bt_addr_le_t which is an unpacked structure that is used inside packed headers, but it turns out that bt_addr_le_t has no alignment requirements at all, so the compiler detects that and doesn’t issue the warning.

The possible solutions:

Declare struct in6_addr and struct in_addr as __packed. This iis going to bloat the code if those structs are used widely outside the actual places where we parse/prepare a packet to go over the wire
Change the contents of the IP and ARP __packed header structs, so that instead of having a struct in_addr they have a uint8_t src.dst[4], and then make sure that we convert to an actual struct in_addr whenever required. In some cases, instead of converting a byte array into a struct in_addr, which requires a copy (since the array may be unaligned) it might be better to create special _raw versions of some funcions like net_ipv_is_ll_addr() that take an array of bytes instead of an address. With this solution, you can use GET_UNALIGNED or sys_le32_to_cpu to take the data and convert it to a struct in_addr.
Remove the __packed from net_ipv4_hdr and net_ipv6_hdr but add BUILD_ASSERT(sizeof(struct net_ipv4_hdr) == 20). Alignment would be an issue in this case though, so we would need to use UNALIGNED_GET() to access the members.

Before we do anything, however, it would be really interesting to see how Linux solves this problem of an intermediate representation that needs packing.

Additionally, in my opinion the code is just wrong in several places. For example:

static inline bool net_ipv6_is_addr_loopback(struct in6_addr *addr)
{
        return UNALIGNED_GET(&addr->s6_addr32[0]) == 0 &&
                UNALIGNED_GET(&addr->s6_addr32[1]) == 0 &&
                UNALIGNED_GET(&addr->s6_addr32[2]) == 0 &&
                ntohl(UNALIGNED_GET(&addr->s6_addr32[3])) == 1;
}

This makes no sense, because the param to the function is struct in6_addr * which implies an aligned struct, since the struct itself is not declared packed when defined, and we don’t take __packed struct in6_addr *addr as param to the function. So that means that the UNALIGNED_GET in this function is not necessary, at least with the warnings enabled. So the right fix for this function, aside from enabling the warnings, would be to either not use UNALIGNED_GET or pass in an array of 16 bytes instead of a struct in6_addr *.

carlescufi on Sep 29, 2021