Re: [iproute PATCH v3 0/6] Big C99 style initializer rework

[Phil Sutter]

On Tue, Jun 28, 2016 at 11:59:04AM -0600, David Ahern wrote:
> On 6/28/16 11:58 AM, Phil Sutter wrote:
> >> since .ifr_qlen is already referenced in that function seems like your
> >> suggestion above (struct ifreq ifr = { .ifr_qlen = 0 };) should be
> >> acceptable.
> >
> > You mean regarding compatibility of using that define? Or are you
> > concerned with gcc creating suboptimal code?
> 
> no, I was thinking in terms of open coding knowledge of a struct.

Still not sure if I understand you correctly. These are not typedefs, so
users are supposed to know the internals and removing a field means
potentially breaking every single user.

> > I'd rather use a more generic approach than the above. Retrospectively,
> > I'd rather have that brace orgy instead of the above since it's
> > intention is more clear and it can be dropped once either gcc guys
> > manage to backport their fix or the last distribution has updated it's
> > compiler.
> 
> ha, that's funny.

At least someone can laugh about it. :)

Cheers, Phil

Re: [iproute PATCH v3 0/6] Big C99 style initializer rework

[David Ahern]

On 6/28/16 11:58 AM, Phil Sutter wrote:

since .ifr_qlen is already referenced in that function seems like your
suggestion above (struct ifreq ifr = { .ifr_qlen = 0 };) should be
acceptable.


You mean regarding compatibility of using that define? Or are you
concerned with gcc creating suboptimal code?


no, I was thinking in terms of open coding knowledge of a struct.


I'd rather use a more generic approach than the above. Retrospectively,
I'd rather have that brace orgy instead of the above since it's
intention is more clear and it can be dropped once either gcc guys
manage to backport their fix or the last distribution has updated it's
compiler.


ha, that's funny.

Re: [iproute PATCH v3 0/6] Big C99 style initializer rework

[Phil Sutter]

On Tue, Jun 28, 2016 at 11:37:43AM -0600, David Ahern wrote:
> On 6/28/16 11:37 AM, Phil Sutter wrote:
> >>> I saw these too with gcc-3.4.6 but not with 5.3.0. It appears to be a
> >>> gcc bug[1]. One possible workaround is to match the brace level of the
> >>> first field, but it's quite ugly: [2]. Another way might be to
> >>> initialize one of the fields to zero, like so:
> >>>
> >>> | struct ifreq ifr = { .ifr_qlen = 0 };
> >>>
> >>> What do you think?
> >>>
> >>> Thanks, Phil
> >>>
> >>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
> >>> [2] 
> >>> http://nwl.cc/cgi-bin/git/gitweb.cgi?p=iproute2.git;a=commitdiff;h=a1cbf2b63c995b2f633c5b4699248ab308b201d2;hp=3809cfec65b03716d1d0360338126df4b4f3fbf6
> >>
> >> I am using gcc on Debian stable which is 5.3.1.
> >
> > Hmm. In a fresh install of Debian 8.5 I see the warnings as well, but it
> > has gcc-4.9.2-10 as most recent version.
> >
> > Another thing I noticed: Using empty braces ('{}') instead of the
> > universal zero initializer seems to work without causing warnings (at
> > least unless '-pedantic' is used).
> 
> since .ifr_qlen is already referenced in that function seems like your 
> suggestion above (struct ifreq ifr = { .ifr_qlen = 0 };) should be 
> acceptable.

You mean regarding compatibility of using that define? Or are you
concerned with gcc creating suboptimal code?

I'd rather use a more generic approach than the above. Retrospectively,
I'd rather have that brace orgy instead of the above since it's
intention is more clear and it can be dropped once either gcc guys
manage to backport their fix or the last distribution has updated it's
compiler.

Cheers, Phil

Re: [iproute PATCH v3 0/6] Big C99 style initializer rework

[David Ahern]

On 6/28/16 11:37 AM, Phil Sutter wrote:

I saw these too with gcc-3.4.6 but not with 5.3.0. It appears to be a
gcc bug[1]. One possible workaround is to match the brace level of the
first field, but it's quite ugly: [2]. Another way might be to
initialize one of the fields to zero, like so:

| struct ifreq ifr = { .ifr_qlen = 0 };

What do you think?

Thanks, Phil

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
[2] 
http://nwl.cc/cgi-bin/git/gitweb.cgi?p=iproute2.git;a=commitdiff;h=a1cbf2b63c995b2f633c5b4699248ab308b201d2;hp=3809cfec65b03716d1d0360338126df4b4f3fbf6


