[dpdk-dev] ixgbe: fix build with gcc 4.4

  With GCC 4.4.7 from CentOS 6.5, the following errors arise:

lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_dev_rx_queue_setup’:
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for ‘dev_info.driver_name’)

lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_set_rsc’:
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for ‘dev_info.driver_name’)

lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_recv_pkts_lro_single_alloc’:
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ‘next_rsc_entry’ may be used uninitialized in this function
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ‘next_rxe’ may be used uninitialized in this function

Fixes: 8eecb3295aed ("ixgbe: add LRO support")

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
  

On 04/14/15 12:31, Thomas Monjalon wrote:
> With GCC 4.4.7 from CentOS 6.5, the following errors arise:
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_dev_rx_queue_setup’:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for ‘dev_info.driver_name’)
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_set_rsc’:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for ‘dev_info.driver_name’)
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_recv_pkts_lro_single_alloc’:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ‘next_rsc_entry’ may be used uninitialized in this function
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ‘next_rxe’ may be used uninitialized in this function
>
> Fixes: 8eecb3295aed ("ixgbe: add LRO support")
>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---
>   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index f1da9ec..a2b8631 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
>   		bool eop;
>   		struct ixgbe_rx_entry *rxe;
>   		struct ixgbe_rsc_entry *rsc_entry;
> -		struct ixgbe_rsc_entry *next_rsc_entry;
> -		struct ixgbe_rx_entry *next_rxe;
> +		struct ixgbe_rsc_entry *next_rsc_entry = NULL;
> +		struct ixgbe_rx_entry *next_rxe = NULL;
>   		struct rte_mbuf *first_seg;
>   		struct rte_mbuf *rxm;
>   		struct rte_mbuf *nmb;
> @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
>   	struct ixgbe_rx_queue *rxq;
>   	struct ixgbe_hw     *hw;
>   	uint16_t len;
> -	struct rte_eth_dev_info dev_info = { 0 };
> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>   	struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;
>   	bool rsc_requested = false;
>   
> @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
>   {
>   	struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
>   	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> -	struct rte_eth_dev_info dev_info = { 0 };
> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };

Hmmm... Unless I miss something this and one above would zero only a 
single field - "max_rx_queues"; and would leave the rest uninitialized.
The original code intend to zero the whole struct. The alternative to 
the original lines could be usage of memset().

>   	bool rsc_capable = false;
>   	uint16_t i;
>   	uint32_t rdrxctl;
  

On 04/14/15 12:31, Thomas Monjalon wrote:
> With GCC 4.4.7 from CentOS 6.5, the following errors arise:
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_dev_rx_queue_setup’:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for ‘dev_info.driver_name’)
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_set_rsc’:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for ‘dev_info.driver_name’)
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_recv_pkts_lro_single_alloc’:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ‘next_rsc_entry’ may be used uninitialized in this function
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ‘next_rxe’ may be used uninitialized in this function

:D Looks like a gcc bug ;) Both are set and only after that (!!!) used 
under "!eop" condition.

>
> Fixes: 8eecb3295aed ("ixgbe: add LRO support")
>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---
>   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index f1da9ec..a2b8631 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
>   		bool eop;
>   		struct ixgbe_rx_entry *rxe;
>   		struct ixgbe_rsc_entry *rsc_entry;
> -		struct ixgbe_rsc_entry *next_rsc_entry;
> -		struct ixgbe_rx_entry *next_rxe;
> +		struct ixgbe_rsc_entry *next_rsc_entry = NULL;
> +		struct ixgbe_rx_entry *next_rxe = NULL;
>   		struct rte_mbuf *first_seg;
>   		struct rte_mbuf *rxm;
>   		struct rte_mbuf *nmb;
> @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
>   	struct ixgbe_rx_queue *rxq;
>   	struct ixgbe_hw     *hw;
>   	uint16_t len;
> -	struct rte_eth_dev_info dev_info = { 0 };
> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>   	struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;
>   	bool rsc_requested = false;
>   
> @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
>   {
>   	struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
>   	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> -	struct rte_eth_dev_info dev_info = { 0 };
> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>   	bool rsc_capable = false;
>   	uint16_t i;
>   	uint32_t rdrxctl;
  

> -----Original Message-----

> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]

> Sent: Tuesday, April 14, 2015 1:49 PM

> To: Thomas Monjalon; Ananyev, Konstantin; Zhang, Helin

> Cc: dev@dpdk.org

> Subject: Re: [PATCH] ixgbe: fix build with gcc 4.4

> 

> 

> 

> On 04/14/15 12:31, Thomas Monjalon wrote:

> > With GCC 4.4.7 from CentOS 6.5, the following errors arise:

> >

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_dev_rx_queue_setup’:

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for ‘dev_info.driver_name’)

> >

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_set_rsc’:

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for ‘dev_info.driver_name’)

> >

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_recv_pkts_lro_single_alloc’:

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ‘next_rsc_entry’ may be used uninitialized in this function

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ‘next_rxe’ may be used uninitialized in this function

