Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 11 Mar 2017 00:01:12 -0500
From:      Pedro Giffuni <pfg@FreeBSD.org>
To:        Bruce Evans <brde@optusnet.com.au>, Ngie Cooper <yaneurabeya@gmail.com>
Cc:        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:  <1acd3d70-acf9-27a2-3a0f-874846d3c1f0@FreeBSD.org>
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


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@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




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1acd3d70-acf9-27a2-3a0f-874846d3c1f0>