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

[Thomas Monjalon <thomas@monjalon.net>]

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