[dpdk-dev] compilation error on Suse 11 - LPM init of anon union

[dpdk-dev] compilation error on Suse 11 - LPM init of anon union

[Thomas Monjalon]

Hi,

There is a new compilation error since this commit in LPM:
	http://dpdk.org/commit/b2e1c99
The brace has been removed because unnecessary with anonymous union.

This union is declared with RTE_STD_C11 for compatibility
with old compilers:
	/** C extension macro for environments lacking C11 features. */
	#if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L
	#define RTE_STD_C11 __extension__                                                                    
	#else
	#define RTE_STD_C11
	#endif

Unfortunately, it does not work on Suse 11 SP2 with GCC 4.5.1:
	lib/librte_lpm/rte_lpm.c: In function ‘add_depth_big_v20’:
	lib/librte_lpm/rte_lpm.c:886:4: error:
	unknown field ‘group_idx’ specified in initializer

Curiously, the error is exactly the same with ICC 16.0.2:
	http://dpdk.org/ml/archives/test-report/2018-January/038443.html
Is it really using different compilers in those 2 tests?

Someone to check the value of __STDC_VERSION__ with those compilers?
	gcc -dM -E -xc /dev/null | grep STDC_VERSION

Thanks for the help

Re: [dpdk-dev] compilation error on Suse 11 - LPM init of anon union

[Adrien Mazarguil]

On Sat, Jan 13, 2018 at 08:14:06PM +0100, Thomas Monjalon wrote:
> Hi,
> 
> There is a new compilation error since this commit in LPM:
> 	http://dpdk.org/commit/b2e1c99
> The brace has been removed because unnecessary with anonymous union.
> 
> This union is declared with RTE_STD_C11 for compatibility
> with old compilers:
> 	/** C extension macro for environments lacking C11 features. */
> 	#if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L
> 	#define RTE_STD_C11 __extension__                                                                    
> 	#else
> 	#define RTE_STD_C11
> 	#endif

Yes, however not only for old compilers, e.g. explicitly specifying -std=c99
on the command-line disables C11 extensions for newer compilers as well.

Not specifying anything (like most applications do) simply defaults to
whatever standard is deemed "current" for it.

In short, RTE_STD_C11 gets expanded as __extension__ when the compiler isn't
in C11 mode, and what follows is therefore an extension to the standard in
use (be it C90 or C99).

__extension__ remains explicitly used in place of RTE_STD_C11 for things
that are not even found in C11, namely GNU syntax extensions fall under this
category. Keep in mind the __extension__ keyword is itself a GNU extension.

> Unfortunately, it does not work on Suse 11 SP2 with GCC 4.5.1:
> 	lib/librte_lpm/rte_lpm.c: In function ‘add_depth_big_v20’:
> 	lib/librte_lpm/rte_lpm.c:886:4: error:
> 	unknown field ‘group_idx’ specified in initializer
> 
> Curiously, the error is exactly the same with ICC 16.0.2:
> 	http://dpdk.org/ml/archives/test-report/2018-January/038443.html
> Is it really using different compilers in those 2 tests?
> 
> Someone to check the value of __STDC_VERSION__ with those compilers?
> 	gcc -dM -E -xc /dev/null | grep STDC_VERSION
> 
> Thanks for the help

Since this problem only appears in big endian, my suggestion would be to add
RTE_STD_C11 to the anonymous union of struct rte_lpm_tbl_entry_v20
(rte_lpm.h), like its little endian counterpart:

 #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
 [...]
         RTE_STD_C11
         union {
                 uint8_t next_hop;
                 uint8_t group_idx;
         };
 [...]
 #else
 __extension__
 struct rte_lpm_tbl_entry_v20 {
         uint8_t depth       :6;
         uint8_t valid_group :1;
         uint8_t valid       :1;
         RTE_STD_C11 // <<< Should be added here
         union {
                 uint8_t group_idx;
                 uint8_t next_hop;
         };
 };

