Date: Sat, 11 Mar 2017 10:16:08 +0800 From: Marcelo Araujo <araujobsdport@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Pedro Giffuni <pfg@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Ngie Cooper <yaneurabeya@gmail.com>, Marcelo Araujo <araujo@freebsd.org> Subject: Re: svn commit: r314989 - head/usr.bin/vmstat Message-ID: <CAOfEmZgZaoAVOfOFEYg7ZrkZ8XPiKVq_Lw%2BET6ZdLBbCXqN0Zg@mail.gmail.com> In-Reply-To: <20170311120737.R1001@besplex.bde.org> 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> <20170311120737.R1001@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
So, in my understanding, bring that cast back might mitigate a bit and make the situation less worst. Am I right? Br, On Mar 11, 2017 10:29 AM, "Bruce Evans" <brde@optusnet.com.au> wrote: > 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?CAOfEmZgZaoAVOfOFEYg7ZrkZ8XPiKVq_Lw%2BET6ZdLBbCXqN0Zg>