diff mbox series

[v2,1/2] net/avp: fix gcc 10 maybe-uninitialized warning

Message ID 20200311113300.32487-1-ktraynor@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series [v2,1/2] net/avp: fix gcc 10 maybe-uninitialized warning |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Kevin Traynor March 11, 2020, 11:32 a.m. UTC 
  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(-)

Comments

Webster, Steven March 11, 2020, 11:56 p.m. UTC | #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>
Ferruh Yigit March 12, 2020, 1:25 p.m. UTC | #2 
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?
Kevin Traynor March 12, 2020, 2:18 p.m. UTC | #3 
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?
>
Ferruh Yigit March 12, 2020, 2:31 p.m. UTC | #4 
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?
>>
>
Kevin Traynor March 12, 2020, 3:15 p.m. UTC | #5 
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?
>>>
>>
>
Ferruh Yigit March 12, 2020, 4:47 p.m. UTC | #6 
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?
>>>>
>>>
>>
>
David Marchand May 6, 2020, 9:18 a.m. UTC | #7 
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.
diff mbox series

Patch

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];