> >

> > Fixes: 8eecb3295aed ("ixgbe: add LRO support")

> >

> > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>

> > ---

> >   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 ++++----

> >   1 file changed, 4 insertions(+), 4 deletions(-)

> >

> > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c

> > index f1da9ec..a2b8631 100644

> > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c

> > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c

> > @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,

> >   		bool eop;

> >   		struct ixgbe_rx_entry *rxe;

> >   		struct ixgbe_rsc_entry *rsc_entry;

> > -		struct ixgbe_rsc_entry *next_rsc_entry;

> > -		struct ixgbe_rx_entry *next_rxe;

> > +		struct ixgbe_rsc_entry *next_rsc_entry = NULL;

> > +		struct ixgbe_rx_entry *next_rxe = NULL;

> >   		struct rte_mbuf *first_seg;

> >   		struct rte_mbuf *rxm;

> >   		struct rte_mbuf *nmb;

> > @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,

> >   	struct ixgbe_rx_queue *rxq;

> >   	struct ixgbe_hw     *hw;

> >   	uint16_t len;

> > -	struct rte_eth_dev_info dev_info = { 0 };

> > +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };

> >   	struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;

> >   	bool rsc_requested = false;

> >

> > @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)

> >   {

> >   	struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;

> >   	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);

> > -	struct rte_eth_dev_info dev_info = { 0 };

> > +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };

> 

> Hmmm... Unless I miss something this and one above would zero only a

> single field - "max_rx_queues"; and would leave the rest uninitialized.

> The original code intend to zero the whole struct. The alternative to

> the original lines could be usage of memset().


As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
So I think we are ok here.
 
> 

> >   	bool rsc_capable = false;

> >   	uint16_t i;

> >   	uint32_t rdrxctl;
  

> -----Original Message-----

> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]

> Sent: Tuesday, April 14, 2015 1:52 PM

> To: Thomas Monjalon; Ananyev, Konstantin; Zhang, Helin

> Cc: dev@dpdk.org

> Subject: Re: [PATCH] ixgbe: fix build with gcc 4.4

> 

> 

> 

> On 04/14/15 12:31, Thomas Monjalon wrote:

> > With GCC 4.4.7 from CentOS 6.5, the following errors arise:

> >

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_dev_rx_queue_setup’:

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for ‘dev_info.driver_name’)

> >

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_set_rsc’:

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for ‘dev_info.driver_name’)

> >

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_recv_pkts_lro_single_alloc’:

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ‘next_rsc_entry’ may be used uninitialized in this function

> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ‘next_rxe’ may be used uninitialized in this function

> 

> :D Looks like a gcc bug ;) Both are set and only after that (!!!) used

> under "!eop" condition.


Possibly, but we still need to make it build cleanly.
Konstantin

> 

> >

> > Fixes: 8eecb3295aed ("ixgbe: add LRO support")

> >

> > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>

> > ---

> >   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 ++++----

> >   1 file changed, 4 insertions(+), 4 deletions(-)

> >

> > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c

> > index f1da9ec..a2b8631 100644

> > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c

> > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c

> > @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,

> >   		bool eop;

> >   		struct ixgbe_rx_entry *rxe;

> >   		struct ixgbe_rsc_entry *rsc_entry;

> > -		struct ixgbe_rsc_entry *next_rsc_entry;

> > -		struct ixgbe_rx_entry *next_rxe;

> > +		struct ixgbe_rsc_entry *next_rsc_entry = NULL;

> > +		struct ixgbe_rx_entry *next_rxe = NULL;

> >   		struct rte_mbuf *first_seg;

> >   		struct rte_mbuf *rxm;

> >   		struct rte_mbuf *nmb;

> > @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,

> >   	struct ixgbe_rx_queue *rxq;

> >   	struct ixgbe_hw     *hw;

> >   	uint16_t len;

> > -	struct rte_eth_dev_info dev_info = { 0 };

> > +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };

> >   	struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;

> >   	bool rsc_requested = false;

> >

> > @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)

> >   {

> >   	struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;

> >   	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);

> > -	struct rte_eth_dev_info dev_info = { 0 };

> > +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };

> >   	bool rsc_capable = false;

> >   	uint16_t i;

> >   	uint32_t rdrxctl;
  

