svn commit: r334669 - head/sys/sys

[Bruce Evans]

On Tue, 5 Jun 2018, Eric van Gyzen wrote:

> On 06/05/2018 15:53, Ian Lepore wrote:
>> On Tue, 2018-06-05 at 20:34 +0000, Eric van Gyzen wrote:
>>> Author: vangyzen
>>> Date: Tue Jun\xc2\xa0\xc2\xa05 20:34:11 2018
>>> New Revision: 334669
>>> URL: https://svnweb.freebsd.org/changeset/base/334669
>>>
>>> Log:
>>> \xc2\xa0 Make Coverity more happy with r334545
>>> \xc2\xa0\xc2\xa0
>>> \xc2\xa0 Coverity complains about:
>>> \xc2\xa0\xc2\xa0
>>> \xc2\xa0\xc2\xa0	if (((flags) & M_WAITOK) || _malloc_item != NULL)
>>> \xc2\xa0\xc2\xa0
>>> \xc2\xa0 saying:
>>> \xc2\xa0\xc2\xa0
>>> \xc2\xa0\xc2\xa0	The expression
>>> \xc2\xa0\xc2\xa0		1 /* (2 | 0x100) & 2 */ || _malloc_item != NULL
>>> \xc2\xa0\xc2\xa0	is suspicious because it performs a Boolean operation
>>> \xc2\xa0\xc2\xa0	on a constant other than 0 or 1.
>>> \xc2\xa0\xc2\xa0
>>> \xc2\xa0 Although the code is correct, add "!= 0" to make it slightly
>>> \xc2\xa0 more legible and to silence hundreds(?) of Coverity warnings.
>>> \xc2\xa0\xc2\xa0

>> This is a sad sad thing. Treating (bits & flagconstants) as boolean has
>> a long long history in C. Surely there are literally thousand of
>> occurrances in freebsd code already, so why did this one get flagged?

Indded, it is a bug in Coverity.  However, it is BSD style (counting
instances in 4.4BSD kern, explicit comparison of the results of mask
tests with 0 so as to convert to boolean are much more common if the
test is for == 0 and slightly more common if the test is for != 0).
However, this rule is violated in thousands if not millions of cases.
It is most commonly violated for 'if (error)' which is not a natural
boolean test so it needs the explicit comparison with 0 much more than
a mask test.

> I agree, and I tend to avoid adding "!= 0" unnecessarily, but I don't
> feel very strongly about it.  This macro is expanded in many locations,

I used to feel very strongly that doing '!= 0' and '== 0' for mask tests
was a style bug.  Then I got used to BSD style and now don't mind
'== 0' and merely don't like '!= 0'.  In BSD style, it is doing the inverse
mask tests as a boolean using '!' instead of using '== 0' that is the
style bug.  This makes a lot of sense -- the result of the expression
(foo & MASK) is an integer, and the best way to convert that to boolean
with negative logic is to compare it with '== 0'.  Thus more common
non-BSD style of !(foo & MASK) involves implicit conversion of an 
integer to a boolean.  The bug in Coverity should cause a warning about
that too.

I still feel very strongly about 'if (error)' and 'if (!error)' for non-
boolean error, so I think it is not a bug for Coverity to warn about these.
However, the conversion for !error is the same as for !(foo & MASK).

If this is not a bug in Coverity, then it is a bug in Coverity to not
warn about implicit narrowing conversions from integer to bool (and
implicit narrowing conversions generally).  It is very convenient and
usually safest to not have to use a cast for these.  However, such
conversions aren't very clear and have runtime costs.  E.g, the
'if (error)' test can be written as 'bool e; e = error; if (e)'.  If
all the code is visible to the compiler, then it can optimize away the
conversion and generate the same code.  Otherwise, it must produce
extra code to convert to 0 or 1.

> so the number of Coverity warnings increased by hundreds in the most
> recent run.  I care about that more than avoiding "!= 0".  I don't

There are currently 736 instances if 'if (error)' in kern.  Do you really
wish to fix them all?  There are currently only 659 instances of
'if (error != 0)' in kern, so the worse style is still more common for
this.  In kern in 4.4BSD, there were 68 instances of 'if (error)' and
zero instances of 'if (error != 0)'.  This shows both the bloat in
-current and its drift from old styles.  The total file size is only
8 times larger.

Mask tests are harder to grep for.

> sprinkle crap all over the code just to appease Coverity, but this one
> seemed perfectly reasonable.  It makes the code slightly more clear and
> legible for some readers, and I imagine it doesn't hurt the others.
>
> Yes, there are probably many old occurrences of this, and there might be
> many old corresponding warnings, but I tend to focus on the recently
> added ones, just because they're more likely relevant.
>
> /me opens the static analysis can of worms...again

Parts related to style should be separate and optional and usually not
enabled by default.

Bruce