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.
The comment explains that short circuiting here is undesirable, as the
calls to test_and_{clear,set}_bit() need to happen for both sides of the
expression.
Clang's suggestion would work to silence the warning but the readability
of the expression would suffer even more. To clean up the warning and
make the block more readable, use a variable for each side of the
bitwise expression.
Link: https://github.com/ClangBuiltLinux/linux/issues/1478
Signed-off-by: Nathan Chancellor <[email protected]>
---
drivers/staging/wlan-ng/hfa384x_usb.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
index 59aa84d1837d..938e11a1a0b6 100644
--- a/drivers/staging/wlan-ng/hfa384x_usb.c
+++ b/drivers/staging/wlan-ng/hfa384x_usb.c
@@ -3778,18 +3778,18 @@ static void hfa384x_usb_throttlefn(struct timer_list *t)
spin_lock_irqsave(&hw->ctlxq.lock, flags);
- /*
- * We need to check BOTH the RX and the TX throttle controls,
- * so we use the bitwise OR instead of the logical OR.
- */
pr_debug("flags=0x%lx\n", hw->usb_flags);
- if (!hw->wlandev->hwremoved &&
- ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
- !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags)) |
- (test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
- !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags))
- )) {
- schedule_work(&hw->usb_work);
+ if (!hw->wlandev->hwremoved) {
+ bool rx_throttle = test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
+ !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags);
+ bool tx_throttle = test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
+ !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags);
+ /*
+ * We need to check BOTH the RX and the TX throttle controls,
+ * so we use the bitwise OR instead of the logical OR.
+ */
+ if (rx_throttle | tx_throttle)
+ schedule_work(&hw->usb_work);
}
spin_unlock_irqrestore(&hw->ctlxq.lock, flags);
base-commit: 6ac113f741a7674e4268eea3eb13972732d83571
--
2.33.1.637.gf443b226ca
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.
regards,
dan carpenter
On Fri, Oct 15, 2021 at 12:43:44PM +0300, Dan Carpenter 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.
I do not disagree. The original motivation for the warning was code like
if (a() & b())
where a '&&' was intended to short circuit the call to b() if a() was
false but then it expanded to encompass bitwise OR as well. The clang
developers felt that warning on bitwise OR was worthwhile because most
of the time, '||' was intended. Feel free to comment on the Phabricator
thread if you feel strongly, there are not too many instances of this
warning and I think the '&' vs '&&' aspect of the warning is useful.
https://reviews.llvm.org/D108003
Cheers,
Nathan
On Fri, Oct 15, 2021 at 10:13:05AM -0700, Nathan Chancellor wrote:
> On Fri, Oct 15, 2021 at 12:43:44PM +0300, Dan Carpenter 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.
>
> I do not disagree. The original motivation for the warning was code like
>
> if (a() & b())
>
> where a '&&' was intended to short circuit the call to b() if a() was
> false but then it expanded to encompass bitwise OR as well. The clang
> developers felt that warning on bitwise OR was worthwhile because most
> of the time, '||' was intended. Feel free to comment on the Phabricator
> thread if you feel strongly, there are not too many instances of this
> warning and I think the '&' vs '&&' aspect of the warning is useful.
Of course, this was a Smatch check early on. Bool is almost all style
debates and false positives. I didn't see a lot of short circuiting
bugs in the kernel.
But I'm not going to tell people how to live their lifes if they want to
spend it debating coding style. (Unless it's GCC's brain dead unsigned
comparison warnings where it tells everyone to make their iterators
unsigned int and leads to a lot bugs and ugly code).
regards,
dan carpenter
On Fri, Oct 15, 2021 at 2:44 AM Dan Carpenter <[email protected]> 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 `||`.
--
Thanks,
~Nick Desaulniers
On Thu, Oct 14, 2021 at 2:57 PM Nathan Chancellor <[email protected]> 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.
>
> The comment explains that short circuiting here is undesirable, as the
> calls to test_and_{clear,set}_bit() need to happen for both sides of the
> expression.
>
> Clang's suggestion would work to silence the warning but the readability
> of the expression would suffer even more. To clean up the warning and
> make the block more readable, use a variable for each side of the
> bitwise expression.
Sure. Thanks for the patch!
Reviewed-by: Nick Desaulniers <[email protected]>
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1478
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> drivers/staging/wlan-ng/hfa384x_usb.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
> index 59aa84d1837d..938e11a1a0b6 100644
> --- a/drivers/staging/wlan-ng/hfa384x_usb.c
> +++ b/drivers/staging/wlan-ng/hfa384x_usb.c
> @@ -3778,18 +3778,18 @@ static void hfa384x_usb_throttlefn(struct timer_list *t)
>
> spin_lock_irqsave(&hw->ctlxq.lock, flags);
>
> - /*
> - * We need to check BOTH the RX and the TX throttle controls,
> - * so we use the bitwise OR instead of the logical OR.
> - */
> pr_debug("flags=0x%lx\n", hw->usb_flags);
> - if (!hw->wlandev->hwremoved &&
> - ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> - !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags)) |
> - (test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
> - !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags))
> - )) {
> - schedule_work(&hw->usb_work);
> + if (!hw->wlandev->hwremoved) {
> + bool rx_throttle = test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> + !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags);
> + bool tx_throttle = test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
> + !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags);
> + /*
> + * We need to check BOTH the RX and the TX throttle controls,
> + * so we use the bitwise OR instead of the logical OR.
> + */
> + if (rx_throttle | tx_throttle)
> + schedule_work(&hw->usb_work);
> }
>
> spin_unlock_irqrestore(&hw->ctlxq.lock, flags);
>
> base-commit: 6ac113f741a7674e4268eea3eb13972732d83571
> --
> 2.33.1.637.gf443b226ca
>
--
Thanks,
~Nick Desaulniers
From: Nathan Chancellor
> Sent: 14 October 2021 22:57
>
> 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.
>
> The comment explains that short circuiting here is undesirable, as the
> calls to test_and_{clear,set}_bit() need to happen for both sides of the
> expression.
>
> Clang's suggestion would work to silence the warning but the readability
> of the expression would suffer even more. To clean up the warning and
> make the block more readable, use a variable for each side of the
> bitwise expression.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1478
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> drivers/staging/wlan-ng/hfa384x_usb.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
> index 59aa84d1837d..938e11a1a0b6 100644
> --- a/drivers/staging/wlan-ng/hfa384x_usb.c
> +++ b/drivers/staging/wlan-ng/hfa384x_usb.c
> @@ -3778,18 +3778,18 @@ static void hfa384x_usb_throttlefn(struct timer_list *t)
>
> spin_lock_irqsave(&hw->ctlxq.lock, flags);
>
> - /*
> - * We need to check BOTH the RX and the TX throttle controls,
> - * so we use the bitwise OR instead of the logical OR.
> - */
> pr_debug("flags=0x%lx\n", hw->usb_flags);
> - if (!hw->wlandev->hwremoved &&
> - ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> - !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags)) |
> - (test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
> - !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags))
> - )) {
> - schedule_work(&hw->usb_work);
> + if (!hw->wlandev->hwremoved) {
> + bool rx_throttle = test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
> + !test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags);
> + bool tx_throttle = test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
> + !test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags);
> + /*
> + * We need to check BOTH the RX and the TX throttle controls,
> + * so we use the bitwise OR instead of the logical OR.
> + */
> + if (rx_throttle | tx_throttle)
> + schedule_work(&hw->usb_work);
Why not the slightly simpler:
bool throttle = test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) &&
!test_and_set_bit(WORK_RX_RESUME, &hw->usb_flags);
throttle |= test_and_clear_bit(THROTTLE_TX, &hw->usb_flags) &&
!test_and_set_bit(WORK_TX_RESUME, &hw->usb_flags);
if (throttle)
schedule_work(&hw->usb_work);
or with s/throttle/throttle_silly_compiler_warning/ :-)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, Oct 18, 2021 at 01:23:45PM -0700, Nick Desaulniers wrote:
> On Fri, Oct 15, 2021 at 2:44 AM Dan Carpenter <[email protected]> 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