On 04/14/15 16:06, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>> Sent: Tuesday, April 14, 2015 1:49 PM
>> To: Thomas Monjalon; Ananyev, Konstantin; Zhang, Helin
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH] ixgbe: fix build with gcc 4.4
>>
>>
>>
>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>> With GCC 4.4.7 from CentOS 6.5, the following errors arise:
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_dev_rx_queue_setup’:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for ‘dev_info.driver_name’)
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_set_rsc’:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for ‘dev_info.driver_name’)
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_recv_pkts_lro_single_alloc’:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ‘next_rsc_entry’ may be used uninitialized in this function
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ‘next_rxe’ may be used uninitialized in this function
>>>
>>> Fixes: 8eecb3295aed ("ixgbe: add LRO support")
>>>
>>> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
>>> ---
>>>    lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> index f1da9ec..a2b8631 100644
>>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
>>>    		bool eop;
>>>    		struct ixgbe_rx_entry *rxe;
>>>    		struct ixgbe_rsc_entry *rsc_entry;
>>> -		struct ixgbe_rsc_entry *next_rsc_entry;
>>> -		struct ixgbe_rx_entry *next_rxe;
>>> +		struct ixgbe_rsc_entry *next_rsc_entry = NULL;
>>> +		struct ixgbe_rx_entry *next_rxe = NULL;
>>>    		struct rte_mbuf *first_seg;
>>>    		struct rte_mbuf *rxm;
>>>    		struct rte_mbuf *nmb;
>>> @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
>>>    	struct ixgbe_rx_queue *rxq;
>>>    	struct ixgbe_hw     *hw;
>>>    	uint16_t len;
>>> -	struct rte_eth_dev_info dev_info = { 0 };
>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>>    	struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;
>>>    	bool rsc_requested = false;
>>>
>>> @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
>>>    {
>>>    	struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
>>>    	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>> -	struct rte_eth_dev_info dev_info = { 0 };
>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>> Hmmm... Unless I miss something this and one above would zero only a
>> single field - "max_rx_queues"; and would leave the rest uninitialized.
>> The original code intend to zero the whole struct. The alternative to
>> the original lines could be usage of memset().
> As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
> So I think we are ok here.

Yeah, I guess it does zero-initializes the rest 
(https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I 
don't understand how the above change fixes the error if it complains 
about the dev_info.driver_name?

What I'm trying to say - the proposed fix is completely unclear and 
confusing. Think of somebody reading this line in a month from today - 
he wouldn't get a clue why is it there, why to explicitly set 
max_rx_queues to zero and leave the rest be zeroed automatically... Why 
to add such artifacts to the code instead of just zeroing the struct 
with a memset() and putting a good clear comment above it explaining why 
we use a memset() and not and initializer?

>   
>>>    	bool rsc_capable = false;
>>>    	uint16_t i;
>>>    	uint32_t rdrxctl;
  

On 04/14/15 16:23, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>> Sent: Tuesday, April 14, 2015 1:52 PM
>> To: Thomas Monjalon; Ananyev, Konstantin; Zhang, Helin
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH] ixgbe: fix build with gcc 4.4
>>
>>
>>
>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>> With GCC 4.4.7 from CentOS 6.5, the following errors arise:
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_dev_rx_queue_setup’:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for ‘dev_info.driver_name’)
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_set_rsc’:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for ‘dev_info.driver_name’)
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ‘ixgbe_recv_pkts_lro_single_alloc’:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ‘next_rsc_entry’ may be used uninitialized in this function
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ‘next_rxe’ may be used uninitialized in this function
>> :D Looks like a gcc bug ;) Both are set and only after that (!!!) used
>> under "!eop" condition.
> Possibly, but we still need to make it build cleanly.

It's clearly - I was just trying to be polite here... ;)
Please, add the comment explaining this initialization so that nobody 
removes these workarounds by mistake...

