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

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

[David Marchand]

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;
 }
 
 typedef void (*resolve_priority_t)
diff --git a/lib/acl/acl_run_altivec.h b/lib/acl/acl_run_altivec.h
index 2d398ffded..d5ccdb94f0 100644
--- a/lib/acl/acl_run_altivec.h
+++ b/lib/acl/acl_run_altivec.h
@@ -199,10 +199,8 @@ search_altivec_8(const struct rte_acl_ctx *ctx, const uint8_t **data,
 	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
 		total_packets, categories, ctx->trans_table);
 
-	for (n = 0; n < MAX_SEARCHES_ALTIVEC8; n++) {
-		cmplt[n].count = 0;
+	for (n = 0; n < MAX_SEARCHES_ALTIVEC8; n++)
 		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
-	}
 
 	 /* Check for any matches. */
 	acl_match_check_x4(0, ctx, parms, &flows, (uint64_t *)&index_array[0]);
@@ -270,10 +268,8 @@ search_altivec_4(const struct rte_acl_ctx *ctx, const uint8_t **data,
 	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
 		total_packets, categories, ctx->trans_table);
 
-	for (n = 0; n < MAX_SEARCHES_ALTIVEC4; n++) {
-		cmplt[n].count = 0;
+	for (n = 0; n < MAX_SEARCHES_ALTIVEC4; n++)
 		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
-	}
 
 	/* Check for any matches. */
 	acl_match_check_x4(0, ctx, parms, &flows, index_array);
diff --git a/lib/acl/acl_run_avx2.h b/lib/acl/acl_run_avx2.h
index 0b8967f22e..e069fb85b2 100644
--- a/lib/acl/acl_run_avx2.h
+++ b/lib/acl/acl_run_avx2.h
@@ -171,10 +171,8 @@ search_avx2x16(const struct rte_acl_ctx *ctx, const uint8_t **data,
 	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
 		total_packets, categories, ctx->trans_table);
 
-	for (n = 0; n < RTE_DIM(cmplt); n++) {
-		cmplt[n].count = 0;
+	for (n = 0; n < RTE_DIM(cmplt); n++)
 		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
-	}
 
 	t0 = _mm256_set_epi64x(index_array[5], index_array[4],
 		index_array[1], index_array[0]);
diff --git a/lib/acl/acl_run_neon.h b/lib/acl/acl_run_neon.h
index 63074f871d..3b9bd0cc39 100644
--- a/lib/acl/acl_run_neon.h
+++ b/lib/acl/acl_run_neon.h
@@ -172,10 +172,8 @@ search_neon_8(const struct rte_acl_ctx *ctx, const uint8_t **data,
 	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
 		     total_packets, categories, ctx->trans_table);
 
-	for (n = 0; n < 8; n++) {
-		cmplt[n].count = 0;
+	for (n = 0; n < 8; n++)
 		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
-	}
 
 	 /* Check for any matches. */
 	acl_match_check_x4(0, ctx, parms, &flows, &index_array[0]);
@@ -234,10 +232,8 @@ search_neon_4(const struct rte_acl_ctx *ctx, const uint8_t **data,
 	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
 		     total_packets, categories, ctx->trans_table);
 
-	for (n = 0; n < 4; n++) {
-		cmplt[n].count = 0;
+	for (n = 0; n < 4; n++)
 		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
-	}
 
 	/* Check for any matches. */
 	acl_match_check_x4(0, ctx, parms, &flows, index_array);
diff --git a/lib/acl/acl_run_scalar.c b/lib/acl/acl_run_scalar.c
index 3d61e79409..a3661b1b6b 100644
--- a/lib/acl/acl_run_scalar.c
+++ b/lib/acl/acl_run_scalar.c
@@ -121,10 +121,8 @@ rte_acl_classify_scalar(const struct rte_acl_ctx *ctx, const uint8_t **data,
 	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results, num,
 		categories, ctx->trans_table);
 
-	for (n = 0; n < MAX_SEARCHES_SCALAR; n++) {
-		cmplt[n].count = 0;
+	for (n = 0; n < MAX_SEARCHES_SCALAR; n++)
 		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
-	}
 
 	transition0 = index_array[0];
 	transition1 = index_array[1];
diff --git a/lib/acl/acl_run_sse.h b/lib/acl/acl_run_sse.h
index 93286a2c38..4ec819a215 100644
--- a/lib/acl/acl_run_sse.h
+++ b/lib/acl/acl_run_sse.h
@@ -205,10 +205,8 @@ search_sse_8(const struct rte_acl_ctx *ctx, const uint8_t **data,
 	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
 		total_packets, categories, ctx->trans_table);
 
