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

PATCH Alexey Dobriyan 8 participants 11 messages Oct 10, 2007 – Oct 11, 2007

Subsystems (5)

Network drivers MMC, SD and SDIO Networking drivers (wireless) Sparse checker +1 more subsystems

lore

Share & tools

lore

Alexey Dobriyan

author

2007-10-10 10:46 UTC #0 lore raw

["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.

evaluate.c +7 -0

index c0e9d17..186756b 100644
@@ -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))

expression.c +2 -0

index 289927a..9bcf586 100644
@@ -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(-)

drivers/mmc/host/tifm_sd.c +1 -1

@@ -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(-)

drivers/net/wireless/arlan.h +1 -1

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

Andreas Schwab

2007-10-10 11:23 UTC #1 lore raw

drivers/net/wireless/arlan.h struct arlan_private { · line 488

Quoted 8 lines · Alexey Dobriyan

drivers/net/wireless/arlan.h +1 -1

@@ -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.

Signature · 4 lines

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."

Josh Triplett

2007-10-10 11:45 UTC #2 lore raw

In reply to Alexey Dobriyan

Quoted 6 lines · 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.

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

Morten Welinder

2007-10-10 13:35 UTC #6 lore raw

In reply to Alexey Dobriyan

Quoted 2 lines

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

Josh Triplett

2007-10-10 18:09 UTC #7 lore raw

Quoted 9 lines · Morten Welinder

Quoted 2 lines
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

Quoted 7 lines

    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

Pierre Ossman

2007-10-10 18:22 UTC #9 lore raw

In reply to Alexey Dobriyan

Quoted 7 lines · 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.

Quoted 26 lines

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(-)

drivers/mmc/host/tifm_sd.c static void tifm_sd_transfer_data(struct tifm_sd · line 186

Quoted 8 lines

drivers/mmc/host/tifm_sd.c +1 -1

@@ -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

Pavel Roskin

2007-10-10 19:02 UTC #8 lore raw

In reply to Josh Triplett

Quoted 7 lines · Josh Triplett

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?

Signature · 2 lines

Regards,
Pavel Roskin

Alexey Dobriyan

author

2007-10-11 07:37 UTC #3 lore raw

In reply to Josh Triplett

Quoted 9 lines · Josh Triplett

Quoted 6 lines · 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.
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(-)

drivers/net/tokenring/smctr.c +1 -1

@@ -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;

Quoted 2 lines

I'll most likely
add a -Wboolean-logic-on-bit-constant to turn this warning on.

Ewww, more options. :-(

Quoted 4 lines

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.

Kyle Moffett

2007-10-11 09:00 UTC #4 lore raw

Quoted 25 lines · Alexey Dobriyan

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(-)
drivers/net/tokenring/smctr.c +1 -1
@@ -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);

Quoted 2 lines

         /* 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

Alex Dubov

maintainer

2007-10-11 14:04 UTC #10 lore raw

In reply to Pierre Ossman

Quoted 42 lines · --- Pierre Ossman

Quoted 7 lines · 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.
Quoted 29 lines
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(-)
drivers/mmc/host/tifm_sd.c +1 -1
@@ -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],

Quoted 9 lines

drivers/mmc/host/tifm_sd.c static void tifm_sd_transfer_data(struct tifm_sd *host) · line 186
Quoted 5 lines
drivers/mmc/host/tifm_sd.c +0 -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

Josh Triplett

2007-10-11 16:27 UTC #5 lore raw

In reply to Alexey Dobriyan

Quoted 26 lines · Alexey Dobriyan

Quoted 8 lines · Josh Triplett
Quoted 6 lines · 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.
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(-)

drivers/net/tokenring/smctr.c static int smctr_make_tx_status_code(struct net_device *dev, · line 3419

Quoted 8 lines

drivers/net/tokenring/smctr.c +1 -1

@@ -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.

Quoted 4 lines

Quoted 2 lines
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.

Quoted 6 lines

Quoted 4 lines
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