> Konstantin
>
>>> Fixes: 8eecb3295aed ("ixgbe: add LRO support")
>>>
>>> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
>>> ---
>>>    lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> index f1da9ec..a2b8631 100644
>>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
>>>    		bool eop;
>>>    		struct ixgbe_rx_entry *rxe;
>>>    		struct ixgbe_rsc_entry *rsc_entry;
>>> -		struct ixgbe_rsc_entry *next_rsc_entry;
>>> -		struct ixgbe_rx_entry *next_rxe;
>>> +		struct ixgbe_rsc_entry *next_rsc_entry = NULL;
>>> +		struct ixgbe_rx_entry *next_rxe = NULL;
>>>    		struct rte_mbuf *first_seg;
>>>    		struct rte_mbuf *rxm;
>>>    		struct rte_mbuf *nmb;
>>> @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
>>>    	struct ixgbe_rx_queue *rxq;
>>>    	struct ixgbe_hw     *hw;
>>>    	uint16_t len;
>>> -	struct rte_eth_dev_info dev_info = { 0 };
>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>>    	struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;
>>>    	bool rsc_requested = false;
>>>
>>> @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
>>>    {
>>>    	struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
>>>    	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>> -	struct rte_eth_dev_info dev_info = { 0 };
>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>>    	bool rsc_capable = false;
>>>    	uint16_t i;
>>>    	uint32_t rdrxctl;
  

2015-04-14 16:38, Vlad Zolotarov:
> On 04/14/15 16:06, Ananyev, Konstantin wrote:
> > From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> >> On 04/14/15 12:31, Thomas Monjalon wrote:
> >>> -	struct rte_eth_dev_info dev_info = { 0 };
> >>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
> >> 
> >> Hmmm... Unless I miss something this and one above would zero only a
> >> single field - "max_rx_queues"; and would leave the rest uninitialized.
> >> The original code intend to zero the whole struct. The alternative to
> >> the original lines could be usage of memset().
> > 
> > As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
> > So I think we are ok here.
> 
> Yeah, I guess it does zero-initializes the rest 
> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I 
> don't understand how the above change fixes the error if it complains 
> about the dev_info.driver_name?

As only 1 field is required, I chose the one which should not be removed
from this structure in the future.

> What I'm trying to say - the proposed fix is completely unclear and 
> confusing. Think of somebody reading this line in a month from today - 
> he wouldn't get a clue why is it there, why to explicitly set 
> max_rx_queues to zero and leave the rest be zeroed automatically... Why 
> to add such artifacts to the code instead of just zeroing the struct 
> with a memset() and putting a good clear comment above it explaining why 
> we use a memset() and not and initializer?

We can make it longer yes.
I think you agree we should avoid extra lines if not needed.
In this case, when reading "= { .field = 0 }", it seems clear our goal
is to zero the structure (it is to me).
I thought it is a basic C practice.

You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
not going to comment each occurence of this coding style.
But it must be explained in the coding style document. Agree?
  

On 04/14/15 17:17, Thomas Monjalon wrote:
> 2015-04-14 16:38, Vlad Zolotarov:
>> On 04/14/15 16:06, Ananyev, Konstantin wrote:
>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>>>> -	struct rte_eth_dev_info dev_info = { 0 };
>>>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>>> Hmmm... Unless I miss something this and one above would zero only a
>>>> single field - "max_rx_queues"; and would leave the rest uninitialized.
>>>> The original code intend to zero the whole struct. The alternative to
>>>> the original lines could be usage of memset().
>>> As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
>>> So I think we are ok here.
>> Yeah, I guess it does zero-initializes the rest
>> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
>> don't understand how the above change fixes the error if it complains
>> about the dev_info.driver_name?
> As only 1 field is required, I chose the one which should not be removed
> from this structure in the future.

I don't follow - where/why only one field is required? The function u 
are patching uses "rx_offload_capa" field. Or u mean this gcc version 
requires only one field? If so, could u, please, provide the errata u 
are referring, since standard doesn't require any field and {0} is an 
absolutely legal (and proper) initializer in this case...

>
>> What I'm trying to say - the proposed fix is completely unclear and
>> confusing. Think of somebody reading this line in a month from today -
>> he wouldn't get a clue why is it there, why to explicitly set
>> max_rx_queues to zero and leave the rest be zeroed automatically... Why
>> to add such artifacts to the code instead of just zeroing the struct
>> with a memset() and putting a good clear comment above it explaining why
>> we use a memset() and not and initializer?
> We can make it longer yes.
> I think you agree we should avoid extra lines if not needed.
> In this case, when reading "= { .field = 0 }", it seems clear our goal
> is to zero the structure (it is to me).
> I thought it is a basic C practice.
>
> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> not going to comment each occurence of this coding style.
> But it must be explained in the coding style document. Agree?
  

2015-04-14 17:30, Vlad Zolotarov:
> On 04/14/15 17:17, Thomas Monjalon wrote:
> > 2015-04-14 16:38, Vlad Zolotarov:
> >> On 04/14/15 16:06, Ananyev, Konstantin wrote:
> >>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> >>>> On 04/14/15 12:31, Thomas Monjalon wrote:
> >>>>> -	struct rte_eth_dev_info dev_info = { 0 };
> >>>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
> >>>> Hmmm... Unless I miss something this and one above would zero only a
> >>>> single field - "max_rx_queues"; and would leave the rest uninitialized.
> >>>> The original code intend to zero the whole struct. The alternative to
> >>>> the original lines could be usage of memset().
> >>> As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
> >>> So I think we are ok here.
> >> Yeah, I guess it does zero-initializes the rest
> >> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
> >> don't understand how the above change fixes the error if it complains
> >> about the dev_info.driver_name?
> > As only 1 field is required, I chose the one which should not be removed
> > from this structure in the future.
> 
> I don't follow - where/why only one field is required? The function u 
> are patching uses "rx_offload_capa" field. Or u mean this gcc version 
> requires only one field? If so, could u, please, provide the errata u 
> are referring, since standard doesn't require any field and {0} is an 
> absolutely legal (and proper) initializer in this case...

Honestly I don't really care what is "legal". The most important is to make
it working with most C compilers with minimal overhead.
You're right about the variable choice: rx_offload_capa is more appropriate.
Are you OK for a v2 replacing max_rx_queues by rx_offload_capa?

> >> What I'm trying to say - the proposed fix is completely unclear and
> >> confusing. Think of somebody reading this line in a month from today -
> >> he wouldn't get a clue why is it there, why to explicitly set
> >> max_rx_queues to zero and leave the rest be zeroed automatically... Why
> >> to add such artifacts to the code instead of just zeroing the struct
> >> with a memset() and putting a good clear comment above it explaining why
> >> we use a memset() and not and initializer?
> > We can make it longer yes.
> > I think you agree we should avoid extra lines if not needed.
> > In this case, when reading "= { .field = 0 }", it seems clear our goal
> > is to zero the structure (it is to me).
> > I thought it is a basic C practice.
> >
> > You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> > not going to comment each occurence of this coding style.
> > But it must be explained in the coding style document. Agree?
>
  

On 04/14/15 17:17, Thomas Monjalon wrote:
> 2015-04-14 16:38, Vlad Zolotarov:
>> On 04/14/15 16:06, Ananyev, Konstantin wrote:
>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>>>> -	struct rte_eth_dev_info dev_info = { 0 };
>>>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>>> Hmmm... Unless I miss something this and one above would zero only a
>>>> single field - "max_rx_queues"; and would leave the rest uninitialized.
>>>> The original code intend to zero the whole struct. The alternative to
>>>> the original lines could be usage of memset().
>>> As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
>>> So I think we are ok here.
>> Yeah, I guess it does zero-initializes the rest
>> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
>> don't understand how the above change fixes the error if it complains
>> about the dev_info.driver_name?
> As only 1 field is required, I chose the one which should not be removed
> from this structure in the future.
>
>> What I'm trying to say - the proposed fix is completely unclear and
>> confusing. Think of somebody reading this line in a month from today -
>> he wouldn't get a clue why is it there, why to explicitly set
>> max_rx_queues to zero and leave the rest be zeroed automatically... Why
>> to add such artifacts to the code instead of just zeroing the struct
>> with a memset() and putting a good clear comment above it explaining why
>> we use a memset() and not and initializer?
> We can make it longer yes.
> I think you agree we should avoid extra lines if not needed.
> In this case, when reading "= { .field = 0 }", it seems clear our goal
> is to zero the structure (it is to me).

I'm sorry but it's not clear to me at all since the common C practice 
for zeroing the struct would be

struct st a = {0};

Like in the lines u are changing. The lines as above are clearly should 
not be commented and are absolutely clear.
The lines u are adding on the other hand are absolutely unclear and 
confusing outside the gcc bug context. Therefore it should be clearly 
stated so in a form of comment. Otherwise somebody (like myself) may see 
this and immediately fix it back (as it should be).

> I thought it is a basic C practice.

I doubt that. ;) Explained above.

