> From: Bruce Richardson [mailto:bruce.richard...@intel.com] > Sent: Thursday, 27 March 2025 11.51 > > On Thu, Mar 27, 2025 at 10:39:09AM +0000, Konstantin Ananyev wrote: > > > > > > > -----Original Message----- > > > From: David Marchand <david.march...@redhat.com> > > > Sent: Thursday, March 27, 2025 10:37 AM > > > To: Bruce Richardson <bruce.richard...@intel.com> > > > Cc: dev@dpdk.org; bl...@debian.org; sta...@dpdk.org; Konstantin > Ananyev <konstantin.v.anan...@yandex.ru>; David Christensen > > > <d...@linux.ibm.com>; Wathsala Vithanage > <wathsala.vithan...@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.richard...@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: sta...@dpdk.org > > > > > > > > > > Signed-off-by: David Marchand <david.march...@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
I agree with Bruce. Also, minimizing the scope of local variables reduces the risk of bugs caused by unintended reuse without re-initialization. Reducing the risk of bugs is important. BTW: The Coding Style documentation [CODINGSTYLE] is still based on an ancient C version, and needs to be updated. [CODINGSTYLE]: https://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
