idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)

idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)

[Alexey Dobriyan]

["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 <adobriyan@sw.ru>
---

 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 <adobriyan@sw.ru>
---

 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));   }

Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)

[Andreas Schwab]

Alexey Dobriyan <adobriyan@sw.ru> 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, schwab@suse.de
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."

Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)

[Josh Triplett]

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

Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)

[Morten Welinder]

> 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

Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)

[Josh Triplett]

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

Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

On Wed, 10 Oct 2007 14:45:40 +0400
Alexey Dobriyan <adobriyan@sw.ru> 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 <adobriyan@sw.ru>
> ---
> 
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)

[Pavel Roskin]

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

Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)

[Alexey Dobriyan]

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 <adobriyan@sw.ru>
---

 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.

Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)

[Kyle Moffett]

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 <adobriyan@sw.ru>
> ---
>
>  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

Re: idio{,ma}tic typos (was Re: + fix-vm_can_nonlinear-check-in-sys_remap_file_pages.patch added to -mm tree)

[Josh Triplett]

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 <adobriyan@sw.ru>
> ---
> 
>  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