>
> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> not going to comment each occurence of this coding style.
> But it must be explained in the coding style document. Agree?

OMG! This is awful! I think everybody agrees that this is a workaround 
and has nothing to do with a codding style (it's an opposite to a style 
actually). I don't know where this should be explained, frankly.

Getting back to the issue - I'm a bit surprised since I use this kind of 
initializer ({0}) in a C code for quite a long time - long before 2012. 
I'd like to understand what is a problem with this specific gcc version. 
This seems to trivial. I'm surprised CentOS has a gcc version with this 
kind of bugs.
  

2015-04-14 17:59, Vlad Zolotarov:
> On 04/14/15 17:17, Thomas Monjalon wrote:
> > 2015-04-14 16:38, Vlad Zolotarov:
> >> On 04/14/15 16:06, Ananyev, Konstantin wrote:
> >>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> >>>> On 04/14/15 12:31, Thomas Monjalon wrote:
> >>>>> -	struct rte_eth_dev_info dev_info = { 0 };
> >>>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
> >>>> Hmmm... Unless I miss something this and one above would zero only a
> >>>> single field - "max_rx_queues"; and would leave the rest uninitialized.
> >>>> The original code intend to zero the whole struct. The alternative to
> >>>> the original lines could be usage of memset().
> >>> As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
> >>> So I think we are ok here.
> >> Yeah, I guess it does zero-initializes the rest
> >> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
> >> don't understand how the above change fixes the error if it complains
> >> about the dev_info.driver_name?
> > As only 1 field is required, I chose the one which should not be removed
> > from this structure in the future.
> >
> >> What I'm trying to say - the proposed fix is completely unclear and
> >> confusing. Think of somebody reading this line in a month from today -
> >> he wouldn't get a clue why is it there, why to explicitly set
> >> max_rx_queues to zero and leave the rest be zeroed automatically... Why
> >> to add such artifacts to the code instead of just zeroing the struct
> >> with a memset() and putting a good clear comment above it explaining why
> >> we use a memset() and not and initializer?
> > We can make it longer yes.
> > I think you agree we should avoid extra lines if not needed.
> > In this case, when reading "= { .field = 0 }", it seems clear our goal
> > is to zero the structure (it is to me).
> 
> I'm sorry but it's not clear to me at all since the common C practice 
> for zeroing the struct would be
> 
> struct st a = {0};
> 
> Like in the lines u are changing. The lines as above are clearly should 
> not be commented and are absolutely clear.
> The lines u are adding on the other hand are absolutely unclear and 
> confusing outside the gcc bug context. Therefore it should be clearly 
> stated so in a form of comment. Otherwise somebody (like myself) may see 
> this and immediately fix it back (as it should be).
> 
> > I thought it is a basic C practice.
> 
> I doubt that. ;) Explained above.
> 
> > You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> > not going to comment each occurence of this coding style.
> > But it must be explained in the coding style document. Agree?
> 
> OMG! This is awful! I think everybody agrees that this is a workaround 
> and has nothing to do with a codding style (it's an opposite to a style 
> actually). I don't know where this should be explained, frankly.

