From owner-svn-src-all@freebsd.org Wed Jun 6 02:54:21 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 65299FDDE63; Wed, 6 Jun 2018 02:54:21 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id AC37F7FC98; Wed, 6 Jun 2018 02:54:20 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id 411C642A6CF; Wed, 6 Jun 2018 12:54:18 +1000 (AEST) Date: Wed, 6 Jun 2018 12:54:17 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eric van Gyzen cc: Ian Lepore , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r334669 - head/sys/sys In-Reply-To: Message-ID: <20180606122034.I994@besplex.bde.org> References: <201806052034.w55KYBsb096418@repo.freebsd.org> <1528232004.63685.25.camel@freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=FNpr/6gs c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=i8ZEjPkR5nfDrTPL43oA:9 a=dMDrLXoxEuWxAglj:21 a=xJ1qA4LxLCKInB2S:21 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Jun 2018 02:54:21 -0000 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