I am using gcc on Debian stable which is 5.3.1.


Hmm. In a fresh install of Debian 8.5 I see the warnings as well, but it
has gcc-4.9.2-10 as most recent version.

Another thing I noticed: Using empty braces ('{}') instead of the
universal zero initializer seems to work without causing warnings (at
least unless '-pedantic' is used).


since .ifr_qlen is already referenced in that function seems like your 
suggestion above (struct ifreq ifr = { .ifr_qlen = 0 };) should be 
acceptable.

Re: [iproute PATCH v3 0/6] Big C99 style initializer rework

[Phil Sutter]

On Mon, Jun 27, 2016 at 02:10:49PM -0700, Stephen Hemminger wrote:
> On Mon, 27 Jun 2016 20:23:02 +0200
> Phil Sutter  wrote:
> 
> > Hi,
> > 
> > On Mon, Jun 27, 2016 at 10:59:12AM -0700, Stephen Hemminger wrote:
> > > On Thu, 23 Jun 2016 17:34:08 +
> > > Phil Sutter  wrote:
> > > 
> > > > This is v3 of my C99-style initializer related patch series. The changes
> > > > since v2 are:
> > [...]
> > > 
> > > I like the idea and it makes code cleaner. But doing this introduces lots 
> > > of warnings
> > > and that is not acceptable.
> > > ip
> > > CC   ip.o
> > > CC   ipaddress.o
> > > ipaddress.c: In function ‘print_queuelen’:
> > > ipaddress.c:175:10: warning: missing braces around initializer 
> > > [-Wmissing-braces]
> > >struct ifreq ifr = { 0 };
> > >   ^
> > 
> > I saw these too with gcc-3.4.6 but not with 5.3.0. It appears to be a
> > gcc bug[1]. One possible workaround is to match the brace level of the
> > first field, but it's quite ugly: [2]. Another way might be to
> > initialize one of the fields to zero, like so:
> > 
> > | struct ifreq ifr = { .ifr_qlen = 0 };
> > 
> > What do you think?
> > 
> > Thanks, Phil
> > 
> > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
> > [2] 
> > http://nwl.cc/cgi-bin/git/gitweb.cgi?p=iproute2.git;a=commitdiff;h=a1cbf2b63c995b2f633c5b4699248ab308b201d2;hp=3809cfec65b03716d1d0360338126df4b4f3fbf6
> 
> I am using gcc on Debian stable which is 5.3.1.

Hmm. In a fresh install of Debian 8.5 I see the warnings as well, but it
has gcc-4.9.2-10 as most recent version.

Another thing I noticed: Using empty braces ('{}') instead of the
universal zero initializer seems to work without causing warnings (at
least unless '-pedantic' is used).

Cheers, Phil

Re: [iproute PATCH v3 0/6] Big C99 style initializer rework

[Stephen Hemminger]

On Mon, 27 Jun 2016 20:23:02 +0200
Phil Sutter  wrote:

> Hi,
> 
> On Mon, Jun 27, 2016 at 10:59:12AM -0700, Stephen Hemminger wrote:
> > On Thu, 23 Jun 2016 17:34:08 +
> > Phil Sutter  wrote:
> > 
> > > This is v3 of my C99-style initializer related patch series. The changes
> > > since v2 are:
> [...]
> > 
> > I like the idea and it makes code cleaner. But doing this introduces lots 
> > of warnings
> > and that is not acceptable.
> > ip
> > CC   ip.o
> > CC   ipaddress.o
> > ipaddress.c: In function ‘print_queuelen’:
> > ipaddress.c:175:10: warning: missing braces around initializer 
> > [-Wmissing-braces]
> >struct ifreq ifr = { 0 };
> >   ^
> 
> I saw these too with gcc-3.4.6 but not with 5.3.0. It appears to be a
> gcc bug[1]. One possible workaround is to match the brace level of the
> first field, but it's quite ugly: [2]. Another way might be to
> initialize one of the fields to zero, like so:
> 
> | struct ifreq ifr = { .ifr_qlen = 0 };
> 
> What do you think?
> 
> Thanks, Phil
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
> [2] 
> http://nwl.cc/cgi-bin/git/gitweb.cgi?p=iproute2.git;a=commitdiff;h=a1cbf2b63c995b2f633c5b4699248ab308b201d2;hp=3809cfec65b03716d1d0360338126df4b4f3fbf6

I am using gcc on Debian stable which is 5.3.1.

Re: [iproute PATCH v3 0/6] Big C99 style initializer rework