Once we assert we want to support this buggy compiler, the workarounds
are automatically parts of the coding style.
I don't know how to deal differently with this constraint.

> Getting back to the issue - I'm a bit surprised since I use this kind of 
> initializer ({0}) in a C code for quite a long time - long before 2012. 
> I'd like to understand what is a problem with this specific gcc version. 
> This seems to trivial. I'm surprised CentOS has a gcc version with this 
> kind of bugs.

Each day brings its surprise :)
  

On 04/14/15 17:53, Thomas Monjalon wrote:
> 2015-04-14 17:30, Vlad Zolotarov:
>> On 04/14/15 17:17, Thomas Monjalon wrote:
>>> 2015-04-14 16:38, Vlad Zolotarov:
>>>> On 04/14/15 16:06, Ananyev, Konstantin wrote:
>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>>>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>>>>>> -	struct rte_eth_dev_info dev_info = { 0 };
>>>>>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>>>>> Hmmm... Unless I miss something this and one above would zero only a
>>>>>> single field - "max_rx_queues"; and would leave the rest uninitialized.
>>>>>> The original code intend to zero the whole struct. The alternative to
>>>>>> the original lines could be usage of memset().
>>>>> As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
>>>>> So I think we are ok here.
>>>> Yeah, I guess it does zero-initializes the rest
>>>> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
>>>> don't understand how the above change fixes the error if it complains
>>>> about the dev_info.driver_name?
>>> As only 1 field is required, I chose the one which should not be removed
>>> from this structure in the future.
>> I don't follow - where/why only one field is required? The function u
>> are patching uses "rx_offload_capa" field. Or u mean this gcc version
>> requires only one field? If so, could u, please, provide the errata u
>> are referring, since standard doesn't require any field and {0} is an
>> absolutely legal (and proper) initializer in this case...
> Honestly I don't really care what is "legal". The most important is to make
> it working with most C compilers with minimal overhead.

It's not just a "legal" - it's the most correct and robust way of 
initializing the struct that is promised to always work correctly. See 
here 
http://stackoverflow.com/questions/11152160/initializing-a-struct-to-0. 
What u hit here is (as appears) a well known Bug #53119 in gcc (see here 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119).

Have u considered adding the compilation options like 
-Wno-missing-braces that would silence this warning for say gcc versions 
below 4.7?

> You're right about the variable choice: rx_offload_capa is more appropriate.
> Are you OK for a v2 replacing max_rx_queues by rx_offload_capa?
>
>>>> What I'm trying to say - the proposed fix is completely unclear and
>>>> confusing. Think of somebody reading this line in a month from today -
>>>> he wouldn't get a clue why is it there, why to explicitly set
>>>> max_rx_queues to zero and leave the rest be zeroed automatically... Why
>>>> to add such artifacts to the code instead of just zeroing the struct
>>>> with a memset() and putting a good clear comment above it explaining why
>>>> we use a memset() and not and initializer?
>>> We can make it longer yes.
>>> I think you agree we should avoid extra lines if not needed.
>>> In this case, when reading "= { .field = 0 }", it seems clear our goal
>>> is to zero the structure (it is to me).
>>> I thought it is a basic C practice.
>>>
>>> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
>>> not going to comment each occurence of this coding style.
>>> But it must be explained in the coding style document. Agree?
>
  