I don't have the adequate test environment to validate this, so please
report if it helps and/or submit a patch, thanks.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] compilation error on Suse 11 - LPM init of anon union

[Adrien Mazarguil]

On Mon, Jan 15, 2018 at 05:18:37PM +0100, Adrien Mazarguil wrote:
> On Sat, Jan 13, 2018 at 08:14:06PM +0100, Thomas Monjalon wrote:
> > Hi,
> > 
> > There is a new compilation error since this commit in LPM:
> > 	http://dpdk.org/commit/b2e1c99
> > The brace has been removed because unnecessary with anonymous union.
> > 
> > This union is declared with RTE_STD_C11 for compatibility
> > with old compilers:
> > 	/** C extension macro for environments lacking C11 features. */
> > 	#if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L
> > 	#define RTE_STD_C11 __extension__                                                                    
> > 	#else
> > 	#define RTE_STD_C11
> > 	#endif
> 
> Yes, however not only for old compilers, e.g. explicitly specifying -std=c99
> on the command-line disables C11 extensions for newer compilers as well.
> 
> Not specifying anything (like most applications do) simply defaults to
> whatever standard is deemed "current" for it.
> 
> In short, RTE_STD_C11 gets expanded as __extension__ when the compiler isn't
> in C11 mode, and what follows is therefore an extension to the standard in
> use (be it C90 or C99).
> 
> __extension__ remains explicitly used in place of RTE_STD_C11 for things
> that are not even found in C11, namely GNU syntax extensions fall under this
> category. Keep in mind the __extension__ keyword is itself a GNU extension.
> 
> > Unfortunately, it does not work on Suse 11 SP2 with GCC 4.5.1:
> > 	lib/librte_lpm/rte_lpm.c: In function ‘add_depth_big_v20’:
> > 	lib/librte_lpm/rte_lpm.c:886:4: error:
> > 	unknown field ‘group_idx’ specified in initializer
> > 
> > Curiously, the error is exactly the same with ICC 16.0.2:
> > 	http://dpdk.org/ml/archives/test-report/2018-January/038443.html
> > Is it really using different compilers in those 2 tests?
> > 
> > Someone to check the value of __STDC_VERSION__ with those compilers?
> > 	gcc -dM -E -xc /dev/null | grep STDC_VERSION
> > 
> > Thanks for the help
> 
> Since this problem only appears in big endian, my suggestion would be to add
> RTE_STD_C11 to the anonymous union of struct rte_lpm_tbl_entry_v20
> (rte_lpm.h), like its little endian counterpart:
> 
>  #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>  [...]
>          RTE_STD_C11
>          union {
>                  uint8_t next_hop;
>                  uint8_t group_idx;
>          };
>  [...]
>  #else
>  __extension__
>  struct rte_lpm_tbl_entry_v20 {
>          uint8_t depth       :6;
>          uint8_t valid_group :1;
>          uint8_t valid       :1;
>          RTE_STD_C11 // <<< Should be added here
>          union {
>                  uint8_t group_idx;
>                  uint8_t next_hop;
>          };
>  };
> 
> I don't have the adequate test environment to validate this, so please
> report if it helps and/or submit a patch, thanks.

Looks like I mixed the issue mentioned by the original patch [1] and the one
you found on SuSE, which appears on little endian systems.

Adding RTE_STD_C11 as suggested above is correct but useless since
__extension__ is part of the parent structure definition anyway, so this is
not the reason.

Adding -pedantic (but not -std), this issue can be reproduced in a form or
another using GCC 4.4 through 4.9 which all default to C90, while GCC 6.3
defaults to C11. Without -pendantic, I only managed to reproduce it with GCC
4.4 (I don't have 4.5 handy, however it can't be reproduced using 4.6).

The problem with GCC 4.4 and likely 4.5 is basically they do not support the
initialization syntax used in rte_lpm.c. Extra { } are needed even with
unnamed union fields, there's no way around that AFAIK.