-	for (n = 0; n < MAX_SEARCHES_SSE8; n++) {
-		cmplt[n].count = 0;
+	for (n = 0; n < MAX_SEARCHES_SSE8; n++)
 		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
-	}
 
 	/*
 	 * indices1 contains index_array[0,1]
@@ -293,10 +291,8 @@ search_sse_4(const struct rte_acl_ctx *ctx, const uint8_t **data,
 	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
 		total_packets, categories, ctx->trans_table);
 
-	for (n = 0; n < MAX_SEARCHES_SSE4; n++) {
-		cmplt[n].count = 0;
+	for (n = 0; n < MAX_SEARCHES_SSE4; n++)
 		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
-	}
 
 	indices1 = _mm_loadu_si128((xmm_t *) &index_array[0]);
 	indices2 = _mm_loadu_si128((xmm_t *) &index_array[2]);
-- 
2.48.1

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

[David Marchand]

Hi guys,

On Wed, Mar 26, 2025 at 11:40 AM David Marchand
<david.marchand@redhat.com> 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>

No pressure, but could you have a look?
This is for fixing some build issue in the CI (affecting main, at least).


-- 
David Marchand

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

[Bruce Richardson]

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.

For the rest of the code, I need to take a bit more time reviewing to be
sure I understand the change. I'll try and get to it later.

/Bruce

>  
>  typedef void (*resolve_priority_t)
> diff --git a/lib/acl/acl_run_altivec.h b/lib/acl/acl_run_altivec.h
> index 2d398ffded..d5ccdb94f0 100644
> --- a/lib/acl/acl_run_altivec.h
> +++ b/lib/acl/acl_run_altivec.h
> @@ -199,10 +199,8 @@ search_altivec_8(const struct rte_acl_ctx *ctx, const uint8_t **data,
>  	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
>  		total_packets, categories, ctx->trans_table);
>  
> -	for (n = 0; n < MAX_SEARCHES_ALTIVEC8; n++) {
> -		cmplt[n].count = 0;
> +	for (n = 0; n < MAX_SEARCHES_ALTIVEC8; n++)
>  		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
> -	}
>
<snip>

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

[David Marchand]

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.


-- 
David Marchand

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

[Konstantin Ananyev]

> -----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 :) 
 

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

[Bruce Richardson]

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

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

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@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.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

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

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

[Konstantin Ananyev]

> > >
> > > > -----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.
Personally, I think it is a good practice to do a 'mental track' of your variables when writing the code,
and having all of them defined in one place definitely helps with that.
Anyway, I am not about to stop people to define variables inside the for() if it is more convenient for them,
but I am against to force people to write code that way.     

> >
> > /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

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

[Bruce Richardson]

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

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

[Konstantin Ananyev]

> 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

Yes, but AFAIK, it is not enabled by default in dpdk build, or I am missing something?

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

[Morten Brørup]

> From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> Sent: Thursday, 27 March 2025 13.10
> 
> > > >
> > > > > From: David Marchand <david.marchand@redhat.com>
> > > > > Sent: Thursday, March 27, 2025 10:37 AM
> > > > >
> > > > > 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:
> > > > > > > @@ -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.

Yes, it used to be a problem, causing bugs that were difficult to find by the developer who wrote the code.
But modern compilers warn about that, so not a problem anymore.

> Personally, I think it is a good practice to do a 'mental track' of
> your variables when writing the code,
> and having all of them defined in one place definitely helps with that.

I partially agree with this. Variables declared in the middle of a code block are difficult to keep mental track of.

However, minimizing the scope of variables reduces the mental load when reviewing code.
If some variable is only used within a block of code, it should not be declared outside that block.

As always, there are exceptions to all rules. E.g. a local variable used at the end of a code block, e.g. as a helper for temporarily cleaning up, is OK to declare there.

> Anyway, I am not about to stop people to define variables inside the
> for() if it is more convenient for them,
> but I am against to force people to write code that way.

Coding style is a balance between readability/reviewability, bug prevention and preferences.

And with our current coding style, either way of declaring "i" in this case should be acceptable.

> 
> > >
> > > /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

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

[Konstantin Ananyev]

> 
> 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;
>  }
> 
>  typedef void (*resolve_priority_t)
> diff --git a/lib/acl/acl_run_altivec.h b/lib/acl/acl_run_altivec.h
> index 2d398ffded..d5ccdb94f0 100644
> --- a/lib/acl/acl_run_altivec.h
> +++ b/lib/acl/acl_run_altivec.h
> @@ -199,10 +199,8 @@ search_altivec_8(const struct rte_acl_ctx *ctx, const uint8_t **data,
>  	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
>  		total_packets, categories, ctx->trans_table);
> 
> -	for (n = 0; n < MAX_SEARCHES_ALTIVEC8; n++) {
> -		cmplt[n].count = 0;
> +	for (n = 0; n < MAX_SEARCHES_ALTIVEC8; n++)
>  		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
> -	}
> 
>  	 /* Check for any matches. */
>  	acl_match_check_x4(0, ctx, parms, &flows, (uint64_t *)&index_array[0]);
> @@ -270,10 +268,8 @@ search_altivec_4(const struct rte_acl_ctx *ctx, const uint8_t **data,
>  	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
>  		total_packets, categories, ctx->trans_table);
> 
> -	for (n = 0; n < MAX_SEARCHES_ALTIVEC4; n++) {
> -		cmplt[n].count = 0;
> +	for (n = 0; n < MAX_SEARCHES_ALTIVEC4; n++)
>  		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
> -	}
> 
>  	/* Check for any matches. */
>  	acl_match_check_x4(0, ctx, parms, &flows, index_array);
> diff --git a/lib/acl/acl_run_avx2.h b/lib/acl/acl_run_avx2.h
> index 0b8967f22e..e069fb85b2 100644
> --- a/lib/acl/acl_run_avx2.h
> +++ b/lib/acl/acl_run_avx2.h
> @@ -171,10 +171,8 @@ search_avx2x16(const struct rte_acl_ctx *ctx, const uint8_t **data,
>  	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
>  		total_packets, categories, ctx->trans_table);
> 
> -	for (n = 0; n < RTE_DIM(cmplt); n++) {
> -		cmplt[n].count = 0;
> +	for (n = 0; n < RTE_DIM(cmplt); n++)
>  		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
> -	}
> 
>  	t0 = _mm256_set_epi64x(index_array[5], index_array[4],
>  		index_array[1], index_array[0]);
> diff --git a/lib/acl/acl_run_neon.h b/lib/acl/acl_run_neon.h
> index 63074f871d..3b9bd0cc39 100644
> --- a/lib/acl/acl_run_neon.h
> +++ b/lib/acl/acl_run_neon.h
> @@ -172,10 +172,8 @@ search_neon_8(const struct rte_acl_ctx *ctx, const uint8_t **data,
>  	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
>  		     total_packets, categories, ctx->trans_table);
> 
> -	for (n = 0; n < 8; n++) {
> -		cmplt[n].count = 0;
> +	for (n = 0; n < 8; n++)
>  		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
> -	}
> 
>  	 /* Check for any matches. */
>  	acl_match_check_x4(0, ctx, parms, &flows, &index_array[0]);
> @@ -234,10 +232,8 @@ search_neon_4(const struct rte_acl_ctx *ctx, const uint8_t **data,
>  	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
>  		     total_packets, categories, ctx->trans_table);
> 
> -	for (n = 0; n < 4; n++) {
> -		cmplt[n].count = 0;
> +	for (n = 0; n < 4; n++)
>  		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
> -	}
> 
>  	/* Check for any matches. */
>  	acl_match_check_x4(0, ctx, parms, &flows, index_array);
> diff --git a/lib/acl/acl_run_scalar.c b/lib/acl/acl_run_scalar.c
> index 3d61e79409..a3661b1b6b 100644
> --- a/lib/acl/acl_run_scalar.c
> +++ b/lib/acl/acl_run_scalar.c
> @@ -121,10 +121,8 @@ rte_acl_classify_scalar(const struct rte_acl_ctx *ctx, const uint8_t **data,
>  	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results, num,
>  		categories, ctx->trans_table);
> 
> -	for (n = 0; n < MAX_SEARCHES_SCALAR; n++) {
> -		cmplt[n].count = 0;
> +	for (n = 0; n < MAX_SEARCHES_SCALAR; n++)
>  		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
> -	}
> 
>  	transition0 = index_array[0];
>  	transition1 = index_array[1];
> diff --git a/lib/acl/acl_run_sse.h b/lib/acl/acl_run_sse.h
> index 93286a2c38..4ec819a215 100644
> --- a/lib/acl/acl_run_sse.h
> +++ b/lib/acl/acl_run_sse.h
> @@ -205,10 +205,8 @@ search_sse_8(const struct rte_acl_ctx *ctx, const uint8_t **data,
>  	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
>  		total_packets, categories, ctx->trans_table);
> 
> -	for (n = 0; n < MAX_SEARCHES_SSE8; n++) {
> -		cmplt[n].count = 0;
> +	for (n = 0; n < MAX_SEARCHES_SSE8; n++)
>  		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
> -	}
> 
>  	/*
>  	 * indices1 contains index_array[0,1]
> @@ -293,10 +291,8 @@ search_sse_4(const struct rte_acl_ctx *ctx, const uint8_t **data,
>  	acl_set_flow(&flows, cmplt, RTE_DIM(cmplt), data, results,
>  		total_packets, categories, ctx->trans_table);
> 
> -	for (n = 0; n < MAX_SEARCHES_SSE4; n++) {
> -		cmplt[n].count = 0;
> +	for (n = 0; n < MAX_SEARCHES_SSE4; n++)
>  		index_array[n] = acl_start_next_trie(&flows, parms, n, ctx);
> -	}
> 
>  	indices1 = _mm_loadu_si128((xmm_t *) &index_array[0]);
>  	indices2 = _mm_loadu_si128((xmm_t *) &index_array[2]);
> --

LGTM. Also run a quick test on my box - all seems ok. 

Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
Tested-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
 

> 2.48.1

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

[Bruce Richardson]

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>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>