[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