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

[Valentine]

On Fri, Jun 07, 2019 at 11:07:48AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Valentine,

Hi Phil,

> 
> On 5/30/19 7:38 PM, 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.
> > 
> > For example:
> > 
> >   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 );
> >         |            ^~~~~~~~~~~~
> >   include/ipxe/refcnt.h:95:18: note: in definition of macro ‘ref_get’
> >      95 |  ref_increment ( refcnt );     \
> >         |                  ^~~~~~
> >   include/ipxe/uri.h: In function ‘uri_put’:
> >   include/ipxe/uri.h:189:12: error: taking address of packed member of ‘struct uri’
> >   may result in an unaligned pointer value [-Werror=address-of-packed-member]
> >     189 |  ref_put ( &uri->refcnt );
> >         |            ^~~~~~~~~~~~
> >   include/ipxe/refcnt.h:109:18: note: in definition of macro ‘ref_put’
> >     109 |  ref_decrement ( refcnt );     \
> >         |                  ^~~~~~
> > 
> > This disables the warning to workaround the compilation issue.
> 
> We should ask yourself why do we get this warning, before ignoring it.
> 
> If 'struct uri *' is not aligned, the ref_put/get functions will indeed
> access unaligned pointer, and might trigger exceptions on some archs.
> 
> Following parse_uri() -> zalloc() -> malloc() -> realloc()
> 
> /**
>  * Reallocate memory
>  *
>  * @v old_ptr		Memory previously allocated by malloc(), or NULL
>  * @v new_size		Requested size
>  * @ret new_ptr		Allocated memory, or NULL
>  *
>  * Allocates memory with no particular alignment requirement.  @c
>  * new_ptr will be aligned to at least a multiple of sizeof(void*).
>  ...
> 
> Is that enough to confirm 'struct uri *' is always aligned?

I think you're missing the point. It's not about structure alignment.
It's about accessing an element of a packed structure. The address
of the element might be unaligned even if the structure itself is
aligned. The problem is that gcc 9.1 does not check the alignment,
but spits a bunch of warnings. If you build with NO_WERROR=1 you
get about 1200 warnings like that. I have checked most of them,
and they are false positives. Take for example 'refcnf' access
which is the *first* member of the uri structure.
Thus, 'packed' attribute does not affect its alignment at all.

In other cases, the address is either aligned correctly, or is used
as input for memcpy/memcmp functions which must work fine with
unaligned addresses on all architectures.

> 
> If so, this should be clearly documented in the commit message and in
> the Makefile around WORKAROUND_CFLAGS += -Wno-address-of-packed-member.

Is the comment like "Our sources have been vetted for correct usage." OK?

> 
> Regards,
> 
> Phil.
> 
> > 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..ad9644c2 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 warnings for taking address of packed member which
> > +# may result in an unaligned pointer value. Inhibit the warnings.
> > +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
> > 

-- 
Thanks,
Val.