[ipxe-devel] [PATCH v2 2/2] [build] Workaround compilation error with gcc 9.1

[Valentine]

On Sun, Jun 09, 2019 at 01:30:12PM +0300, Valentine Barshak wrote:
> Compiling with gcc 9.1 generates lots of "taking address of packed
> member of ... may result in an unaligned pointer value" warnings:
> 
>   include/ipxe/uri.h: In function ‘uri_get’:
>   include/ipxe/uri.h:178:12: error: taking address of packed member of ‘struct uri’
>   may result in an unaligned pointer value [-Werror=address-of-packed-member]
>     178 |  ref_get ( &uri->refcnt );
>         |            ^~~~~~~~~~~~
> 
> The problem is that gcc does not check the alignment, but shows
> lots of false positive warnings. For example, 'refcnt' is the first
> member of the 'uri' structure, so its alignment is not affected
> by the 'packed' attribute.

I took a closer look at it. It's a lot more messed up to say the least
than it initially seemed. According to gcc documentation, the "packed"
attribute doesn't just remove padding from the structure. It also tells
the compiler to make no assumptions about structure alignment.
This indeed indicates that the code might not work on some
architectures. We may want to use "packed, aligned(X)" attributes in
some cases to fix that.

However, there's another issue. Even if we access a char or void pointer,
gcc still shows the warning if the structure contains a VLA.
For example:
net/tls.c: In function ‘tls_send_client_hello’:
net/tls.c:1091:47: error: taking address of packed member of ‘struct
<anonymous>’ may result in an unaligned pointer value
[-Werror=address-of-packed-member]
 1091 |  memcpy ( hello.extensions.server_name.list[0].name, session->name,
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~

The "name" is a byte array, so there should be no alignment issues whatsoever.
But because the structure contains "uint8_t session_id[tls->session_id_len];",
which is a Variable-Length Array, it shows the warning.
If the session_id length was predefined, it would have worked regardless of
the actual session_id length.

I'm not sure how to nicely cope with this problem. The only thing
I could come up with is converting "hello" structure to byte buffer
and using offsetof, as follows:

  struct hello hello;
  char *buffer (char *)hello;
  ...
  extensions.server_name.list[0].name)
  memcpy ( buffer + offsetof(hello, extensions.server_name.list[0].name), session->name,
  ...

It's not pretty, but it makes gcc happy. I'd really like to hear any suggestions
from the maintainers because the problem has been around for more than a month
now, and nobody seems to really care.

The easiest would be to disable the warning. It doesn't break anything.
You cannot break what's already broken.

However, if we really want to support other architectures which are more
sensitive to pointer alignment, the code has to be rewritten.

> 
> This disables the warning to workaround the compilation issue.
> 
> Signed-off-by: Valentine Barshak <gvaxon at gmail.com>
> ---
>  src/Makefile.housekeeping | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/Makefile.housekeeping b/src/Makefile.housekeeping
> index f8334921..3634c4b2 100644
> --- a/src/Makefile.housekeeping
> +++ b/src/Makefile.housekeeping
> @@ -185,6 +185,13 @@ WNST_TEST = $(CC) -Wstringop-truncation -x c -c /dev/null -o /dev/null \
>  		  >/dev/null 2>&1
>  WNST_FLAGS := $(shell $(WNST_TEST) && $(ECHO) '-Wno-stringop-truncation')
>  WORKAROUND_CFLAGS += $(WNST_FLAGS)
> +
> +# gcc 9.1 generates false positive warnings for taking address of a packed
> +# structure member which may result in an unaligned pointer. Turn them off.
> +WNST_TEST = $(CC) -Wno-address-of-packed-member -x c -c /dev/null -o /dev/null \
> +		  >/dev/null 2>&1
> +WNST_FLAGS := $(shell $(WNST_TEST) && $(ECHO) '-Wno-address-of-packed-member')
> +WORKAROUND_CFLAGS += $(WNST_FLAGS)
>  endif
>  
>  # Some versions of gas choke on division operators, treating them as
> -- 
> 2.21.0
> 

-- 
Thanks,
Val.