Since we likely don't want to revert [1] and although GCC 4.5 is not
recommended (4.9 minimum according to [2]), I suggest using a more
conventional initialization for this particular field, e.g. replacing:
 
 struct rte_lpm_tbl_entry_v20 new_tbl24_entry = {
     .group_idx = (uint8_t)tbl8_group_index,
     .valid = VALID,
     .valid_group = 1,
     .depth = 0,
 };

With something like:

 struct rte_lpm_tbl_entry_v20 new_tbl24_entry = {
     .valid = VALID,
     .valid_group = 1,
     .depth = 0,
 };

 /* Anonymous union field initialized outside (GCC < 4.6 compatibility). */
 new_tbl24_entry.group_idx = (uint8_t)tbl8_group_index;

Your call.

[1] http://dpdk.org/commit/b2e1c99
[2] http://dpdk.org/doc/guides/linux_gsg/sys_reqs.html

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] compilation error on Suse 11 - LPM init of anon union

[Thomas Monjalon]

We need someone from Intel to check on the testing platform please.
It can be decided to drop testing of Suse 11 SP2.
Thanks

15/01/2018 18:08, Adrien Mazarguil:
> On Mon, Jan 15, 2018 at 05:18:37PM +0100, Adrien Mazarguil wrote:
> > On Sat, Jan 13, 2018 at 08:14:06PM +0100, Thomas Monjalon wrote:
> > > Hi,
> > > 
> > > There is a new compilation error since this commit in LPM:
> > > 	http://dpdk.org/commit/b2e1c99
> > > The brace has been removed because unnecessary with anonymous union.
> > > 
> > > This union is declared with RTE_STD_C11 for compatibility
> > > with old compilers:
> > > 	/** C extension macro for environments lacking C11 features. */
> > > 	#if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L
> > > 	#define RTE_STD_C11 __extension__                                                                    
> > > 	#else
> > > 	#define RTE_STD_C11
> > > 	#endif
> > 
> > Yes, however not only for old compilers, e.g. explicitly specifying -std=c99
> > on the command-line disables C11 extensions for newer compilers as well.
> > 
> > Not specifying anything (like most applications do) simply defaults to
> > whatever standard is deemed "current" for it.
> > 
> > In short, RTE_STD_C11 gets expanded as __extension__ when the compiler isn't
> > in C11 mode, and what follows is therefore an extension to the standard in
> > use (be it C90 or C99).
> > 
> > __extension__ remains explicitly used in place of RTE_STD_C11 for things
> > that are not even found in C11, namely GNU syntax extensions fall under this
> > category. Keep in mind the __extension__ keyword is itself a GNU extension.
> > 
> > > Unfortunately, it does not work on Suse 11 SP2 with GCC 4.5.1:
> > > 	lib/librte_lpm/rte_lpm.c: In function ‘add_depth_big_v20’:
> > > 	lib/librte_lpm/rte_lpm.c:886:4: error:
> > > 	unknown field ‘group_idx’ specified in initializer
> > > 
> > > Curiously, the error is exactly the same with ICC 16.0.2:
> > > 	http://dpdk.org/ml/archives/test-report/2018-January/038443.html
> > > Is it really using different compilers in those 2 tests?
> > > 
> > > Someone to check the value of __STDC_VERSION__ with those compilers?
> > > 	gcc -dM -E -xc /dev/null | grep STDC_VERSION
> > > 
> > > Thanks for the help
> > 
> > Since this problem only appears in big endian, my suggestion would be to add
> > RTE_STD_C11 to the anonymous union of struct rte_lpm_tbl_entry_v20
> > (rte_lpm.h), like its little endian counterpart:
> > 
> >  #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> >  [...]
> >          RTE_STD_C11
> >          union {
> >                  uint8_t next_hop;
> >                  uint8_t group_idx;
> >          };
> >  [...]
> >  #else
> >  __extension__
> >  struct rte_lpm_tbl_entry_v20 {
> >          uint8_t depth       :6;
> >          uint8_t valid_group :1;
> >          uint8_t valid       :1;
> >          RTE_STD_C11 // <<< Should be added here
> >          union {
> >                  uint8_t group_idx;
> >                  uint8_t next_hop;
> >          };
> >  };
> > 
> > I don't have the adequate test environment to validate this, so please
> > report if it helps and/or submit a patch, thanks.
> 
> Looks like I mixed the issue mentioned by the original patch [1] and the one
> you found on SuSE, which appears on little endian systems.
> 
> Adding RTE_STD_C11 as suggested above is correct but useless since
> __extension__ is part of the parent structure definition anyway, so this is
> not the reason.
> 
> Adding -pedantic (but not -std), this issue can be reproduced in a form or
> another using GCC 4.4 through 4.9 which all default to C90, while GCC 6.3
> defaults to C11. Without -pendantic, I only managed to reproduce it with GCC
> 4.4 (I don't have 4.5 handy, however it can't be reproduced using 4.6).
> 
> The problem with GCC 4.4 and likely 4.5 is basically they do not support the
> initialization syntax used in rte_lpm.c. Extra { } are needed even with
> unnamed union fields, there's no way around that AFAIK.
> 
> Since we likely don't want to revert [1] and although GCC 4.5 is not
> recommended (4.9 minimum according to [2]), I suggest using a more
> conventional initialization for this particular field, e.g. replacing:
>  
>  struct rte_lpm_tbl_entry_v20 new_tbl24_entry = {
>      .group_idx = (uint8_t)tbl8_group_index,
>      .valid = VALID,
>      .valid_group = 1,
>      .depth = 0,
>  };
> 
> With something like:
> 
>  struct rte_lpm_tbl_entry_v20 new_tbl24_entry = {
>      .valid = VALID,
>      .valid_group = 1,
>      .depth = 0,
>  };
> 
>  /* Anonymous union field initialized outside (GCC < 4.6 compatibility). */
>  new_tbl24_entry.group_idx = (uint8_t)tbl8_group_index;
> 
> Your call.
> 
> [1] http://dpdk.org/commit/b2e1c99
> [2] http://dpdk.org/doc/guides/linux_gsg/sys_reqs.html

