Re: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev
Subject: Re: [PATCH] staging: wlan-ng: Avoid bitwise vs logical OR warning in hfa384x_usb_throttlefn()
Date: Mon, 1 Nov 2021 15:49:34 +0300	[thread overview]
Message-ID: <20211101124934.GA2914@kadam> (raw)
In-Reply-To: <CAKwvOd=eEyjLMSEiBd25-Jkvm0DTFtvcB_EmuRVwqWYjQEvD5w@mail.gmail.com>

On Mon, Oct 18, 2021 at 01:23:45PM -0700, Nick Desaulniers wrote:
> On Fri, Oct 15, 2021 at 2:44 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Thu, Oct 14, 2021 at 02:57:03PM -0700, Nathan Chancellor wrote:
> > > A new warning in clang points out a place in this file where a bitwise
> > > OR is being used with boolean expressions:
> > >
> > > In file included from drivers/staging/wlan-ng/prism2usb.c:2:
> > > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
> > >             ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> > >             ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: note: cast one or both operands to int to silence this warning
> > > 1 warning generated.
> >
> > Both sides of this bitwise OR are bool, so | and || are equivalent
> > logically.  Clang should not warn about it.
> 
> Not when the LHS AND RHS of the the binary operator have side effects,
> which is the only condition under which this warning is emitted.  RHS
> potentially sets a bit, and potentially would not be executed if `|`
> was replaced with `||`.

Yes.  But as in this case, if you silenced the "bitwise-instead-of-logical"
warning in the "obvious way" by changing the | to || then it will
introduce a bug so it's a risky warning.

Ideally everyone would try to understand the code they are changing but
that's just not true in real life.  What happens is that every single
new person compiles the code and sees the warning.  There is only one
or two people who understand the driver code and a hundred people who
compile the code and look at warnings.  So there is a slightly over 98%
chance that the person looking at the warning doesn't understand the
code and they're going to try to fix it.  But instead they're going to
introduce a bug because they're going to change | to ||.

Of course, Nathan and I are a bit experienced so we're careful but other
people are less careful and we've seen this over and over where risky
warnings just introduce bugs.  I saw this a month ago where Smatch
complained about a missing error code.  It was ancient code so
the original author was not arround.  A junior dev saw the bug and
changed it to return -EINVAL.  That Smatch check is very high quality
and it did look like it was supposed to be an error path so the patch
was back ported to stable.  But it turned out that return path was
supposed to be success so it broke the boot.

I haven't looked in detail.  I silenced these warnings in Smatch because
my impression at the time was that there was a high false positive rate.
That's still my impression, but I haven't looked.  It might also be
different for different code bases.

regards,
dan carpenter