Date: Sat, 11 Mar 2017 12:29:47 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ngie Cooper <yaneurabeya@gmail.com> Cc: Pedro Giffuni <pfg@freebsd.org>, Bruce Evans <brde@optusnet.com.au>, Marcelo Araujo <araujo@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r314989 - head/usr.bin/vmstat Message-ID: <20170311120737.R1001@besplex.bde.org> In-Reply-To: <DECB06D9-8136-491E-B786-0C853C70C472@gmail.com> References: <201703100449.v2A4neTK046456@repo.freebsd.org> <20170310181404.C1416@besplex.bde.org> <0906af1d-09aa-fd1e-35cc-9361a68fc160@FreeBSD.org> <DECB06D9-8136-491E-B786-0C853C70C472@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 10 Mar 2017, Ngie Cooper wrote: >> On Mar 10, 2017, at 03:59, Pedro Giffuni <pfg@FreeBSD.org> wrote: >> >>> On 3/10/2017 2:45 AM, Bruce Evans wrote: >>>> On Fri, 10 Mar 2017, Marcelo Araujo wrote: >>>> >>>> ... >>>> Log: >>>> Use nitems() from sys/param.h and also remove the cast. >>>> >>>> Reviewed by: markj >>>> MFC after: 3 weeks. >>>> Differential Revision: https://reviews.freebsd.org/D9937 >>>> ... >>>> Modified: head/usr.bin/vmstat/vmstat.c >>>> ============================================================================== >>>> --- head/usr.bin/vmstat/vmstat.c Fri Mar 10 04:30:31 2017 (r314988) >>>> +++ head/usr.bin/vmstat/vmstat.c Fri Mar 10 04:49:40 2017 (r314989) >>>> @@ -288,17 +288,13 @@ retry_nlist: >>>> namelist[X_SUM].n_name = "_cnt"; >>>> goto retry_nlist; >>>> } >>>> - for (c = 0; >>>> - c < (int)(sizeof(namelist)/sizeof(namelist[0])); >>>> - c++) >>>> + for (c = 0; c < nitems(namelist); c++) >>>> if (namelist[c].n_type == 0) >>>> bufsize += strlen(namelist[c].n_name) + 1; >>> >>> This undoes fixes to compile at WARNS=2 in r87690 and now breaks at WARNS=3. >>> vmstat is still compiled at WARNS=1. >>> >>> nitems suffers from the same unsigned poisoning as the sizeof() expression >>> (since it reduces to the same expression. Casting to int in the expression >>> to fix the warning would break exotic cases. Of course, nitems is >>> undocumented so no one knows when it is supposed to work). >>> >>> vmstat compiles with no errors at WARNS=2. At WARNS=3, it used to compile >>> with 9 excessive warnings (about the good style of omitting redundant >>> initializers). Now it compiles with 10 excessive warnings. 1 more about >>> comparison between signed unsigned. This warning is a compiler bug. Both >>> gcc-4.2.1 and clang-3.9.0 have it. It is enabled by -W, which is put in >>> CFLAGS at WARNS >= 3, or by -Wsign-compare. >>> >>> These compilers even complain about: >>> >>> int c; >>> >>> for (c = 0; c < 1U; c++) >>> foo(); >>> >>> where it is extremely clear that c never gets converted to a wrong value >>> when it is promoted to unsigned for the comparison. Compilers should >>> only warn about sign mismatches if they can't figure out the ranges or >>> if they can figure out the ranges but dangerous promotiions occur. >>> Compilers do excessive unrolling and other optimizations of loops like >>> the above, and they must figure out the ranges for this. >>> >> >> I haven't looked at the code but it would seem like you can unsign c and avoid the cast. That would be much worse than the cast. The cast is at least in the right place -- it unpoisons nitimems. "Unsigning" c would poison C. > Yeah. This might introduce a domino effect though of changes. Domino effect = fast poisoning. The dominos collapse immediately, but poisoning takes a long time to spread. In K&R-1 size_t didn't even exist and the type of sizeof() was unspecified. size_t was born with unsign poisoning a little later. That is almost 40 years ago, but its poisoning hasn't spread to many loop variables yet. Full poisoning would spread it to the closure of all uses of the loop variables, or better stopped at the source of the poisoning. In the above, the variable is not used outside of the loop, so the closure is small and easy to see. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170311120737.R1001>