On 04/14/15 18:13, Thomas Monjalon wrote:
> 2015-04-14 17:59, Vlad Zolotarov:
>> On 04/14/15 17:17, Thomas Monjalon wrote:
>>> 2015-04-14 16:38, Vlad Zolotarov:
>>>> On 04/14/15 16:06, Ananyev, Konstantin wrote:
>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>>>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>>>>>> -	struct rte_eth_dev_info dev_info = { 0 };
>>>>>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>>>>> Hmmm... Unless I miss something this and one above would zero only a
>>>>>> single field - "max_rx_queues"; and would leave the rest uninitialized.
>>>>>> The original code intend to zero the whole struct. The alternative to
>>>>>> the original lines could be usage of memset().
>>>>> As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
>>>>> So I think we are ok here.
>>>> Yeah, I guess it does zero-initializes the rest
>>>> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
>>>> don't understand how the above change fixes the error if it complains
>>>> about the dev_info.driver_name?
>>> As only 1 field is required, I chose the one which should not be removed
>>> from this structure in the future.
>>>
>>>> What I'm trying to say - the proposed fix is completely unclear and
>>>> confusing. Think of somebody reading this line in a month from today -
>>>> he wouldn't get a clue why is it there, why to explicitly set
>>>> max_rx_queues to zero and leave the rest be zeroed automatically... Why
>>>> to add such artifacts to the code instead of just zeroing the struct
>>>> with a memset() and putting a good clear comment above it explaining why
>>>> we use a memset() and not and initializer?
>>> We can make it longer yes.
>>> I think you agree we should avoid extra lines if not needed.
>>> In this case, when reading "= { .field = 0 }", it seems clear our goal
>>> is to zero the structure (it is to me).
>> I'm sorry but it's not clear to me at all since the common C practice
>> for zeroing the struct would be
>>
>> struct st a = {0};
>>
>> Like in the lines u are changing. The lines as above are clearly should
>> not be commented and are absolutely clear.
>> The lines u are adding on the other hand are absolutely unclear and
>> confusing outside the gcc bug context. Therefore it should be clearly
>> stated so in a form of comment. Otherwise somebody (like myself) may see
>> this and immediately fix it back (as it should be).
>>
>>> I thought it is a basic C practice.
>> I doubt that. ;) Explained above.
>>
>>> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
>>> not going to comment each occurence of this coding style.
>>> But it must be explained in the coding style document. Agree?
>> OMG! This is awful! I think everybody agrees that this is a workaround
>> and has nothing to do with a codding style (it's an opposite to a style
>> actually). I don't know where this should be explained, frankly.
> Once we assert we want to support this buggy compiler, the workarounds
> are automatically parts of the coding style.

It'd rather not... ;)

> I don't know how to deal differently with this constraint.

Add -Wno-missing-braces compilation option for compiler versions below 
4.7. U (and me and I guess most other developers) compile DPDK code with 
a newer compiler thus the code would be properly inspected with these 
compilers and we may afford to be less restrictive with compilation 
warnings with legacy compiler versions...

>
>> Getting back to the issue - I'm a bit surprised since I use this kind of
>> initializer ({0}) in a C code for quite a long time - long before 2012.
>> I'd like to understand what is a problem with this specific gcc version.
>> This seems to trivial. I'm surprised CentOS has a gcc version with this
>> kind of bugs.
> Each day brings its surprise :)
>
  

2015-04-14 18:21, Vlad Zolotarov:
> 
> On 04/14/15 18:13, Thomas Monjalon wrote:
> > 2015-04-14 17:59, Vlad Zolotarov:
> >> On 04/14/15 17:17, Thomas Monjalon wrote:
> >>> 2015-04-14 16:38, Vlad Zolotarov:
> >>>> On 04/14/15 16:06, Ananyev, Konstantin wrote:
> >>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> >>>>>> On 04/14/15 12:31, Thomas Monjalon wrote:
> >>>>>>> -	struct rte_eth_dev_info dev_info = { 0 };
> >>>>>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
> >>>>>> Hmmm... Unless I miss something this and one above would zero only a
> >>>>>> single field - "max_rx_queues"; and would leave the rest uninitialized.
> >>>>>> The original code intend to zero the whole struct. The alternative to
> >>>>>> the original lines could be usage of memset().
> >>>>> As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
> >>>>> So I think we are ok here.
> >>>> Yeah, I guess it does zero-initializes the rest
> >>>> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
> >>>> don't understand how the above change fixes the error if it complains
> >>>> about the dev_info.driver_name?
> >>> As only 1 field is required, I chose the one which should not be removed
> >>> from this structure in the future.
> >>>
> >>>> What I'm trying to say - the proposed fix is completely unclear and
> >>>> confusing. Think of somebody reading this line in a month from today -
> >>>> he wouldn't get a clue why is it there, why to explicitly set
> >>>> max_rx_queues to zero and leave the rest be zeroed automatically... Why
> >>>> to add such artifacts to the code instead of just zeroing the struct
> >>>> with a memset() and putting a good clear comment above it explaining why
> >>>> we use a memset() and not and initializer?
> >>> We can make it longer yes.
> >>> I think you agree we should avoid extra lines if not needed.
> >>> In this case, when reading "= { .field = 0 }", it seems clear our goal
> >>> is to zero the structure (it is to me).
> >> I'm sorry but it's not clear to me at all since the common C practice
> >> for zeroing the struct would be
> >>
> >> struct st a = {0};
> >>
> >> Like in the lines u are changing. The lines as above are clearly should
> >> not be commented and are absolutely clear.
> >> The lines u are adding on the other hand are absolutely unclear and
> >> confusing outside the gcc bug context. Therefore it should be clearly
> >> stated so in a form of comment. Otherwise somebody (like myself) may see
> >> this and immediately fix it back (as it should be).
> >>
> >>> I thought it is a basic C practice.
> >> I doubt that. ;) Explained above.
> >>
> >>> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> >>> not going to comment each occurence of this coding style.
> >>> But it must be explained in the coding style document. Agree?
> >> OMG! This is awful! I think everybody agrees that this is a workaround
> >> and has nothing to do with a codding style (it's an opposite to a style
> >> actually). I don't know where this should be explained, frankly.
> > Once we assert we want to support this buggy compiler, the workarounds
> > are automatically parts of the coding style.
> 
> It'd rather not... ;)
> 
> > I don't know how to deal differently with this constraint.
> 
> Add -Wno-missing-braces compilation option for compiler versions below 
> 4.7. U (and me and I guess most other developers) compile DPDK code with 
> a newer compiler thus the code would be properly inspected with these 
> compilers and we may afford to be less restrictive with compilation 
> warnings with legacy compiler versions...