[Phil Sutter]

Hi,

On Mon, Jun 27, 2016 at 10:59:12AM -0700, Stephen Hemminger wrote:
> On Thu, 23 Jun 2016 17:34:08 +
> Phil Sutter  wrote:
> 
> > This is v3 of my C99-style initializer related patch series. The changes
> > since v2 are:
[...]
> 
> I like the idea and it makes code cleaner. But doing this introduces lots of 
> warnings
> and that is not acceptable.
> ip
> CC   ip.o
> CC   ipaddress.o
> ipaddress.c: In function ‘print_queuelen’:
> ipaddress.c:175:10: warning: missing braces around initializer 
> [-Wmissing-braces]
>struct ifreq ifr = { 0 };
>   ^

I saw these too with gcc-3.4.6 but not with 5.3.0. It appears to be a
gcc bug[1]. One possible workaround is to match the brace level of the
first field, but it's quite ugly: [2]. Another way might be to
initialize one of the fields to zero, like so:

| struct ifreq ifr = { .ifr_qlen = 0 };

What do you think?

Thanks, Phil

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
[2] 
http://nwl.cc/cgi-bin/git/gitweb.cgi?p=iproute2.git;a=commitdiff;h=a1cbf2b63c995b2f633c5b4699248ab308b201d2;hp=3809cfec65b03716d1d0360338126df4b4f3fbf6

Re: [iproute PATCH v3 0/6] Big C99 style initializer rework

[Stephen Hemminger]

On Thu, 23 Jun 2016 17:34:08 +
Phil Sutter  wrote:

