["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-&gt...
[ message continues ]

On Wed, 10 Oct 2007 14:45:40 +0400
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 
-

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

-

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

-

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
-

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
-

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 */
OK, let me try too.
-

Nice catch.  I agree with Kyle, though, that fixing it may require
Go for it.  I look forward to your next patch.
Thanks,

Josh Triplett

-

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
-

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

-