svn commit: r314989 - head/usr.bin/vmstat
Pedro Giffuni
pfg at FreeBSD.org
Sat Mar 11 04:58:15 UTC 2017
On 3/10/2017 8:29 PM, Bruce Evans wrote:
> On Fri, 10 Mar 2017, Ngie Cooper wrote:
>
>>> On Mar 10, 2017, at 03:59, Pedro Giffuni <pfg at 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.
>
As I said I hadn't looked at the code; the "c" is holding the return
value of getopt() and later kvm_nlist() before getting reused as a loop
index so it must be an int.
I would think it is such reuse of the same variable which is poisonous,
although it does involve some efficiency.
Pedro.
> 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
More information about the svn-src-head
mailing list