> This is v3 of my C99-style initializer related patch series. The changes
> since v2 are:
> 
> - Flattened embedded struct's initializers:
>   Since the field names are very short, I figured it makes more sense to
>   keep indenting low. Also, the same style is already used in
>   ip/xfrm_policy.c so take that as an example.
> 
> - Moved leftover nlmsg_seq initializing into the common place as well:
>   I was unsure whether this is a good idea at first (due to the
>   increment), but again it's done in ip/xfrm_policy.c as well so should
>   be fine.
> 
> - Added a comma after the last field initializer as suggested by Jakub.
> 
> - Dropped patch 7 since it was NACKed.
> 
> - Eliminated checkpatch non-compliance.
> 
> - Second go at union bpf_attr in tc/tc_bpf.c:
>   I figured that while it is not possible to initialize fields, gcc-3.4.6
>   does not complain when setting the whole union to zero using '= {0}'.
>   So I did this and thereby at least got rid of the memset calls.
> 
> For reference, here's the v2 changelog:
> 
> - Rebased onto current upstream master:
>   My own commit a0a73b298a579 ("tc: m_action: Use C99 style initializers
>   for struct req") contains most of the changes to tc/m_action.c already,
>   so I put the remaining ones into a dedicated patch (the first one here)
>   with a better description.
> 
> - Tested against gcc-3.4.6:
>   This is the oldest gcc version I was able to install locally. It indeed
>   does not like the former changes in tc/tc_bpf.c, so I reverted them.
>   Apart from emitting many warnings, it successfully compiles the
>   sources.
> 
> In the process of compatibility testing, I made a few more changes which
> make sense to have:
> 
> - New patch 5 allows to conveniently override the compiler via command
>   line.
> 
> - New patch 6 eliminates a warning with old gcc but looks valid in
>   general.
> 
> - A warning made me look at ip/tcp_metrics.c and I found a minor code
>   simplification (patch 7).
> 
> Phil Sutter (6):
>   tc: m_action: Improve conversion to C99 style initializers
>   Use C99 style initializers everywhere
>   Replace malloc && memset by calloc
>   No need to initialize rtattr fields before parsing
>   Makefile: Allow to override CC
>   misc/ifstat: simplify unsigned value comparison
> 
>  Makefile   |   4 +-
>  bridge/fdb.c   |  25 ++--
>  bridge/link.c  |  14 +++
>  bridge/mdb.c   |  17 -
>  bridge/vlan.c  |  17 -
>  genl/ctrl.c|  44 +
>  genl/genl.c|   3 +-
>  ip/ip6tunnel.c |  10 ++---
>  ip/ipaddress.c |  33 +++-
>  ip/ipaddrlabel.c   |  21 --
>  ip/iplink.c|  61 -
>  ip/iplink_can.c|   4 +-
>  ip/ipmaddr.c   |  25 
>  ip/ipmroute.c  |   8 +---
>  ip/ipneigh.c   |  30 ++-
>  ip/ipnetconf.c |  10 ++---
>  ip/ipnetns.c   |  39 +--
>  ip/ipntable.c  |  25 
>  ip/iproute.c   |  78 +
>  ip/iprule.c|  22 +--
>  ip/iptoken.c   |  19 -
>  ip/iptunnel.c  |  31 +--
>  ip/ipxfrm.c|  26 -
>  ip/link_gre.c  |  18 -
>  ip/link_gre6.c |  18 -
>  ip/link_ip6tnl.c   |  25 +---
>  ip/link_iptnl.c|  22 +--
>  ip/link_vti.c  |  18 -
>  ip/link_vti6.c |  18 -
>  ip/xfrm_policy.c   |  99 +++
>  ip/xfrm_state.c| 110 
> ++---
>  lib/libnetlink.c   |  77 ++---
>  lib/ll_map.c   |   1 -
>  lib/names.c|   7 +---
>  misc/arpd.c|  64 ++-
>  misc/ifstat.c  |   2 +-
>  misc/lnstat.c  |   6 +--
>  misc/lnstat_util.c |   4 +-
>  misc/ss.c  |  37 +++---
>  tc/e_bpf.c |   7 +---
>  tc/em_canid.c  |   4 +-
>  tc/em_cmp.c|   4 +-
>  tc/em_ipset.c  |   4 +-
>  tc/em_meta.c   |   4 +-
>  tc/em_nbyte.c  |   4 +-
>  tc/em_u32.c|   4 +-
>  tc/f_flow.c|   3 --
>  tc/f_flower.c  |   3 +-
>  tc/f_fw.c  |   6 +--
>  tc/f_route.c   |   3 --
>  tc/f_rsvp.c|   6 +--
>  tc/f_u32.c |  12 ++
>  tc/m_action.c  |  26 -
>  tc/m_bpf.c |   5 +--
>  tc/m_csum.c|   4 +-
>  tc/m_ematch.c  |   4 +-
>  tc/m_gact.c|   5 +--
>  tc/m_ife.c |   5 +--
>  tc/m_ipt.c |  13 ++-
>  tc/m_mirred.c  |   7 +---
>  tc/m_nat.c |   4 +-
>  tc/m_pedit.c   |  11 ++
>  tc/m_police.c  |   5 +--
>  tc/q_atm.c |   3 +-
>  tc/q_cbq.c |  22 +++
>  tc/q_choke.c   |   4 +-
>  tc/q_codel.c   |   3 +-
>  tc/q_dsmark.c  |   1 -
>  tc/q_fifo.c|   4 +-

Re: [iproute PATCH v3 0/6] Big C99 style initializer rework

[Nicolas Dichtel]

Le 23/06/2016 19:34, Phil Sutter a écrit :
> This is v3 of my C99-style initializer related patch series. The changes
> since v2 are:
Compile-tested with a gcc 4.4.7.


Regards,
Nicolas

Re: [iproute PATCH v3 0/6] Big C99 style initializer rework

[Phil Sutter]

Hi,

On Fri, Jun 24, 2016 at 09:17:07AM +, David Laight wrote:
> From: Phil Sutter
> > Sent: 23 June 2016 18:34
> >
> > This is v3 of my C99-style initializer related patch series.
> ...
> 
> It would be interesting to know how this affect the kernel code size?
> 
> While gcc will generate a memset() call for 'struct foo = {0}' if you
> initialise some members it might generate explicit zeroing instructions
> for all the other words of the structure.
> 
> I've seen gcc use memset() to zero the end of a structure, it may use
> memset() for large gaps earlier in the structure.
> 
> But if you initialise a byte half way down you are very unlikely to
> get a single memset() and then a write to the single location.

I did a standard build ('make distclean; make') before and after this
commit in my tree. The 'ip' binary didn't change in size at all (quite
surprising), the 'tc' binary shrunk by 48 bytes.

Cheers, Phil

RE: [iproute PATCH v3 0/6] Big C99 style initializer rework

[David Laight]

From: Phil Sutter
> Sent: 23 June 2016 18:34
>
> This is v3 of my C99-style initializer related patch series.
...

It would be interesting to know how this affect the kernel code size?

While gcc will generate a memset() call for 'struct foo = {0}' if you
initialise some members it might generate explicit zeroing instructions
for all the other words of the structure.

I've seen gcc use memset() to zero the end of a structure, it may use
memset() for large gaps earlier in the structure.

But if you initialise a byte half way down you are very unlikely to
get a single memset() and then a write to the single location.

David