From: Julia Lawall <[email protected]>
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 <[email protected]>
---
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;
}
On Tue, Feb 26, 2008 at 10:44 PM, Julia Lawall <[email protected]> wrote:
> From: Julia Lawall <[email protected]>
>
> 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 <[email protected]>
>
> ---
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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
[email protected]
* Harvey Harrison <[email protected]> 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
* Julia Lawall <[email protected]> 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
* Christopher Li <[email protected]> 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
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 <[email protected]> wrote:
>
> * Julia Lawall <[email protected]> wrote:
>
> > From: Julia Lawall <[email protected]>
> >
> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
* Bart Van Assche <[email protected]> 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
On Wed, Mar 5, 2008 at 1:20 PM, Ingo Molnar <[email protected]> wrote:
>
> * Julia Lawall <[email protected]> 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.
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
>
On Wed, 5 Mar 2008, Ingo Molnar wrote:
>
> * Julia Lawall <[email protected]> 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
On Wed, 2008-03-05 at 08:02 +0100, Ingo Molnar wrote:
> * Christopher Li <[email protected]> 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
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):
http://www.knosof.co.uk/cbook/accu06a.pdf
http://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:[email protected]
Applications Standards Conformance Testing http://www.knosof.co.uk
* Julia Lawall <[email protected]> wrote:
> From: Julia Lawall <[email protected]>
>
> 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