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

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

On Thu, Mar 27, 2025 at 12:10:12PM +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.
> 
> I understand that it is probably more convenient, though from my perspective it is also more error prone.
> I saw several times people unintentionally defined new variable (in a local scope) with the same name
> that was already used in an outer scope, especially when function becomes large and clunky.

There is a gcc warning flag to indicate such cases "-Wshadow" or
"-Wshadow-local" [1].

[1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html