Re: [PATCH] acl: fix build with GCC 15 on aarch64

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

On Thu, Mar 27, 2025 at 10:39:09AM +0000, Konstantin Ananyev wrote:
> 
> 
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Thursday, March 27, 2025 10:37 AM
> > To: Bruce Richardson <bruce.richardson@intel.com>
> > Cc: dev@dpdk.org; bluca@debian.org; stable@dpdk.org; Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>; David Christensen
> > <drc@linux.ibm.com>; Wathsala Vithanage <wathsala.vithanage@arm.com>
> > Subject: Re: [PATCH] acl: fix build with GCC 15 on aarch64
> > 
> > On Thu, Mar 27, 2025 at 9:55 AM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Wed, Mar 26, 2025 at 11:39:28AM +0100, David Marchand wrote:
> > > > Caught in OBS for Fedora Rawhide on aarch64:
> > > >
> > > > [  198s] In file included from ../lib/acl/acl_run_neon.h:7,
> > > > [  198s]                  from ../lib/acl/acl_run_neon.c:5:
> > > > [  198s] In function ‘alloc_completion’,
> > > > [  198s]     inlined from ‘acl_start_next_trie’ at
> > > >       ../lib/acl/acl_run.h:140:24,
> > > > [  198s]     inlined from ‘search_neon_4.isra’ at
> > > >       ../lib/acl/acl_run_neon.h:239:20:
> > > > [  198s] ../lib/acl/acl_run.h:93:25: error: ‘cmplt’ may be used
> > > >       uninitialized [-Werror=maybe-uninitialized]
> > > > [  198s]    93 |                 if (p[n].count == 0) {
> > > > [  198s]       |                     ~~~~^~~~~~
> > > > [  198s] ../lib/acl/acl_run_neon.h: In function ‘search_neon_4.isra’:
> > > > [  198s] ../lib/acl/acl_run_neon.h:230:27: note: ‘cmplt’ declared here
> > > > [  198s]   230 |         struct completion cmplt[4];
> > > > [  198s]       |                           ^~~~~
> > > >
> > > > The code was resetting sequentially cmpl[].count at the exact index that
> > > > later call to alloc_completion uses.
> > > > While this code seems correct, GCC 15 does not understand this (probably
> > > > when applying some optimisations).
> > > >
> > > > Instead, reset cmpl[].count all at once in acl_set_flow, and cleanup the
> > > > various vectorized implementations accordingly.
> > > >
> > > > Bugzilla ID: 1678
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > ---
> > > >  lib/acl/acl_run.h         | 5 +++++
> > > >  lib/acl/acl_run_altivec.h | 8 ++------
> > > >  lib/acl/acl_run_avx2.h    | 4 +---
> > > >  lib/acl/acl_run_neon.h    | 8 ++------
> > > >  lib/acl/acl_run_scalar.c  | 4 +---
> > > >  lib/acl/acl_run_sse.h     | 8 ++------
> > > >  6 files changed, 13 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/lib/acl/acl_run.h b/lib/acl/acl_run.h
> > > > index 7f092413cd..9fd3e60021 100644
> > > > --- a/lib/acl/acl_run.h
> > > > +++ b/lib/acl/acl_run.h
> > > > @@ -176,6 +176,8 @@ acl_set_flow(struct acl_flow_data *flows, struct completion *cmplt,
> > > >       uint32_t cmplt_size, const uint8_t **data, uint32_t *results,
> > > >       uint32_t data_num, uint32_t categories, const uint64_t *trans)
> > > >  {
> > > > +     unsigned int i;
> > > > +
> > > >       flows->num_packets = 0;
> > > >       flows->started = 0;
> > > >       flows->trie = 0;
> > > > @@ -187,6 +189,9 @@ acl_set_flow(struct acl_flow_data *flows, struct completion *cmplt,
> > > >       flows->data = data;
> > > >       flows->results = results;
> > > >       flows->trans = trans;
> > > > +
> > > > +     for (i = 0; i < cmplt_size; i++)
> > > > +             cmplt[i].count = 0;
> > > >  }
> > >
> > > Minor nit, but since we are using c11 standard, is it not better to declare
> > > "i" inside the "for" statement. Keeps diffs simpler for adding/removing
> > > code, I think.
> > 
> > I still have this (bad) habit but yes, it looks nicer with declaring
> > in for() itself.
> 
> My vote would be to keep it in an old fashioned way.
> Nothing is wrong in defining variable to use at the start of the function :) 
>  

No, there isn't. However, there is also a reason why later GCC revisions
and modern languages allow use of a temporary variable defined within the
loop itself. Cognitively, it's easier to have variables defined at point of
use, as it saves the user having to mentally track them or move up and down the
code. Furthermore, when debugging or reworking the code, it's far easier to
have the variable inside the "for" statement as it means that as we
comment/uncomment, or remove/re-add, the code block, the variable definition
also gets commented/uncommented too, without having to constantly scroll up
to make changes in two places. Lastly, it makes for smaller git diffs too.

/Bruce