gcc 10.0.1 reports: ../drivers/net/avp/avp_ethdev.c: In function ‘avp_xmit_scattered_pkts’: ../drivers/net/avp/avp_ethdev.c:1791:24: warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] 1791 | tx_bufs[i] = avp_bufs[count]; | ~~~~~~~~^~~~~~~ ../drivers/net/avp/avp_ethdev.c:1791:24: warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] Fix by intializing the array. Fixes: 295abce2d25b ("net/avp: add packet transmit functions") Cc: stable@dpdk.org Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- note, commit log violates line length but I didn't want to split warning msg. Cc: allain.legacy@windriver.com Cc: Steven Webster <steven.webster@windriver.com> Cc: Matt Peters <matt.peters@windriver.com> --- drivers/net/avp/avp_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c index cd747b6be..1abe96ce5 100644 --- a/drivers/net/avp/avp_ethdev.c +++ b/drivers/net/avp/avp_ethdev.c @@ -1695,5 +1695,5 @@ avp_xmit_scattered_pkts(void *tx_queue, { struct rte_avp_desc *avp_bufs[(AVP_MAX_TX_BURST * - RTE_AVP_MAX_MBUF_SEGMENTS)]; + RTE_AVP_MAX_MBUF_SEGMENTS)] = {}; struct avp_queue *txq = (struct avp_queue *)tx_queue; struct rte_avp_desc *tx_bufs[AVP_MAX_TX_BURST]; -- 2.21.1
gcc 10.0.1 reports: ../examples/ipsec-secgw/ipsec_process.c: In function ‘ipsec_process’: ../examples/ipsec-secgw/ipsec_process.c:132:34: error: ‘grp.m’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 132 | grp[n].cnt = pkts + i - grp[n].m; | ~~~~~~^~ Fix by initializing the array. Fixes: 3e5f4625dc17 ("examples/ipsec-secgw: make data-path to use IPsec library") Cc: stable@dpdk.org Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- note, commit log violates line length but I didn't want to split warning msg. Cc: konstantin.ananyev@intel.com Cc: Radu Nicolau <radu.nicolau@intel.com> Cc: Akhil Goyal <akhil.goyal@nxp.com> --- examples/ipsec-secgw/ipsec_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/ipsec-secgw/ipsec_process.c b/examples/ipsec-secgw/ipsec_process.c index bb2f2b82d..0032c5c08 100644 --- a/examples/ipsec-secgw/ipsec_process.c +++ b/examples/ipsec-secgw/ipsec_process.c @@ -287,5 +287,5 @@ ipsec_process(struct ipsec_ctx *ctx, struct ipsec_traffic *trf) struct rte_ipsec_group *pg; struct rte_ipsec_session *ips; - struct rte_ipsec_group grp[RTE_DIM(trf->ipsec.pkts)]; + struct rte_ipsec_group grp[RTE_DIM(trf->ipsec.pkts)] = {}; n = sa_group(trf->ipsec.saptr, trf->ipsec.pkts, grp, trf->ipsec.num); -- 2.21.1
>
> gcc 10.0.1 reports:
>
> ../examples/ipsec-secgw/ipsec_process.c: In function ‘ipsec_process’:
> ../examples/ipsec-secgw/ipsec_process.c:132:34:
> error: ‘grp.m’ may be used uninitialized in this function [-Werror=maybe-
> uninitialized]
> 132 | grp[n].cnt = pkts + i - grp[n].m;
> | ~~~~~~^~
>
> Fix by initializing the array.
>
> Fixes: 3e5f4625dc17 ("examples/ipsec-secgw: make data-path to use IPsec
> library")
> Cc: stable@dpdk.org
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
> note, commit log violates line length but I didn't want to split warning msg.
>
> Cc: konstantin.ananyev@intel.com
> Cc: Radu Nicolau <radu.nicolau@intel.com>
> Cc: Akhil Goyal <akhil.goyal@nxp.com>
> ---
Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
Hi Kevin, > gcc 10.0.1 reports: > > ../examples/ipsec-secgw/ipsec_process.c: In function ‘ipsec_process’: > ../examples/ipsec-secgw/ipsec_process.c:132:34: > error: ‘grp.m’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 132 | grp[n].cnt = pkts + i - grp[n].m; > | ~~~~~~^~ > > Fix by initializing the array. > > Fixes: 3e5f4625dc17 ("examples/ipsec-secgw: make data-path to use IPsec library") > Cc: stable@dpdk.org > > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > --- > note, commit log violates line length but I didn't want to split warning msg. > > Cc: konstantin.ananyev@intel.com > Cc: Radu Nicolau <radu.nicolau@intel.com> > Cc: Akhil Goyal <akhil.goyal@nxp.com> > --- > examples/ipsec-secgw/ipsec_process.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/examples/ipsec-secgw/ipsec_process.c b/examples/ipsec-secgw/ipsec_process.c > index bb2f2b82d..0032c5c08 100644 > --- a/examples/ipsec-secgw/ipsec_process.c > +++ b/examples/ipsec-secgw/ipsec_process.c > @@ -287,5 +287,5 @@ ipsec_process(struct ipsec_ctx *ctx, struct ipsec_traffic *trf) > struct rte_ipsec_group *pg; > struct rte_ipsec_session *ips; > - struct rte_ipsec_group grp[RTE_DIM(trf->ipsec.pkts)]; > + struct rte_ipsec_group grp[RTE_DIM(trf->ipsec.pkts)] = {}; Wouldn't that force to generate an extra instructions to zero-out a chunk of memory, grp pointitg to? Considering that this is perf critical pass, that's probably not a best thing. If disabling compiler warning, is not an option, then probably something like code below would help? Konstantin diff --git a/examples/ipsec-secgw/ipsec_process.c b/examples/ipsec-secgw/ipsec_process.c index bb2f2b82d..6d3a3c9a1 100644 --- a/examples/ipsec-secgw/ipsec_process.c +++ b/examples/ipsec-secgw/ipsec_process.c @@ -126,6 +126,7 @@ sa_group(void *sa_ptr[], struct rte_mbuf *pkts[], void * const nosa = &spi; sa = nosa; + grp[0].m = pkts; for (i = 0, n = 0; i != num; i++) { if (sa != sa_ptr[i]) {
On 10/03/2020 13:08, Ananyev, Konstantin wrote: > Hi Kevin, > Hi Konstantin, >> gcc 10.0.1 reports: >> >> ../examples/ipsec-secgw/ipsec_process.c: In function ‘ipsec_process’: >> ../examples/ipsec-secgw/ipsec_process.c:132:34: >> error: ‘grp.m’ may be used uninitialized in this function [-Werror=maybe-uninitialized] >> 132 | grp[n].cnt = pkts + i - grp[n].m; >> | ~~~~~~^~ >> >> Fix by initializing the array. >> >> Fixes: 3e5f4625dc17 ("examples/ipsec-secgw: make data-path to use IPsec library") >> Cc: stable@dpdk.org >> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >> --- >> note, commit log violates line length but I didn't want to split warning msg. >> >> Cc: konstantin.ananyev@intel.com >> Cc: Radu Nicolau <radu.nicolau@intel.com> >> Cc: Akhil Goyal <akhil.goyal@nxp.com> >> --- >> examples/ipsec-secgw/ipsec_process.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/examples/ipsec-secgw/ipsec_process.c b/examples/ipsec-secgw/ipsec_process.c >> index bb2f2b82d..0032c5c08 100644 >> --- a/examples/ipsec-secgw/ipsec_process.c >> +++ b/examples/ipsec-secgw/ipsec_process.c >> @@ -287,5 +287,5 @@ ipsec_process(struct ipsec_ctx *ctx, struct ipsec_traffic *trf) >> struct rte_ipsec_group *pg; >> struct rte_ipsec_session *ips; >> - struct rte_ipsec_group grp[RTE_DIM(trf->ipsec.pkts)]; >> + struct rte_ipsec_group grp[RTE_DIM(trf->ipsec.pkts)] = {}; > > Wouldn't that force to generate an extra instructions to zero-out a chunk of memory, > grp pointitg to? Yes > Considering that this is perf critical pass, that's probably not a best thing. > If disabling compiler warning, is not an option, then probably something like code > below would help? Yes, that is a nice suggestion - this will remove the warning with less instructions and LGTM. In this case we can see that the code is safe because the grp[0].cnt is written with a valid value in the second instance but I suppose disabling the warning could mask something else later. If you're ok with the approach below, I can prepare a v2 with your "Suggested-by". WDYT? Kevin. > Konstantin > > diff --git a/examples/ipsec-secgw/ipsec_process.c b/examples/ipsec-secgw/ipsec_process.c > index bb2f2b82d..6d3a3c9a1 100644 > --- a/examples/ipsec-secgw/ipsec_process.c > +++ b/examples/ipsec-secgw/ipsec_process.c > @@ -126,6 +126,7 @@ sa_group(void *sa_ptr[], struct rte_mbuf *pkts[], > void * const nosa = &spi; > > sa = nosa; > + grp[0].m = pkts; > for (i = 0, n = 0; i != num; i++) { > > if (sa != sa_ptr[i]) { >
> >> gcc 10.0.1 reports: > >> > >> ../examples/ipsec-secgw/ipsec_process.c: In function ‘ipsec_process’: > >> ../examples/ipsec-secgw/ipsec_process.c:132:34: > >> error: ‘grp.m’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > >> 132 | grp[n].cnt = pkts + i - grp[n].m; > >> | ~~~~~~^~ > >> > >> Fix by initializing the array. > >> > >> Fixes: 3e5f4625dc17 ("examples/ipsec-secgw: make data-path to use IPsec library") > >> Cc: stable@dpdk.org > >> > >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > >> --- > >> note, commit log violates line length but I didn't want to split warning msg. > >> > >> Cc: konstantin.ananyev@intel.com > >> Cc: Radu Nicolau <radu.nicolau@intel.com> > >> Cc: Akhil Goyal <akhil.goyal@nxp.com> > >> --- > >> examples/ipsec-secgw/ipsec_process.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/examples/ipsec-secgw/ipsec_process.c b/examples/ipsec-secgw/ipsec_process.c > >> index bb2f2b82d..0032c5c08 100644 > >> --- a/examples/ipsec-secgw/ipsec_process.c > >> +++ b/examples/ipsec-secgw/ipsec_process.c > >> @@ -287,5 +287,5 @@ ipsec_process(struct ipsec_ctx *ctx, struct ipsec_traffic *trf) > >> struct rte_ipsec_group *pg; > >> struct rte_ipsec_session *ips; > >> - struct rte_ipsec_group grp[RTE_DIM(trf->ipsec.pkts)]; > >> + struct rte_ipsec_group grp[RTE_DIM(trf->ipsec.pkts)] = {}; > > > > Wouldn't that force to generate an extra instructions to zero-out a chunk of memory, > > grp pointitg to? > > Yes > > > Considering that this is perf critical pass, that's probably not a best thing. > > If disabling compiler warning, is not an option, then probably something like code > > below would help? > > Yes, that is a nice suggestion - this will remove the warning with less > instructions and LGTM. > > In this case we can see that the code is safe because the grp[0].cnt is > written with a valid value in the second instance but I suppose > disabling the warning could mask something else later. > > If you're ok with the approach below, I can prepare a v2 with your > "Suggested-by". WDYT? Sounds like a good plan to me. Thanks Konstantin > > Kevin. > > > Konstantin > > > > diff --git a/examples/ipsec-secgw/ipsec_process.c b/examples/ipsec-secgw/ipsec_process.c > > index bb2f2b82d..6d3a3c9a1 100644 > > --- a/examples/ipsec-secgw/ipsec_process.c > > +++ b/examples/ipsec-secgw/ipsec_process.c > > @@ -126,6 +126,7 @@ sa_group(void *sa_ptr[], struct rte_mbuf *pkts[], > > void * const nosa = &spi; > > > > sa = nosa; > > + grp[0].m = pkts; > > for (i = 0, n = 0; i != num; i++) { > > > > if (sa != sa_ptr[i]) { > >
gcc 10.0.1 reports: ../drivers/net/avp/avp_ethdev.c: In function ‘avp_xmit_scattered_pkts’: ../drivers/net/avp/avp_ethdev.c:1791:24: warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] 1791 | tx_bufs[i] = avp_bufs[count]; | ~~~~~~~~^~~~~~~ ../drivers/net/avp/avp_ethdev.c:1791:24: warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] Fix by intializing the array. Fixes: 295abce2d25b ("net/avp: add packet transmit functions") Cc: stable@dpdk.org Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- v2: no change note, commit log violates line length but I didn't want to split warning msg. Cc: allain.legacy@windriver.com Cc: Steven Webster <steven.webster@windriver.com> Cc: Matt Peters <matt.peters@windriver.com> --- drivers/net/avp/avp_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c index cd747b6be..1abe96ce5 100644 --- a/drivers/net/avp/avp_ethdev.c +++ b/drivers/net/avp/avp_ethdev.c @@ -1695,5 +1695,5 @@ avp_xmit_scattered_pkts(void *tx_queue, { struct rte_avp_desc *avp_bufs[(AVP_MAX_TX_BURST * - RTE_AVP_MAX_MBUF_SEGMENTS)]; + RTE_AVP_MAX_MBUF_SEGMENTS)] = {}; struct avp_queue *txq = (struct avp_queue *)tx_queue; struct rte_avp_desc *tx_bufs[AVP_MAX_TX_BURST]; -- 2.21.1
gcc 10.0.1 reports: ../examples/ipsec-secgw/ipsec_process.c: In function ‘ipsec_process’: ../examples/ipsec-secgw/ipsec_process.c:132:34: error: ‘grp.m’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 132 | grp[n].cnt = pkts + i - grp[n].m; | ~~~~~~^~ This is a correct warning for the initial execution of the statement. However, it is the design of the loop that grp[0].cnt will later be written with the correct value using an initialized grp[0].m before it is used. In order to remove the warning, initialize grp[0].m for the initial and unused calculation of grp[0].cnt. Fixes: 3e5f4625dc17 ("examples/ipsec-secgw: make data-path to use IPsec library") Cc: stable@dpdk.org Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- v2: just initialize grp[0].m instead of the full array. note, commit log violates line length but I didn't want to split warning msg. Cc: konstantin.ananyev@intel.com Cc: Radu Nicolau <radu.nicolau@intel.com> Cc: Akhil Goyal <akhil.goyal@nxp.com> --- examples/ipsec-secgw/ipsec_process.c | 1 + 1 file changed, 1 insertion(+) diff --git a/examples/ipsec-secgw/ipsec_process.c b/examples/ipsec-secgw/ipsec_process.c index bb2f2b82d..6d3a3c9a1 100644 --- a/examples/ipsec-secgw/ipsec_process.c +++ b/examples/ipsec-secgw/ipsec_process.c @@ -127,4 +127,5 @@ sa_group(void *sa_ptr[], struct rte_mbuf *pkts[], sa = nosa; + grp[0].m = pkts; for (i = 0, n = 0; i != num; i++) { -- 2.21.1
> -----Original Message----- > From: Kevin Traynor <ktraynor@redhat.com> > Sent: Wednesday, March 11, 2020 11:33 AM > To: dev@dpdk.org > Cc: Kevin Traynor <ktraynor@redhat.com>; stable@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Nicolau, Radu > <radu.nicolau@intel.com>; Akhil Goyal <akhil.goyal@nxp.com> > Subject: [PATCH v2 2/2] examples/ipsec-gw: fix gcc 10 maybe-uninitialized warning > > gcc 10.0.1 reports: > > ../examples/ipsec-secgw/ipsec_process.c: In function ‘ipsec_process’: > ../examples/ipsec-secgw/ipsec_process.c:132:34: > error: ‘grp.m’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 132 | grp[n].cnt = pkts + i - grp[n].m; > | ~~~~~~^~ > > This is a correct warning for the initial execution of the statement. > However, it is the design of the loop that grp[0].cnt will later be > written with the correct value using an initialized grp[0].m before it > is used. > > In order to remove the warning, initialize grp[0].m for the initial and > unused calculation of grp[0].cnt. > > Fixes: 3e5f4625dc17 ("examples/ipsec-secgw: make data-path to use IPsec library") > Cc: stable@dpdk.org > > Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com> > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > --- > v2: just initialize grp[0].m instead of the full array. > > note, commit log violates line length but I didn't want to split warning msg. > > Cc: konstantin.ananyev@intel.com > Cc: Radu Nicolau <radu.nicolau@intel.com> > Cc: Akhil Goyal <akhil.goyal@nxp.com> > --- > examples/ipsec-secgw/ipsec_process.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/examples/ipsec-secgw/ipsec_process.c b/examples/ipsec-secgw/ipsec_process.c > index bb2f2b82d..6d3a3c9a1 100644 > --- a/examples/ipsec-secgw/ipsec_process.c > +++ b/examples/ipsec-secgw/ipsec_process.c > @@ -127,4 +127,5 @@ sa_group(void *sa_ptr[], struct rte_mbuf *pkts[], > > sa = nosa; > + grp[0].m = pkts; > for (i = 0, n = 0; i != num; i++) { > > -- Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com> > 2.21.1
> gcc 10.0.1 reports:
>
> ../drivers/net/avp/avp_ethdev.c: In function ‘avp_xmit_scattered_pkts’:
> ../drivers/net/avp/avp_ethdev.c:1791:24:
> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-
> Wmaybe-uninitialized]
> 1791 | tx_bufs[i] = avp_bufs[count];
> | ~~~~~~~~^~~~~~~
> ../drivers/net/avp/avp_ethdev.c:1791:24:
> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-
> Wmaybe-uninitialized]
>
> Fix by intializing the array.
>
> Fixes: 295abce2d25b ("net/avp: add packet transmit functions")
> Cc: stable@dpdk.org
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
Acked-by: Steven Webster <steven.webster@windriver.com>
On 3/11/2020 11:32 AM, Kevin Traynor wrote:
> gcc 10.0.1 reports:
>
> ../drivers/net/avp/avp_ethdev.c: In function ‘avp_xmit_scattered_pkts’:
> ../drivers/net/avp/avp_ethdev.c:1791:24:
> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 1791 | tx_bufs[i] = avp_bufs[count];
> | ~~~~~~~~^~~~~~~
> ../drivers/net/avp/avp_ethdev.c:1791:24:
> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> Fix by intializing the array.
>
> Fixes: 295abce2d25b ("net/avp: add packet transmit functions")
> Cc: stable@dpdk.org
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> ---
> v2: no change
>
> note, commit log violates line length but I didn't want to split warning msg.
>
> Cc: allain.legacy@windriver.com
> Cc: Steven Webster <steven.webster@windriver.com>
> Cc: Matt Peters <matt.peters@windriver.com>
> ---
> drivers/net/avp/avp_ethdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c
> index cd747b6be..1abe96ce5 100644
> --- a/drivers/net/avp/avp_ethdev.c
> +++ b/drivers/net/avp/avp_ethdev.c
> @@ -1695,5 +1695,5 @@ avp_xmit_scattered_pkts(void *tx_queue,
> {
> struct rte_avp_desc *avp_bufs[(AVP_MAX_TX_BURST *
> - RTE_AVP_MAX_MBUF_SEGMENTS)];
> + RTE_AVP_MAX_MBUF_SEGMENTS)] = {};
> struct avp_queue *txq = (struct avp_queue *)tx_queue;
> struct rte_avp_desc *tx_bufs[AVP_MAX_TX_BURST];
>
Isn't this a false positive, can there be any case 'avp_bufs[]' used
uninitialized? Or am I missing something.
If this is false positive, does it worth to report to issue to gcc?
On 12/03/2020 13:25, Ferruh Yigit wrote: > On 3/11/2020 11:32 AM, Kevin Traynor wrote: >> gcc 10.0.1 reports: >> >> ../drivers/net/avp/avp_ethdev.c: In function ‘avp_xmit_scattered_pkts’: >> ../drivers/net/avp/avp_ethdev.c:1791:24: >> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] >> 1791 | tx_bufs[i] = avp_bufs[count]; >> | ~~~~~~~~^~~~~~~ >> ../drivers/net/avp/avp_ethdev.c:1791:24: >> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] >> >> Fix by intializing the array. >> >> Fixes: 295abce2d25b ("net/avp: add packet transmit functions") >> Cc: stable@dpdk.org >> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >> --- >> v2: no change >> >> note, commit log violates line length but I didn't want to split warning msg. >> >> Cc: allain.legacy@windriver.com >> Cc: Steven Webster <steven.webster@windriver.com> >> Cc: Matt Peters <matt.peters@windriver.com> >> --- >> drivers/net/avp/avp_ethdev.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c >> index cd747b6be..1abe96ce5 100644 >> --- a/drivers/net/avp/avp_ethdev.c >> +++ b/drivers/net/avp/avp_ethdev.c >> @@ -1695,5 +1695,5 @@ avp_xmit_scattered_pkts(void *tx_queue, >> { >> struct rte_avp_desc *avp_bufs[(AVP_MAX_TX_BURST * >> - RTE_AVP_MAX_MBUF_SEGMENTS)]; >> + RTE_AVP_MAX_MBUF_SEGMENTS)] = {}; >> struct avp_queue *txq = (struct avp_queue *)tx_queue; >> struct rte_avp_desc *tx_bufs[AVP_MAX_TX_BURST]; >> > > Isn't this a false positive, can there be any case 'avp_bufs[]' used > uninitialized? Or am I missing something. > I presume it's because it's not being initialized in the fn and there is some paths in fn's it's passed to that don't initialize it. Of course in practice with "normal" values this might not happen. > If this is false positive, does it worth to report to issue to gcc? >
On 3/12/2020 2:18 PM, Kevin Traynor wrote: > On 12/03/2020 13:25, Ferruh Yigit wrote: >> On 3/11/2020 11:32 AM, Kevin Traynor wrote: >>> gcc 10.0.1 reports: >>> >>> ../drivers/net/avp/avp_ethdev.c: In function ‘avp_xmit_scattered_pkts’: >>> ../drivers/net/avp/avp_ethdev.c:1791:24: >>> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] >>> 1791 | tx_bufs[i] = avp_bufs[count]; >>> | ~~~~~~~~^~~~~~~ >>> ../drivers/net/avp/avp_ethdev.c:1791:24: >>> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] >>> >>> Fix by intializing the array. >>> >>> Fixes: 295abce2d25b ("net/avp: add packet transmit functions") >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >>> --- >>> v2: no change >>> >>> note, commit log violates line length but I didn't want to split warning msg. >>> >>> Cc: allain.legacy@windriver.com >>> Cc: Steven Webster <steven.webster@windriver.com> >>> Cc: Matt Peters <matt.peters@windriver.com> >>> --- >>> drivers/net/avp/avp_ethdev.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c >>> index cd747b6be..1abe96ce5 100644 >>> --- a/drivers/net/avp/avp_ethdev.c >>> +++ b/drivers/net/avp/avp_ethdev.c >>> @@ -1695,5 +1695,5 @@ avp_xmit_scattered_pkts(void *tx_queue, >>> { >>> struct rte_avp_desc *avp_bufs[(AVP_MAX_TX_BURST * >>> - RTE_AVP_MAX_MBUF_SEGMENTS)]; >>> + RTE_AVP_MAX_MBUF_SEGMENTS)] = {}; >>> struct avp_queue *txq = (struct avp_queue *)tx_queue; >>> struct rte_avp_desc *tx_bufs[AVP_MAX_TX_BURST]; >>> >> >> Isn't this a false positive, can there be any case 'avp_bufs[]' used >> uninitialized? Or am I missing something. >> > > I presume it's because it's not being initialized in the fn and there is > some paths in fn's it's passed to that don't initialize it. Of course in > practice with "normal" values this might not happen. 'avp_fifo_get(alloc_q, (void **)&avp_bufs, segments);' initializes it, and I am not just talking about 'normal' case, I don't see any case that 'avp_bufs[]' used uninitialized, can you see any? > >> If this is false positive, does it worth to report to issue to gcc? >> >
On 12/03/2020 14:31, Ferruh Yigit wrote: > On 3/12/2020 2:18 PM, Kevin Traynor wrote: >> On 12/03/2020 13:25, Ferruh Yigit wrote: >>> On 3/11/2020 11:32 AM, Kevin Traynor wrote: >>>> gcc 10.0.1 reports: >>>> >>>> ../drivers/net/avp/avp_ethdev.c: In function ‘avp_xmit_scattered_pkts’: >>>> ../drivers/net/avp/avp_ethdev.c:1791:24: >>>> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] >>>> 1791 | tx_bufs[i] = avp_bufs[count]; >>>> | ~~~~~~~~^~~~~~~ >>>> ../drivers/net/avp/avp_ethdev.c:1791:24: >>>> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] >>>> >>>> Fix by intializing the array. >>>> >>>> Fixes: 295abce2d25b ("net/avp: add packet transmit functions") >>>> Cc: stable@dpdk.org >>>> >>>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >>>> --- >>>> v2: no change >>>> >>>> note, commit log violates line length but I didn't want to split warning msg. >>>> >>>> Cc: allain.legacy@windriver.com >>>> Cc: Steven Webster <steven.webster@windriver.com> >>>> Cc: Matt Peters <matt.peters@windriver.com> >>>> --- >>>> drivers/net/avp/avp_ethdev.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c >>>> index cd747b6be..1abe96ce5 100644 >>>> --- a/drivers/net/avp/avp_ethdev.c >>>> +++ b/drivers/net/avp/avp_ethdev.c >>>> @@ -1695,5 +1695,5 @@ avp_xmit_scattered_pkts(void *tx_queue, >>>> { >>>> struct rte_avp_desc *avp_bufs[(AVP_MAX_TX_BURST * >>>> - RTE_AVP_MAX_MBUF_SEGMENTS)]; >>>> + RTE_AVP_MAX_MBUF_SEGMENTS)] = {}; >>>> struct avp_queue *txq = (struct avp_queue *)tx_queue; >>>> struct rte_avp_desc *tx_bufs[AVP_MAX_TX_BURST]; >>>> >>> >>> Isn't this a false positive, can there be any case 'avp_bufs[]' used >>> uninitialized? Or am I missing something. >>> >> >> I presume it's because it's not being initialized in the fn and there is >> some paths in fn's it's passed to that don't initialize it. Of course in >> practice with "normal" values this might not happen. > > 'avp_fifo_get(alloc_q, (void **)&avp_bufs, segments);' initializes it, and I am > not just talking about 'normal' case, I don't see any case that 'avp_bufs[]' > used uninitialized, can you see any? > Well, it's initialization there is dependent on not hitting the first return and the loop executing. >> >>> If this is false positive, does it worth to report to issue to gcc? >>> >> >
On 3/12/2020 3:15 PM, Kevin Traynor wrote: > On 12/03/2020 14:31, Ferruh Yigit wrote: >> On 3/12/2020 2:18 PM, Kevin Traynor wrote: >>> On 12/03/2020 13:25, Ferruh Yigit wrote: >>>> On 3/11/2020 11:32 AM, Kevin Traynor wrote: >>>>> gcc 10.0.1 reports: >>>>> >>>>> ../drivers/net/avp/avp_ethdev.c: In function ‘avp_xmit_scattered_pkts’: >>>>> ../drivers/net/avp/avp_ethdev.c:1791:24: >>>>> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] >>>>> 1791 | tx_bufs[i] = avp_bufs[count]; >>>>> | ~~~~~~~~^~~~~~~ >>>>> ../drivers/net/avp/avp_ethdev.c:1791:24: >>>>> warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized] >>>>> >>>>> Fix by intializing the array. >>>>> >>>>> Fixes: 295abce2d25b ("net/avp: add packet transmit functions") >>>>> Cc: stable@dpdk.org >>>>> >>>>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >>>>> --- >>>>> v2: no change >>>>> >>>>> note, commit log violates line length but I didn't want to split warning msg. >>>>> >>>>> Cc: allain.legacy@windriver.com >>>>> Cc: Steven Webster <steven.webster@windriver.com> >>>>> Cc: Matt Peters <matt.peters@windriver.com> >>>>> --- >>>>> drivers/net/avp/avp_ethdev.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c >>>>> index cd747b6be..1abe96ce5 100644 >>>>> --- a/drivers/net/avp/avp_ethdev.c >>>>> +++ b/drivers/net/avp/avp_ethdev.c >>>>> @@ -1695,5 +1695,5 @@ avp_xmit_scattered_pkts(void *tx_queue, >>>>> { >>>>> struct rte_avp_desc *avp_bufs[(AVP_MAX_TX_BURST * >>>>> - RTE_AVP_MAX_MBUF_SEGMENTS)]; >>>>> + RTE_AVP_MAX_MBUF_SEGMENTS)] = {}; >>>>> struct avp_queue *txq = (struct avp_queue *)tx_queue; >>>>> struct rte_avp_desc *tx_bufs[AVP_MAX_TX_BURST]; >>>>> >>>> >>>> Isn't this a false positive, can there be any case 'avp_bufs[]' used >>>> uninitialized? Or am I missing something. >>>> >>> >>> I presume it's because it's not being initialized in the fn and there is >>> some paths in fn's it's passed to that don't initialize it. Of course in >>> practice with "normal" values this might not happen. >> >> 'avp_fifo_get(alloc_q, (void **)&avp_bufs, segments);' initializes it, and I am >> not just talking about 'normal' case, I don't see any case that 'avp_bufs[]' >> used uninitialized, can you see any? >> > > Well, it's initialization there is dependent on not hitting the first > return and the loop executing. If whole array not initialized, the next line, 'if (unlikely(n != segments))', will catch it and function return without using 'avp_bufs[]' at all. Anyway, as I said I can't see a case that 'avp_bufs[]' used uninitialized, and not sure about additional zeroing out in datapath function if this is a false positive, but if windriver guys are OK I won't object. > >>> >>>> If this is false positive, does it worth to report to issue to gcc? >>>> >>> >> >
On Thu, Mar 12, 2020 at 12:56 AM Webster, Steven
<Steven.Webster@windriver.com> wrote:
>
> > gcc 10.0.1 reports:
> >
> > ../drivers/net/avp/avp_ethdev.c: In function ‘avp_xmit_scattered_pkts’:
> > ../drivers/net/avp/avp_ethdev.c:1791:24:
> > warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-
> > Wmaybe-uninitialized]
> > 1791 | tx_bufs[i] = avp_bufs[count];
> > | ~~~~~~~~^~~~~~~
> > ../drivers/net/avp/avp_ethdev.c:1791:24:
> > warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-
> > Wmaybe-uninitialized]
> >
> > Fix by intializing the array.
> >
> > Fixes: 295abce2d25b ("net/avp: add packet transmit functions")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> > ---
> Acked-by: Steven Webster <steven.webster@windriver.com>
Applied, thanks.
--
David Marchand
On Wed, Mar 11, 2020 at 1:05 PM Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
> > From: Kevin Traynor <ktraynor@redhat.com>
> > Sent: Wednesday, March 11, 2020 11:33 AM
> > To: dev@dpdk.org
> > Cc: Kevin Traynor <ktraynor@redhat.com>; stable@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Nicolau, Radu
> > <radu.nicolau@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>
> > Subject: [PATCH v2 2/2] examples/ipsec-gw: fix gcc 10 maybe-uninitialized warning
> >
> > gcc 10.0.1 reports:
> >
> > ../examples/ipsec-secgw/ipsec_process.c: In function ‘ipsec_process’:
> > ../examples/ipsec-secgw/ipsec_process.c:132:34:
> > error: ‘grp.m’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > 132 | grp[n].cnt = pkts + i - grp[n].m;
> > | ~~~~~~^~
> >
> > This is a correct warning for the initial execution of the statement.
> > However, it is the design of the loop that grp[0].cnt will later be
> > written with the correct value using an initialized grp[0].m before it
> > is used.
> >
> > In order to remove the warning, initialize grp[0].m for the initial and
> > unused calculation of grp[0].cnt.
> >
> > Fixes: 3e5f4625dc17 ("examples/ipsec-secgw: make data-path to use IPsec library")
> > Cc: stable@dpdk.org
> >
> > Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Applied, thanks.
--
David Marchand