[PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

In commit e6bafba5b4765a5a252f1b8d31cbf6d2459da337, a bug was fixed that
involved converting !x & y to !(x & y).  The code below shows the same
pattern, and thus should perhaps be fixed in the same way.

This is not tested and clearly changes the semantics, so it is only
something to consider.

The semantic patch that makes this change is as follows:
(http://www.emn.fr/x-info/coccinelle/)

// <smpl>
@@ expression E1,E2; @@
(
  !E1 & !E2
|
- !E1 & E2
+ !(E1 & E2)
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---

diff -u -p a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-02-05 20:56:01.000000000 +0100
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-02-26 08:09:12.000000000 +0100
@@ -4589,7 +4589,7 @@ static u8 iwl4965_is_fat_tx_allowed(stru
 
 	if (sta_ht_inf) {
 		if ((!sta_ht_inf->ht_supported) ||
-		   (!sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH))
+		   (!(sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH)))
 			return 0;
 	}
 

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Tomas Winkler]

On Tue, Feb 26, 2008 at 10:44 PM, Julia Lawall <julia@diku.dk> wrote:
> From: Julia Lawall <julia@diku.dk>
>
>  In commit e6bafba5b4765a5a252f1b8d31cbf6d2459da337, a bug was fixed that
>  involved converting !x & y to !(x & y).  The code below shows the same
>  pattern, and thus should perhaps be fixed in the same way.
>
>  This is not tested and clearly changes the semantics, so it is only
>  something to consider.
>
>  The semantic patch that makes this change is as follows:
>  (http://www.emn.fr/x-info/coccinelle/)
>
>  // <smpl>
>  @@ expression E1,E2; @@
>  (
>   !E1 & !E2
>  |
>  - !E1 & E2
>  + !(E1 & E2)
>  )
>  // </smpl>
>
>  Signed-off-by: Julia Lawall <julia@diku.dk>
>
>  ---
>
>  diff -u -p a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
>  --- a/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-02-05 20:56:01.000000000 +0100
>  +++ b/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-02-26 08:09:12.000000000 +0100
>  @@ -4589,7 +4589,7 @@ static u8 iwl4965_is_fat_tx_allowed(stru
>
>         if (sta_ht_inf) {
>                 if ((!sta_ht_inf->ht_supported) ||
>  -                  (!sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH))
>  +                  (!(sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH)))
>                         return 0;
>         }

The patch was already submitted by Roel Kluin '[PATCH]
[wireless/iwlwifi/iwl-4965.c] add parentheses' I've acked it.
Didn't track it if it's actually committed into tree... John, Reinette ?

Thanks
Tomas


>  -
>  To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>  the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[John W. Linville]

On Wed, Feb 27, 2008 at 12:47:56AM +0200, Tomas Winkler wrote:

> >  diff -u -p a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
> >  --- a/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-02-05 20:56:01.000000000 +0100
> >  +++ b/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-02-26 08:09:12.000000000 +0100
> >  @@ -4589,7 +4589,7 @@ static u8 iwl4965_is_fat_tx_allowed(stru
> >
> >         if (sta_ht_inf) {
> >                 if ((!sta_ht_inf->ht_supported) ||
> >  -                  (!sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH))
> >  +                  (!(sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH)))
> >                         return 0;
> >         }
> 
> The patch was already submitted by Roel Kluin '[PATCH]
> [wireless/iwlwifi/iwl-4965.c] add parentheses' I've acked it.
> Didn't track it if it's actually committed into tree... John, Reinette ?

Already merged and sent to davem...

Thanks,

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Ingo Molnar]

* Julia Lawall <julia@diku.dk> wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> In commit e6bafba5b4765a5a252f1b8d31cbf6d2459da337, a bug was fixed 
> that involved converting !x & y to !(x & y).  The code below shows the 
> same pattern, and thus should perhaps be fixed in the same way.

>  	if (sta_ht_inf) {
>  		if ((!sta_ht_inf->ht_supported) ||
> -		   (!sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH))
> +		   (!(sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH)))
>  			return 0;

i'm wondering, could Sparse be extended to check for such patterns? 

People are regularly running "make C=1" and are sending fixes to make 
entire subsystems sparse-warning-free, so Sparse is a nice mechanism 
that works and it keeps code clean in the long run.

I dont think the "!X & Y" pattern is ever used legitimately [and even if 
it were used legitimately, it's easy to avoid the sparse false positive 
- while in the buggy case we have a clear bug].

	Ingo

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Christopher Li]

I think Al Viro has sent a patch to linux-sparse with subject
"[PATCH 3/3] catch !x & y brainos" does exactly that.

Chris



On Tue, Mar 4, 2008 at 10:38 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
>  * Julia Lawall <julia@diku.dk> wrote:
>
>  > From: Julia Lawall <julia@diku.dk>
>  >
>  > In commit e6bafba5b4765a5a252f1b8d31cbf6d2459da337, a bug was fixed
>  > that involved converting !x & y to !(x & y).  The code below shows the
>  > same pattern, and thus should perhaps be fixed in the same way.
>
>
> >       if (sta_ht_inf) {
>  >               if ((!sta_ht_inf->ht_supported) ||
>  > -                (!sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH))
>  > +                (!(sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH)))
>  >                       return 0;
>
>  i'm wondering, could Sparse be extended to check for such patterns?
>
>  People are regularly running "make C=1" and are sending fixes to make
>  entire subsystems sparse-warning-free, so Sparse is a nice mechanism
>  that works and it keeps code clean in the long run.
>
>  I dont think the "!X & Y" pattern is ever used legitimately [and even if
>  it were used legitimately, it's easy to avoid the sparse false positive
>  - while in the buggy case we have a clear bug].
>
>         Ingo
>  --
>  To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
>
>
> the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Ingo Molnar]