Re: [dpdk-dev] compilation error on Suse 11 - LPM init of anon union

[Thomas Monjalon]

Sending again with fixed recipient email.

17/01/2018 23:49, Thomas Monjalon:
> We need someone from Intel to check on the testing platform please.
> It can be decided to drop testing of Suse 11 SP2.
> Thanks
> 
> 15/01/2018 18:08, Adrien Mazarguil:
> > On Mon, Jan 15, 2018 at 05:18:37PM +0100, Adrien Mazarguil wrote:
> > > On Sat, Jan 13, 2018 at 08:14:06PM +0100, Thomas Monjalon wrote:
> > > > Hi,
> > > > 
> > > > There is a new compilation error since this commit in LPM:
> > > > 	http://dpdk.org/commit/b2e1c99
> > > > The brace has been removed because unnecessary with anonymous union.
> > > > 
> > > > This union is declared with RTE_STD_C11 for compatibility
> > > > with old compilers:
> > > > 	/** C extension macro for environments lacking C11 features. */
> > > > 	#if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L
> > > > 	#define RTE_STD_C11 __extension__                                                                    
> > > > 	#else
> > > > 	#define RTE_STD_C11
> > > > 	#endif
> > > 
> > > Yes, however not only for old compilers, e.g. explicitly specifying -std=c99
> > > on the command-line disables C11 extensions for newer compilers as well.
> > > 
> > > Not specifying anything (like most applications do) simply defaults to
> > > whatever standard is deemed "current" for it.
> > > 
> > > In short, RTE_STD_C11 gets expanded as __extension__ when the compiler isn't
> > > in C11 mode, and what follows is therefore an extension to the standard in
> > > use (be it C90 or C99).
> > > 
> > > __extension__ remains explicitly used in place of RTE_STD_C11 for things
> > > that are not even found in C11, namely GNU syntax extensions fall under this
> > > category. Keep in mind the __extension__ keyword is itself a GNU extension.
> > > 
> > > > Unfortunately, it does not work on Suse 11 SP2 with GCC 4.5.1:
> > > > 	lib/librte_lpm/rte_lpm.c: In function ‘add_depth_big_v20’:
> > > > 	lib/librte_lpm/rte_lpm.c:886:4: error:
> > > > 	unknown field ‘group_idx’ specified in initializer
> > > > 
> > > > Curiously, the error is exactly the same with ICC 16.0.2:
> > > > 	http://dpdk.org/ml/archives/test-report/2018-January/038443.html
> > > > Is it really using different compilers in those 2 tests?
> > > > 
> > > > Someone to check the value of __STDC_VERSION__ with those compilers?
> > > > 	gcc -dM -E -xc /dev/null | grep STDC_VERSION
> > > > 
> > > > Thanks for the help
> > > 
> > > Since this problem only appears in big endian, my suggestion would be to add
> > > RTE_STD_C11 to the anonymous union of struct rte_lpm_tbl_entry_v20
> > > (rte_lpm.h), like its little endian counterpart:
> > > 
> > >  #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > >  [...]
> > >          RTE_STD_C11
> > >          union {
> > >                  uint8_t next_hop;
> > >                  uint8_t group_idx;
> > >          };
> > >  [...]
> > >  #else
> > >  __extension__
> > >  struct rte_lpm_tbl_entry_v20 {
> > >          uint8_t depth       :6;
> > >          uint8_t valid_group :1;
> > >          uint8_t valid       :1;
> > >          RTE_STD_C11 // <<< Should be added here
> > >          union {
> > >                  uint8_t group_idx;
> > >                  uint8_t next_hop;
> > >          };
> > >  };
> > > 
> > > I don't have the adequate test environment to validate this, so please
> > > report if it helps and/or submit a patch, thanks.
> > 
> > Looks like I mixed the issue mentioned by the original patch [1] and the one
> > you found on SuSE, which appears on little endian systems.
> > 
> > Adding RTE_STD_C11 as suggested above is correct but useless since
> > __extension__ is part of the parent structure definition anyway, so this is
> > not the reason.
> > 
> > Adding -pedantic (but not -std), this issue can be reproduced in a form or
> > another using GCC 4.4 through 4.9 which all default to C90, while GCC 6.3
> > defaults to C11. Without -pendantic, I only managed to reproduce it with GCC
> > 4.4 (I don't have 4.5 handy, however it can't be reproduced using 4.6).
> > 
> > The problem with GCC 4.4 and likely 4.5 is basically they do not support the
> > initialization syntax used in rte_lpm.c. Extra { } are needed even with
> > unnamed union fields, there's no way around that AFAIK.
> > 
> > Since we likely don't want to revert [1] and although GCC 4.5 is not
> > recommended (4.9 minimum according to [2]), I suggest using a more
> > conventional initialization for this particular field, e.g. replacing:
> >  
> >  struct rte_lpm_tbl_entry_v20 new_tbl24_entry = {
> >      .group_idx = (uint8_t)tbl8_group_index,
> >      .valid = VALID,
> >      .valid_group = 1,
> >      .depth = 0,
> >  };
> > 
> > With something like:
> > 
> >  struct rte_lpm_tbl_entry_v20 new_tbl24_entry = {
> >      .valid = VALID,
> >      .valid_group = 1,
> >      .depth = 0,
> >  };
> > 
> >  /* Anonymous union field initialized outside (GCC < 4.6 compatibility). */
> >  new_tbl24_entry.group_idx = (uint8_t)tbl8_group_index;
> > 
> > Your call.
> > 
> > [1] http://dpdk.org/commit/b2e1c99
> > [2] http://dpdk.org/doc/guides/linux_gsg/sys_reqs.html