["if (!x & y)" patch from yanzheng@]
["if (!x & y)" patch from adobriyan@]
["if (!x & y)" patches from viro@]
While we're at it, below is somewhat ugly sparse patch for detecting
"&& 0x" typos.
diff --git a/evaluate.c b/evaluate.c
index c0e9d17..186756b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -877,6 +877,13 @@ static struct symbol *evaluate_logical(struct expression *expr)
if (!evaluate_conditional(expr->right, 0))
return NULL;
+ if (expr->op == SPECIAL_LOGICAL_AND) {
+ if (expr->left->type == EXPR_VALUE && expr->left->orig_in_hex)
+ warning(expr->pos, "dubious && 0x");
+ if (expr->right->type == EXPR_VALUE && expr->right->orig_in_hex)
+ warning(expr->pos, "dubious && 0x");
+ }
+
expr->ctype = &bool_ctype;
if (expr->flags) {
if (!(expr->left->flags & expr->right->flags & Int_const_expr))
diff --git a/expression.c b/expression.c
index 289927a..9bcf586 100644
--- a/expression.c
+++ b/expression.c
@@ -361,6 +361,7 @@ got_it:
expr->flags = Int_const_expr;
expr->ctype = ctype_integer(modifiers);
expr->value = value;
+ expr->orig_in_hex = (str[0] == '0' && (str[1] == 'x' || str[1] == 'X'));
return;
Eoverflow:
error_die(expr->pos, "constant %s is too big even for unsigned long long",
@@ -70,6 +70,7 @@ struct expression {
struct {
unsigned long long value;
unsigned taint;
+ unsigned int orig_in_hex:1;
};
// EXPR_FVALUE
Here are wonders that it found:
drivers/media/video/se401.c:1283:13: warning: dubious && 0x
if (!cp[2] && SE401_FORMAT_BAYER) {
err("Bayer format not supported!");
return 1;
}
known issue, I sent a patch about it.
drivers/mmc/host/tifm_sd.c:183:9: warning: dubious && 0x
if ((r_data->flags & MMC_DATA_WRITE)
&& DATA_CARRY)
writel(host->bounce_buf_data[0],
host->dev->addr
+ SOCK_MMCSD_DATA);
given that DATA_CARRY is always used together with ->cmd_flags,
this place is asking for obvious fixlet:
[PATCH] tifm_sd.c: fix DATA_CARRY check
Signed-off-by: Alexey Dobriyan <[email protected]>
---
drivers/mmc/host/tifm_sd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/mmc/host/tifm_sd.c
+++ b/drivers/mmc/host/tifm_sd.c
@@ -180,7 +180,7 @@ static void tifm_sd_transfer_data(struct tifm_sd *host)
host->sg_pos++;
if (host->sg_pos == host->sg_len) {
if ((r_data->flags & MMC_DATA_WRITE)
- && DATA_CARRY)
+ && (host->cmd_flags & DATA_CARRY))
writel(host->bounce_buf_data[0],
host->dev->addr
+ SOCK_MMCSD_DATA);
drivers/net/wireless/arlan-main.c:439:3: warning: dubious && 0x
drivers/net/wireless/arlan-main.c:439:3: warning: dubious && 0x
drivers/net/wireless/arlan-main.c:439:3: warning: dubious && 0x
drivers/net/wireless/arlan-main.c:439:3: warning: dubious && 0x
#define setPowerOff(dev){\
writeControlRegister(dev,readControlRegister(dev) |
(ARLAN_POWER && ARLAN_ACCESS));
^^^
[PATCH] arlan: fix "x && y" typo
Signed-off-by: Alexey Dobriyan <[email protected]>
---
drivers/net/wireless/arlan.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/net/wireless/arlan.h
+++ b/drivers/net/wireless/arlan.h
@@ -485,7 +485,7 @@ struct arlan_private {
#define clearClearInterrupt(dev){\
writeControlRegister(dev,readControlRegister(dev) & ~ARLAN_CLEAR_INTERRUPT);}
#define setPowerOff(dev){\
- writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER && ARLAN_ACCESS));\
+ writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER & ARLAN_ACCESS));\
writeControlRegister(dev,readControlRegister(dev) & ~ARLAN_ACCESS);}
#define setPowerOn(dev){\
writeControlRegister(dev,readControlRegister(dev) & ~(ARLAN_POWER)); }
Alexey Dobriyan <[email protected]> writes:
> --- a/drivers/net/wireless/arlan.h
> +++ b/drivers/net/wireless/arlan.h
> @@ -485,7 +485,7 @@ struct arlan_private {
> #define clearClearInterrupt(dev){\
> writeControlRegister(dev,readControlRegister(dev) & ~ARLAN_CLEAR_INTERRUPT);}
> #define setPowerOff(dev){\
> - writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER && ARLAN_ACCESS));\
> + writeControlRegister(dev,readControlRegister(dev) | (ARLAN_POWER & ARLAN_ACCESS));\
Methinks that this actually wants (ARLAN_POWER | ARLAN_ACCESS), since
(ARLAN_POWER & ARLAN_ACCESS) == 0.
Andreas.
--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstra?e 5, 90409 N?rnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Alexey Dobriyan wrote:
> ["if (!x & y)" patch from yanzheng@]
> ["if (!x & y)" patch from adobriyan@]
> ["if (!x & y)" patches from viro@]
>
> While we're at it, below is somewhat ugly sparse patch for detecting
> "&& 0x" typos.
Excellent idea! I think it applies to || as well. I'll most likely
add a -Wboolean-logic-on-bit-constant to turn this warning on.
Any reason why this wouldn't apply to octal constants or to GCC's new
binary constants? I can trivially modify this patch to handle those
as well, just by dropping the check for an 'x' or 'X', and renaming the
flag.
As far as patch beauty goes, I think this patch looks just fine.
- Josh Triplett
> While we're at it, below is somewhat ugly sparse patch for detecting
> "&& 0x" typos.
Excellent idea, and there is something to be said about a low-footprint patch
like that. However, if you really want to capture this kind of bugs, you would
need to have some kind "not a boolean" or "bitfield" attribute that
can propagate.
For example, you would want
if (foo && (BAR | BAZ)) ...;
with BAR and BAZ being hex constants to produce the same warning.
Incidentally, it is probably not just hex constants that deserve this treatment:
octal constants and variations of (1 << cst) are of the same nature. As well
as enums defined in such manners.
Morten
Morten Welinder wrote:
>> While we're at it, below is somewhat ugly sparse patch for detecting
>> "&& 0x" typos.
>
> Excellent idea, and there is something to be said about a low-footprint patch
> like that. However, if you really want to capture this kind of bugs, you would
> need to have some kind "not a boolean" or "bitfield" attribute that
> can propagate.
> For example, you would want
>
> if (foo && (BAR | BAZ)) ...;
>
> with BAR and BAZ being hex constants to produce the same warning.
>
> Incidentally, it is probably not just hex constants that deserve this treatment:
> octal constants and variations of (1 << cst) are of the same nature. As well
> as enums defined in such manners.
Sparse has a notion of "integer constant expression" already, which it
uses to validate expressions used for things like bitfield widths or
array sizes. I could easily have Sparse warn on any use of an integer
constant expression as an operand of || or &&. However, I can imagine
that that might lead to some false positives when intentionally using
an integer constant expression in a condition and expecting the
compiler to optimize it out at compile time.
- Josh Triplett
On Wed, 10 Oct 2007 14:45:40 +0400
Alexey Dobriyan <[email protected]> wrote:
> ["if (!x & y)" patch from yanzheng@]
> ["if (!x & y)" patch from adobriyan@]
> ["if (!x & y)" patches from viro@]
>
> While we're at it, below is somewhat ugly sparse patch for detecting
> "&& 0x" typos.
>
The maintainer for tifm is Alex Dubov, so cc:ing him.
> drivers/mmc/host/tifm_sd.c:183:9: warning: dubious && 0x
>
> if ((r_data->flags & MMC_DATA_WRITE)
> && DATA_CARRY)
> writel(host->bounce_buf_data[0],
> host->dev->addr
> + SOCK_MMCSD_DATA);
>
> given that DATA_CARRY is always used together with
> ->cmd_flags, this place is asking for obvious fixlet:
>
>
> [PATCH] tifm_sd.c: fix DATA_CARRY check
>
> Signed-off-by: Alexey Dobriyan <[email protected]>
> ---
>
> drivers/mmc/host/tifm_sd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/mmc/host/tifm_sd.c
> +++ b/drivers/mmc/host/tifm_sd.c
> @@ -180,7 +180,7 @@ static void tifm_sd_transfer_data(struct tifm_sd
> *host) host->sg_pos++;
> if (host->sg_pos == host->sg_len) {
> if ((r_data->flags & MMC_DATA_WRITE)
> - && DATA_CARRY)
> + && (host->cmd_flags &
> DATA_CARRY)) writel(host->bounce_buf_data[0],
> host->dev->addr
> + SOCK_MMCSD_DATA);
>
>
>
Rgds
Pierre
On Wed, 2007-10-10 at 11:08 -0700, Josh Triplett wrote:
> Sparse has a notion of "integer constant expression" already, which it
> uses to validate expressions used for things like bitfield widths or
> array sizes. I could easily have Sparse warn on any use of an integer
> constant expression as an operand of || or &&. However, I can imagine
> that that might lead to some false positives when intentionally using
> an integer constant expression in a condition and expecting the
> compiler to optimize it out at compile time.
I can imagine 0 or 1 being used, maybe -1, but hardly anything else.
Maybe you could add the code printing the value and then get some
statics on the actual Linux kernel to see which values are common and
which are not?
--
Regards,
Pavel Roskin
On Wed, Oct 10, 2007 at 04:45:46AM -0700, Josh Triplett wrote:
> Alexey Dobriyan wrote:
> > ["if (!x & y)" patch from yanzheng@]
> > ["if (!x & y)" patch from adobriyan@]
> > ["if (!x & y)" patches from viro@]
> >
> > While we're at it, below is somewhat ugly sparse patch for detecting
> > "&& 0x" typos.
>
> Excellent idea! I think it applies to || as well.
Sadly, yes.
[PATCH] smctr: fix "|| 0x" typo
IBM_PASS_SOURCE_ADDR is 1, so logically ORing it with status bits is
pretty useless. Do bitwise OR, instead.
Signed-off-by: Alexey Dobriyan <[email protected]>
---
drivers/net/tokenring/smctr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/net/tokenring/smctr.c
+++ b/drivers/net/tokenring/smctr.c
@@ -3413,7 +3413,7 @@ static int smctr_make_tx_status_code(struct net_device *dev,
tsv->svi = TRANSMIT_STATUS_CODE;
tsv->svl = S_TRANSMIT_STATUS_CODE;
- tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) || IBM_PASS_SOURCE_ADDR);
+ tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) | IBM_PASS_SOURCE_ADDR);
/* Stripped frame status of Transmitted Frame */
tsv->svv[1] = tx_fstatus & 0xff;
> I'll most likely
> add a -Wboolean-logic-on-bit-constant to turn this warning on.
Ewww, more options. :-(
> Any reason why this wouldn't apply to octal constants or to GCC's new
> binary constants? I can trivially modify this patch to handle those
> as well, just by dropping the check for an 'x' or 'X', and renaming the
> flag.
OK, let me try too.
On Oct 11, 2007, at 03:35:37, Alexey Dobriyan wrote:
> Sadly, yes.
>
> [PATCH] smctr: fix "|| 0x" typo
>
> IBM_PASS_SOURCE_ADDR is 1, so logically ORing it with status bits is
> pretty useless. Do bitwise OR, instead.
>
> Signed-off-by: Alexey Dobriyan <[email protected]>
> ---
>
> drivers/net/tokenring/smctr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/net/tokenring/smctr.c
> +++ b/drivers/net/tokenring/smctr.c
> @@ -3413,7 +3413,7 @@ static int smctr_make_tx_status_code(struct
> net_device *dev,
> tsv->svi = TRANSMIT_STATUS_CODE;
> tsv->svl = S_TRANSMIT_STATUS_CODE;
>
> - tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) ||
> IBM_PASS_SOURCE_ADDR);
> + tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) |
> IBM_PASS_SOURCE_ADDR);
>
> /* Stripped frame status of Transmitted Frame */
> tsv->svv[1] = tx_fstatus & 0xff;
Hmm, here's a question for you: The old code was equivalent to "tsv-
>svv[0] = 1;", what's your proof that we don't rely on this "bug"
elsewhere in the code? In other words, this is a significant
behavior change (albeit fixing an apparent bug) from what we've done
for a while. You might want to do a git-blame on this bit of code to
see who the last person to modify it was and ask them to test or
confirm the patch first. The same general questions apply to the
other logical-op bugs.
Cheers,
Kyle Moffett
--- Pierre Ossman <[email protected]> wrote:
> On Wed, 10 Oct 2007 14:45:40 +0400
> Alexey Dobriyan <[email protected]> wrote:
>
> > ["if (!x & y)" patch from yanzheng@]
> > ["if (!x & y)" patch from adobriyan@]
> > ["if (!x & y)" patches from viro@]
> >
> > While we're at it, below is somewhat ugly sparse patch for detecting
> > "&& 0x" typos.
> >
>
> The maintainer for tifm is Alex Dubov, so cc:ing him.
>
> > drivers/mmc/host/tifm_sd.c:183:9: warning: dubious && 0x
> >
> > if ((r_data->flags & MMC_DATA_WRITE)
> > && DATA_CARRY)
> > writel(host->bounce_buf_data[0],
> > host->dev->addr
> > + SOCK_MMCSD_DATA);
> >
> > given that DATA_CARRY is always used together with
> > ->cmd_flags, this place is asking for obvious fixlet:
> >
> >
> > [PATCH] tifm_sd.c: fix DATA_CARRY check
> >
> > Signed-off-by: Alexey Dobriyan <[email protected]>
> > ---
> >
> > drivers/mmc/host/tifm_sd.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- a/drivers/mmc/host/tifm_sd.c
> > +++ b/drivers/mmc/host/tifm_sd.c
> > @@ -180,7 +180,7 @@ static void tifm_sd_transfer_data(struct tifm_sd
> > *host) host->sg_pos++;
> > if (host->sg_pos == host->sg_len) {
> > if ((r_data->flags & MMC_DATA_WRITE)
> > - && DATA_CARRY)
> > + && (host->cmd_flags &
> > DATA_CARRY)) writel(host->bounce_buf_data[0],
> > host->dev->addr
> > + SOCK_MMCSD_DATA);
> >
> >
> >
>
> Rgds
> Pierre
>
Oops. I wonder why this was never triggered (some users are having problems with dma, so they use
PIO rather extensively). The patch is probably correct, but I can't do any testing because I'm
currently on vacation.
____________________________________________________________________________________
Shape Yahoo! in your own image. Join our Network Research Panel today! http://surveylink.yahoo.com/gmrs/yahoo_panel_invite.asp?a=7
Alexey Dobriyan wrote:
> On Wed, Oct 10, 2007 at 04:45:46AM -0700, Josh Triplett wrote:
>> Alexey Dobriyan wrote:
>>> ["if (!x & y)" patch from yanzheng@]
>>> ["if (!x & y)" patch from adobriyan@]
>>> ["if (!x & y)" patches from viro@]
>>>
>>> While we're at it, below is somewhat ugly sparse patch for detecting
>>> "&& 0x" typos.
>> Excellent idea! I think it applies to || as well.
>
> Sadly, yes.
>
> [PATCH] smctr: fix "|| 0x" typo
>
> IBM_PASS_SOURCE_ADDR is 1, so logically ORing it with status bits is
> pretty useless. Do bitwise OR, instead.
>
> Signed-off-by: Alexey Dobriyan <[email protected]>
> ---
>
> drivers/net/tokenring/smctr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/net/tokenring/smctr.c
> +++ b/drivers/net/tokenring/smctr.c
> @@ -3413,7 +3413,7 @@ static int smctr_make_tx_status_code(struct net_device *dev,
> tsv->svi = TRANSMIT_STATUS_CODE;
> tsv->svl = S_TRANSMIT_STATUS_CODE;
>
> - tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) || IBM_PASS_SOURCE_ADDR);
> + tsv->svv[0] = ((tx_fstatus & 0x0100 >> 6) | IBM_PASS_SOURCE_ADDR);
>
> /* Stripped frame status of Transmitted Frame */
> tsv->svv[1] = tx_fstatus & 0xff;
Nice catch. I agree with Kyle, though, that fixing it may require
some care to make sure it doesn't break things relying on the bug.
>> I'll most likely
>> add a -Wboolean-logic-on-bit-constant to turn this warning on.
>
> Ewww, more options. :-(
With heuristic warnings like this, people need a way to turn them off.
>> Any reason why this wouldn't apply to octal constants or to GCC's new
>> binary constants? I can trivially modify this patch to handle those
>> as well, just by dropping the check for an 'x' or 'X', and renaming the
>> flag.
>
> OK, let me try too.
Go for it. I look forward to your next patch.
Thanks,
Josh Triplett