* Christopher Li <sparse@chrisli.org> wrote:

> I think Al Viro has sent a patch to linux-sparse with subject "[PATCH 
> 3/3] catch !x & y brainos" does exactly that.

ah - nice :-)

/me checks the linux-sparse archive

Al's patch is:

+                       if (op == '&' && expr->left->type == EXPR_PREOP &&
+                           expr->left->op == '!')
+                               warning(expr->pos, "dubious: !x & y");

i think there might be similar patterns: "x & !y", "!x | y", "x | !y" ?

	Ingo

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Harvey Harrison]

On Wed, 2008-03-05 at 08:02 +0100, Ingo Molnar wrote:
> * Christopher Li <sparse@chrisli.org> wrote:
> 
> > I think Al Viro has sent a patch to linux-sparse with subject "[PATCH 
> > 3/3] catch !x & y brainos" does exactly that.
> 
> ah - nice :-)
> 
> /me checks the linux-sparse archive
> 
> Al's patch is:
> 
> +                       if (op == '&' && expr->left->type == EXPR_PREOP &&
> +                           expr->left->op == '!')
> +                               warning(expr->pos, "dubious: !x & y");
> 
> i think there might be similar patterns: "x & !y", "!x | y", "x | !y" ?
> 

Well, (!x & y) and (!x | y) are probably the two that might have been
intended otherwise.  (x & !y), (x | !y) are probably ok.

Harvey

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Ingo Molnar]

* Harvey Harrison <harvey.harrison@gmail.com> wrote:

> > Al's patch is:
> > 
> > +                       if (op == '&' && expr->left->type == EXPR_PREOP &&
> > +                           expr->left->op == '!')
> > +                               warning(expr->pos, "dubious: !x & y");
> > 
> > i think there might be similar patterns: "x & !y", "!x | y", "x | !y" ?
> > 
> 
> Well, (!x & y) and (!x | y) are probably the two that might have been 
> intended otherwise.  (x & !y), (x | !y) are probably ok.

i think the proper intention in the latter cases is (x & ~y) and
(x | ~y).

My strong bet is that in 99% of the cases they are real bugs and && or 
|| was intended.

	Ingo

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Julia Lawall]

There are some legitimate uses of !x & y which are actually of the form !x 
& !y, where x and y are function calls.  That is a not particularly 
elegant way of getting both x and y to be evaluated and then combining the 
results using "and".  If such code is considered acceptable, then perhaps 
the sparse patch should be more complicated.

julia

> Al's patch is:
> 
> +                       if (op == '&' && expr->left->type == EXPR_PREOP &&
> +                           expr->left->op == '!')
> +                               warning(expr->pos, "dubious: !x & y");
> 
> i think there might be similar patterns: "x & !y", "!x | y", "x | !y" ?
> 
> 	Ingo
> 

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Derek M Jones]

All,

>>> i think there might be similar patterns: "x & !y", "!x | y", "x | !y" ?
>>>
>> Well, (!x & y) and (!x | y) are probably the two that might have been 
>> intended otherwise.  (x & !y), (x | !y) are probably ok.
> 
> i think the proper intention in the latter cases is (x & ~y) and
> (x | ~y).
> 
> My strong bet is that in 99% of the cases they are real bugs and && or 
> || was intended.