You're right.
I will test it and submit a v2.
Then I could use the above grep command to replace other occurences of this
workaround.

> >> Getting back to the issue - I'm a bit surprised since I use this kind of
> >> initializer ({0}) in a C code for quite a long time - long before 2012.
> >> I'd like to understand what is a problem with this specific gcc version.
> >> This seems to trivial. I'm surprised CentOS has a gcc version with this
> >> kind of bugs.
> > Each day brings its surprise :)
  

On 04/14/15 18:28, Thomas Monjalon wrote:
> 2015-04-14 18:21, Vlad Zolotarov:
>> On 04/14/15 18:13, Thomas Monjalon wrote:
>>> 2015-04-14 17:59, Vlad Zolotarov:
>>>> On 04/14/15 17:17, Thomas Monjalon wrote:
>>>>> 2015-04-14 16:38, Vlad Zolotarov:
>>>>>> On 04/14/15 16:06, Ananyev, Konstantin wrote:
>>>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>>>>>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>>>>>>>> -	struct rte_eth_dev_info dev_info = { 0 };
>>>>>>>>> +	struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>>>>>>> Hmmm... Unless I miss something this and one above would zero only a
>>>>>>>> single field - "max_rx_queues"; and would leave the rest uninitialized.
>>>>>>>> The original code intend to zero the whole struct. The alternative to
>>>>>>>> the original lines could be usage of memset().
>>>>>>> As I understand, in that case compiler had to set all non-explicitly initialised members to 0.
>>>>>>> So I think we are ok here.
>>>>>> Yeah, I guess it does zero-initializes the rest
>>>>>> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
>>>>>> don't understand how the above change fixes the error if it complains
>>>>>> about the dev_info.driver_name?
>>>>> As only 1 field is required, I chose the one which should not be removed
>>>>> from this structure in the future.
>>>>>
>>>>>> What I'm trying to say - the proposed fix is completely unclear and
>>>>>> confusing. Think of somebody reading this line in a month from today -
>>>>>> he wouldn't get a clue why is it there, why to explicitly set
>>>>>> max_rx_queues to zero and leave the rest be zeroed automatically... Why
>>>>>> to add such artifacts to the code instead of just zeroing the struct
>>>>>> with a memset() and putting a good clear comment above it explaining why
>>>>>> we use a memset() and not and initializer?
>>>>> We can make it longer yes.
>>>>> I think you agree we should avoid extra lines if not needed.
>>>>> In this case, when reading "= { .field = 0 }", it seems clear our goal
>>>>> is to zero the structure (it is to me).
>>>> I'm sorry but it's not clear to me at all since the common C practice
>>>> for zeroing the struct would be
>>>>
>>>> struct st a = {0};
>>>>
>>>> Like in the lines u are changing. The lines as above are clearly should
>>>> not be commented and are absolutely clear.
>>>> The lines u are adding on the other hand are absolutely unclear and
>>>> confusing outside the gcc bug context. Therefore it should be clearly
>>>> stated so in a form of comment. Otherwise somebody (like myself) may see
>>>> this and immediately fix it back (as it should be).
>>>>
>>>>> I thought it is a basic C practice.
>>>> I doubt that. ;) Explained above.
>>>>
>>>>> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
>>>>> not going to comment each occurence of this coding style.
>>>>> But it must be explained in the coding style document. Agree?
>>>> OMG! This is awful! I think everybody agrees that this is a workaround
>>>> and has nothing to do with a codding style (it's an opposite to a style
>>>> actually). I don't know where this should be explained, frankly.
>>> Once we assert we want to support this buggy compiler, the workarounds
>>> are automatically parts of the coding style.
>> It'd rather not... ;)
>>
>>> I don't know how to deal differently with this constraint.
>> Add -Wno-missing-braces compilation option for compiler versions below
>> 4.7. U (and me and I guess most other developers) compile DPDK code with
>> a newer compiler thus the code would be properly inspected with these
>> compilers and we may afford to be less restrictive with compilation
>> warnings with legacy compiler versions...
> You're right.
> I will test it and submit a v2.
> Then I could use the above grep command to replace other occurences of this
> workaround.

U read my mind!.. ;)

>
>>>> Getting back to the issue - I'm a bit surprised since I use this kind of
>>>> initializer ({0}) in a C code for quite a long time - long before 2012.
>>>> I'd like to understand what is a problem with this specific gcc version.
>>>> This seems to trivial. I'm surprised CentOS has a gcc version with this
>>>> kind of bugs.
>>> Each day brings its surprise :)
>