svn commit: r314989 - head/usr.bin/vmstat

[Marcelo Araujo]

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 at optusnet.com.au> 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.
>>
>
> 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
>