Developer knowledge of operator precedence and the issue of what
they intended to write are interesting topics.  Some experimental
work is described in (binary operators only I'm afraid):

www.knosof.co.uk/cbook/accu06a.pdf
www.knosof.co.uk/cbook/accu07a.pdf

The ACCU 2006 experiment provides evidence that developer knowledge
is proportional to the number of occurrences of a construct in
source code, it also shows a stunningly high percentage of incorrect
answers.

The ACCU 2007 experiment provides evidence that the names of the
operands has a significant impact on operator precedence choice.

I wonder what kind of names are used as the operand of unary
operators?

I would expect the ~ operator to have a bitwise name, but the
! operator might have an arithmetic or bitwise name.

-- 
Derek M. Jones                              tel: +44 (0) 1252 520 667
Knowledge Software Ltd                      mailto:derek@knosof.co.uk
Applications Standards Conformance Testing    http://www.knosof.co.uk

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Ingo Molnar]

* Julia Lawall <julia@diku.dk> wrote:

> There are some legitimate uses of !x & y which are actually of the 
> form !x & !y, where x and y are function calls.  That is a not 
> particularly elegant way of getting both x and y to be evaluated and 
> then combining the results using "and".  If such code is considered 
> acceptable, then perhaps the sparse patch should be more complicated.

i tend to be of the opinion that the details in C source code should be 
visually obvious and should be heavily simplified down from what is 
'possible' language-wise - with most deviations and complications that 
depart from convention considered an error. I'd consider "!fn1() & 
!fn2()" a borderline coding style violation in any case - and it costs 
nothing to change it to "!fn1() && !fn2()".

	Ingo

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Bart Van Assche]

On Wed, Mar 5, 2008 at 1:20 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
>  * Julia Lawall <julia@diku.dk> wrote:
>
>  > There are some legitimate uses of !x & y which are actually of the
>  > form !x & !y, where x and y are function calls.  That is a not
>  > particularly elegant way of getting both x and y to be evaluated and
>  > then combining the results using "and".  If such code is considered
>  > acceptable, then perhaps the sparse patch should be more complicated.
>
>  i tend to be of the opinion that the details in C source code should be
>  visually obvious and should be heavily simplified down from what is
>  'possible' language-wise - with most deviations and complications that
>  depart from convention considered an error. I'd consider "!fn1() &
>  !fn2()" a borderline coding style violation in any case - and it costs
>  nothing to change it to "!fn1() && !fn2()".

If someone writes (!x & !y) instead of (!x && !y) because both x and y
have to be evaluated, this means that both x and y have side effects.
Please keep in mind that the C language does not specify whether x or
y has to be evaluated first, so if x and y have to be evaluated in
that order, an expression like (!x & !y) can be the cause of very
subtle bugs. I prefer readability above brevity.

Bart Van Assche.

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Julia Lawall]

On Wed, 5 Mar 2008, Ingo Molnar wrote:

>
> * Julia Lawall <julia@diku.dk> wrote:
>
> > There are some legitimate uses of !x & y which are actually of the
> > form !x & !y, where x and y are function calls.  That is a not
> > particularly elegant way of getting both x and y to be evaluated and
> > then combining the results using "and".  If such code is considered
> > acceptable, then perhaps the sparse patch should be more complicated.
>
> i tend to be of the opinion that the details in C source code should be
> visually obvious and should be heavily simplified down from what is
> 'possible' language-wise - with most deviations and complications that
> depart from convention considered an error. I'd consider "!fn1() &
> !fn2()" a borderline coding style violation in any case - and it costs
> nothing to change it to "!fn1() && !fn2()".

!fn1() && !fn2() does not have the same semantics as !fn1() & !fn2().  In
!fn1() & !fn2() both function calls are evaluated.  In !fn1() && !fn2(),
if !fn1() returns false then !fn2() is not evaluated.  I haven't studied
the particular instances of fn2(), though, to know whether it makes a
difference.

One could instead do something like:

x = fn1();
y = fn2();
if (!x && !y) ...

It would certainly be clearer, but more verbose.

julia

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Ingo Molnar]

* Bart Van Assche <bart.vanassche@gmail.com> wrote:

> If someone writes (!x & !y) instead of (!x && !y) because both x and y 
> have to be evaluated, this means that both x and y have side effects.
> Please keep in mind that the C language does not specify whether x or 
> y has to be evaluated first, so if x and y have to be evaluated in 
> that order, an expression like (!x & !y) can be the cause of very 
> subtle bugs. I prefer readability above brevity.

such expressions _must_ be written as:

  ret1 = x();
  ret2 = y();

  if (ret1 && ret2)
	...

any side-effects are totally un-obvious when they are in expressions and 
someone doing cleanups later on could easily change the '&' to '&&' and 
introduce a bug.

	Ingo