Re: [dpdk-dev] [PATCH 1/2] mk: fix build for clang 4

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Oct 11, 2024 at 08:20:12PM +0200, Morten Brørup wrote:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Friday, 11 October 2024 19.48
> > 
> > On Fri, 11 Oct 2024 19:36:08 +0200
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> > 
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> > > > Sent: Monday, 17 April 2017 16.36
> > > >
> > > > clang 4 gives "taking address of packed member may result in an
> > > > unaligned pointer value" warnings in a few locations [1].
> > > >
> > > > Disabled "-Waddress-of-packed-member" warning for clang >= 4
> > > >
> > > > [1] build errors:
> > > > .../lib/librte_eal/common/eal_common_memzone.c:275:25:
> > > > error: taking address of packed member 'mlock' of class or
> > structure
> > > > 'rte_mem_config' may result in an unaligned pointer value
> > > > [-Werror,-Waddress-of-packed-member]
> > > >         rte_rwlock_write_lock(&mcfg->mlock);
> > > >                                ^~~~~~~~~~~
> > > >
> > > > .../lib/librte_ip_frag/rte_ipv4_reassembly.c:139:31:
> > > > error: taking address of packed member 'src_addr' of class or
> > structure
> > > > 'ipv4_hdr' may result in an unaligned pointer value
> > > > [-Werror,-Waddress-of-packed-member]
> > > >         psd = (unaligned_uint64_t *)&ip_hdr->src_addr;
> > > >                                      ^~~~~~~~~~~~~~~~
> > > >
> > > > .../lib/librte_vhost/vhost_user.c:1037:34:
> > > > error: taking address of packed member 'payload' of class or
> > structure
> > > > 'VhostUserMsg' may result in an unaligned pointer value
> > > > [-Werror,-Waddress-of-packed-member]
> > > > 	vhost_user_set_vring_num(dev, &msg.payload.state);
> > > > 				       ^~~~~~~~~~~~~~~~~
> > > >
> > > > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > ---
> > > >  mk/toolchain/clang/rte.vars.mk | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/mk/toolchain/clang/rte.vars.mk
> > > > b/mk/toolchain/clang/rte.vars.mk
> > > > index 7749b99..af34c10 100644
> > > > --- a/mk/toolchain/clang/rte.vars.mk
> > > > +++ b/mk/toolchain/clang/rte.vars.mk
> > > > @@ -79,5 +79,10 @@ include
> > > > $(RTE_SDK)/mk/toolchain/$(RTE_TOOLCHAIN)/rte.toolchain-compat.mk
> > > >  # workaround clang bug with warning "missing field initializer"
> > for "=
> > > > {0}"
> > > >  WERROR_FLAGS += -Wno-missing-field-initializers
> > > >
> > > > +# disable packed member unalign warnings
> > > > +ifeq ($(shell test $(CLANG_MAJOR_VERSION) -ge 4 && echo 1), 1)
> > > > +WERROR_FLAGS += -Wno-address-of-packed-member
> > > > +endif
> > > > +
> > > >  export CC AS AR LD OBJCOPY OBJDUMP STRIP READELF
> > > >  export TOOLCHAIN_CFLAGS TOOLCHAIN_LDFLAGS TOOLCHAIN_ASFLAGS
> > > > --
> > > > 2.9.3
> > > >
> > >
> > > Revisiting an old patch...
> > >
> > > Maybe the code causing the warnings should have been fixed instead of
> > disabling the compiler warning?
> > >
> > > We would probably handle this differently today... I wonder how many
> > warnings it spews out with the warning enabled?
> > >
> > > -Morten
> > >
> > 
> > With gcc-14 and re-enable packed-member and no-zero-length-bounds, lots
> > and lots of warnings.
> 
> Thanks for testing this, Stephen.
> 
> It seems to confirm my suspicion...
> 
> Random sampling:
> - The warning in lib/ip_frag/rte_ipv4_reassembly.c is caused by using a temporary variable instead of mempcy().
> - The warning in drivers/bus/ifpga/ifpga_bus.c is caused by a struct being packed for no good reason.
> 
> It reminds me of compiling the kernel in the good old days... spewed out lots of new warnings when using a new compiler version.
> 
> This really should be fixed. Disabling compiler warnings because it requires too much work fixing the bugs is not a valid reason.
> 
> > 
> > The ones related to locks are particularly concerning. Since many cpu's
> > can't do locked operation
> > on unaligned fields.
> 
> It certainly does raise the level of concern!
> 
> > Mlx5 driver seems particularly bad.
> 
> More sampling - zooming in on the mlx5 driver - makes me wonder why struct mlx5_mr_share_cache is packed.
> 
> Perhaps most of these warnings - generally, not limited to the mlx5 driver - will go away if we review if the involved structures really need to be packed.
> 
> > 

Is a first step on this to change the flag to fix any easy warnings, and
then change the flag to be per-component only for those that need more work
to fix? That should at least stop more issues from appearing in the
codebase over time.

/Bruce