[PATCH] drivers/infiniband: fix comparsion between unsigned and negative

[PATCH] drivers/infiniband: fix comparsion between unsigned and negative

[Bill Nottingham]

Recent gcc versions emit warnings when unsigned variables are compared < 0 or >= 0.

Signed-off-by: Bill Nottingham <notting@redhat.com>

---
 core/sysfs.c       |    2 +-
 core/ucm.c         |    2 +-
 core/ucma.c        |    2 +-
 core/user_mad.c    |    5 ++---
 core/uverbs_main.c |    3 +--
 core/verbs.c       |    3 +--
 hw/mlx4/qp.c       |    2 +-
 7 files changed, 8 insertions(+), 11 deletions(-)

diff -ru linux-2.6.21-old/drivers/infiniband/core/sysfs.c linux-2.6.21/drivers/infiniband/core/sysfs.c
--- linux-2.6.21-old/drivers/infiniband/core/sysfs.c	2007-05-30 02:52:52.000000000 -0400
+++ linux-2.6.21/drivers/infiniband/core/sysfs.c	2007-05-30 02:07:31.000000000 -0400
@@ -112,7 +112,7 @@
 		return ret;
 
 	return sprintf(buf, "%d: %s\n", attr.state,
-		       attr.state >= 0 && attr.state < ARRAY_SIZE(state_name) ?
+		       attr.state < ARRAY_SIZE(state_name) ?
 		       state_name[attr.state] : "UNKNOWN");
 }
 
diff -ru linux-2.6.21-old/drivers/infiniband/core/ucma.c linux-2.6.21/drivers/infiniband/core/ucma.c
--- linux-2.6.21-old/drivers/infiniband/core/ucma.c	2007-05-30 02:52:52.000000000 -0400
+++ linux-2.6.21/drivers/infiniband/core/ucma.c	2007-05-30 02:09:34.000000000 -0400
@@ -955,7 +955,7 @@
 	if (copy_from_user(&hdr, buf, sizeof(hdr)))
 		return -EFAULT;
 
-	if (hdr.cmd < 0 || hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
+	if (hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
 		return -EINVAL;
 
 	if (hdr.in + sizeof(hdr) > len)
diff -ru linux-2.6.21-old/drivers/infiniband/core/ucm.c linux-2.6.21/drivers/infiniband/core/ucm.c
--- linux-2.6.21-old/drivers/infiniband/core/ucm.c	2007-05-30 02:52:52.000000000 -0400
+++ linux-2.6.21/drivers/infiniband/core/ucm.c	2007-05-30 02:08:01.000000000 -0400
@@ -1125,7 +1125,7 @@
 	if (copy_from_user(&hdr, buf, sizeof(hdr)))
 		return -EFAULT;
 
-	if (hdr.cmd < 0 || hdr.cmd >= ARRAY_SIZE(ucm_cmd_table))
+	if (hdr.cmd >= ARRAY_SIZE(ucm_cmd_table))
 		return -EINVAL;
 
 	if (hdr.in + sizeof(hdr) > len)
diff -ru linux-2.6.21-old/drivers/infiniband/core/user_mad.c linux-2.6.21/drivers/infiniband/core/user_mad.c
--- linux-2.6.21-old/drivers/infiniband/core/user_mad.c	2007-05-30 02:52:52.000000000 -0400
+++ linux-2.6.21/drivers/infiniband/core/user_mad.c	2007-05-30 02:08:32.000000000 -0400
@@ -455,8 +455,7 @@
 		goto err;
 	}
 
-	if (packet->mad.hdr.id < 0 ||
-	    packet->mad.hdr.id >= IB_UMAD_MAX_AGENTS) {
+	if (packet->mad.hdr.id >= IB_UMAD_MAX_AGENTS) {
 		ret = -EINVAL;
 		goto err;
 	}
@@ -665,7 +664,7 @@
 
 	down_write(&file->port->mutex);
 
-	if (id < 0 || id >= IB_UMAD_MAX_AGENTS || !__get_agent(file, id)) {
+	if (id >= IB_UMAD_MAX_AGENTS || !__get_agent(file, id)) {
 		ret = -EINVAL;
 		goto out;
 	}
diff -ru linux-2.6.21-old/drivers/infiniband/core/uverbs_main.c linux-2.6.21/drivers/infiniband/core/uverbs_main.c
--- linux-2.6.21-old/drivers/infiniband/core/uverbs_main.c	2007-05-30 02:52:52.000000000 -0400
+++ linux-2.6.21/drivers/infiniband/core/uverbs_main.c	2007-05-30 02:09:07.000000000 -0400
@@ -592,8 +592,7 @@
 	if (hdr.in_words * 4 != count)
 		return -EINVAL;
 
-	if (hdr.command < 0				||
-	    hdr.command >= ARRAY_SIZE(uverbs_cmd_table) ||
+	if (hdr.command >= ARRAY_SIZE(uverbs_cmd_table) ||
 	    !uverbs_cmd_table[hdr.command]		||
 	    !(file->device->ib_dev->uverbs_cmd_mask & (1ull << hdr.command)))
 		return -EINVAL;
diff -ru linux-2.6.21-old/drivers/infiniband/core/verbs.c linux-2.6.21/drivers/infiniband/core/verbs.c
--- linux-2.6.21-old/drivers/infiniband/core/verbs.c	2007-05-30 02:52:52.000000000 -0400
+++ linux-2.6.21/drivers/infiniband/core/verbs.c	2007-05-30 02:07:06.000000000 -0400
@@ -535,8 +535,7 @@
 {
 	enum ib_qp_attr_mask req_param, opt_param;
 
-	if (cur_state  < 0 || cur_state  > IB_QPS_ERR ||
-	    next_state < 0 || next_state > IB_QPS_ERR)
+	if (cur_state  > IB_QPS_ERR || next_state > IB_QPS_ERR)
 		return 0;
 
 	if (mask & IB_QP_CUR_STATE  &&
diff -ru linux-2.6.21-old/drivers/infiniband/hw/mlx4/qp.c linux-2.6.21/drivers/infiniband/hw/mlx4/qp.c
--- linux-2.6.21-old/drivers/infiniband/hw/mlx4/qp.c	2007-05-30 02:52:52.000000000 -0400
+++ linux-2.6.21/drivers/infiniband/hw/mlx4/qp.c	2007-05-30 02:10:18.000000000 -0400
@@ -1284,7 +1284,7 @@
 		 */
 		wmb();
 
-		if (wr->opcode < 0 || wr->opcode >= ARRAY_SIZE(mlx4_ib_opcode)) {
+		if (wr->opcode >= ARRAY_SIZE(mlx4_ib_opcode)) {
 			err = -EINVAL;
 			goto out;
 		}

dealing with gcc 'comparison is always false' warnings (was: [PATCH] drivers/infiniband: fix comparsion between unsigned and negative)

[Roland Dreier]

thanks... I'm wondering if there's a consensus among kernel hackers
about changes like:

 > -	if (hdr.cmd < 0 || hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
 > +	if (hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
 >  		return -EINVAL;

I understand that new gcc sees that hdr.cmd is unsigned and hence
can't be < 0, and generates a warning for that, and having a build
cluttered with warnings hides bugs and so on.  However the code here
looks quite sensible to me -- otherwise we end up with missing range
checking if hdr.cmd ever changes to a signed type.  This seems like a
good way to introduce bugs: delete valid range checking code to shut
up a silly gcc warning, and then change the type of a variable.

Can't we just make gcc shut up about the comparison and generate no
code for it because it knows it can't be true?

 - R.

Re: dealing with gcc 'comparison is always false' warnings (was: [PATCH] drivers/infiniband: fix comparsion between unsigned and negative)

[Satyam Sharma]

On 5/30/07, Roland Dreier <rdreier@cisco.com> wrote:
> thanks... I'm wondering if there's a consensus among kernel hackers
> about changes like:
>
>  > -    if (hdr.cmd < 0 || hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
>  > +    if (hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
>  >              return -EINVAL;
>
> I understand that new gcc sees that hdr.cmd is unsigned and hence
> can't be < 0, and generates a warning for that, and having a build
> cluttered with warnings hides bugs and so on.  However the code here
> looks quite sensible to me -- otherwise we end up with missing range
> checking if hdr.cmd ever changes to a signed type.  This seems like a
> good way to introduce bugs: delete valid range checking code to shut
> up a silly gcc warning, and then change the type of a variable.

You're *absolutely* correct about the issue that these "fixes" that remove
such conditions end up remove range-checking making the code more
flakey / less readable.

However, gcc is _just as correct_. It is only crying about seeing a condition
that the programmer could have written with some purpose in mind but which
is being completely compiled away by it when generating the code because
of it being a tautology / contradiction ...

> Can't we just make gcc shut up about the comparison and generate no
> code for it because it knows it can't be true?

No, shutting gcc up wouldn't be the right thing, IMHO. These warnings are
a good reminder to the programmer to go and see if there is a real bug
somewhere and if something really needs to be done with the code (could
be simply to change the type of a variable to signed that was mistakenly
declared unsigned, f.e.).

But yes, the kind of "fixes" you pointed out that _remove_ these conditions
are definitely *not* what we would want to do.

Satyam

Re: [ofa-general] Re: dealing with gcc 'comparison is always false' warnings

[Roland Dreier]

 > However, gcc is _just as correct_. It is only crying about seeing a condition
 > that the programmer could have written with some purpose in mind but which
 > is being completely compiled away by it when generating the code because
 > of it being a tautology / contradiction ...

Well, OK, but there's lots of things gcc could warn about.  How about

	while (1) { ...

By your argument gcc should warn that '1' always evaluates to true.
Or how about

#if 0

why shouldn't the preprocessor warn that the conditional is always false?

 > No, shutting gcc up wouldn't be the right thing, IMHO. These warnings are
 > a good reminder to the programmer to go and see if there is a real bug
 > somewhere and if something really needs to be done with the code (could
 > be simply to change the type of a variable to signed that was mistakenly
 > declared unsigned, f.e.).

OK, but suppose I looked at it and there's no bug.  Leaving the
warning has a cost too: it hides useful warnings (that might be
showing real bugs) in all the clutter.

 - R.

Re: [ofa-general] Re: dealing with gcc 'comparison is always false' warnings

[Satyam Sharma]

[ Sorry, the threading broke because the subject changed,
so I missed seeing this mail earlier. ]

On 5/30/07, Roland Dreier <rdreier@cisco.com> wrote:
>  > However, gcc is _just as correct_. It is only crying about seeing a condition
>  > that the programmer could have written with some purpose in mind but which
>  > is being completely compiled away by it when generating the code because
>  > of it being a tautology / contradiction ...
>
> Well, OK, but there's lots of things gcc could warn about.  How about
>
>         while (1) { ...

Umm ... perhaps because gcc does not compile away any code for
such cases, but only the condition? Or because gcc knows this is
a common idiom in a *lot* of C code? I don't know (or care!) ... the
precise cases for which the warning is emitted would be known only
by reading gcc sources (which I have no intention of doing :-)

> By your argument gcc should warn that '1' always evaluates to true.

Note that my "argument" was about conditions that weren't as
simplistic as #if 0 or while (1) and that involved not merely 1 or 0,
but variables whose values might not be available at compile-time ...

> Or how about
>
> #if 0
>
> why shouldn't the preprocessor warn that the conditional is always false?

Perhaps because gcc knows programmers often use this common
method to disable some code?

I can't answer all these questions, of course (better ask the gcc folks),
but I don't care either. Clearly, none of the above are any reason why
gcc should *not* complain when it sees a _seemingly_ meaningful
condition conceivably written by the programmer with something in
mind but being completely optimized away by it.

[ BTW, perhaps the reason why the gcc folks did *not* put a warning
for while (1) or #if 0 is also because they know that programmers often
write such conditions with something meaningful in mind. ]

>  > No, shutting gcc up wouldn't be the right thing, IMHO. These warnings are
>  > a good reminder to the programmer to go and see if there is a real bug
>  > somewhere and if something really needs to be done with the code (could
>  > be simply to change the type of a variable to signed that was mistakenly
>  > declared unsigned, f.e.).
>
> OK, but suppose I looked at it and there's no bug.  Leaving the
> warning has a cost too: it hides useful warnings (that might be
> showing real bugs) in all the clutter.

Agreed, this warning emits a lot of false positives. But this warning isn't
enabled with -Wall either, or is it (now)? I remember the only way to
enable this was with -Wextra, and last I heard the top-level Makefile
did not specify that ... (?)

Satyam

Re: dealing with gcc 'comparison is always false' warnings (was: [PATCH] drivers/infiniband: fix comparsion between unsigned and negative)

[Satyam Sharma]

On 5/30/07, Satyam Sharma <satyam.sharma@gmail.com> wrote:
> On 5/30/07, Roland Dreier <rdreier@cisco.com> wrote:
> > thanks... I'm wondering if there's a consensus among kernel hackers
> > about changes like:
> >
> >  > -    if (hdr.cmd < 0 || hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
> >  > +    if (hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
> >  >              return -EINVAL;
> >
> > I understand that new gcc sees that hdr.cmd is unsigned and hence
> > can't be < 0, and generates a warning for that, and having a build
> > cluttered with warnings hides bugs and so on.  However the code here
> > looks quite sensible to me -- otherwise we end up with missing range
> > checking if hdr.cmd ever changes to a signed type.  This seems like a
> > good way to introduce bugs: delete valid range checking code to shut
> > up a silly gcc warning, and then change the type of a variable.
>
> You're *absolutely* correct about the issue that these "fixes" that remove
> such conditions end up remove range-checking making the code more
> flakey / less readable.
>
> However, gcc is _just as correct_. It is only crying about seeing a condition
> that the programmer could have written with some purpose in mind but which
> is being completely compiled away by it when generating the code because
> of it being a tautology / contradiction ...
>
> > Can't we just make gcc shut up about the comparison and generate no
> > code for it because it knows it can't be true?

[ BTW gcc does not generate code for such cases already; either for the
condition whose truth value is already known, or for the codepath that
will never be executed as a result. ]

> No, shutting gcc up wouldn't be the right thing, IMHO. These warnings are
> a good reminder to the programmer to go and see if there is a real bug
> somewhere and if something really needs to be done with the code (could
> be simply to change the type of a variable to signed that was mistakenly
> declared unsigned, f.e.).

A common scenario I could imagine for the above would be where a typo
makes someone declare a var as size_t when it should've been ssize_t.
This is clearly a real bug that would get caught with this gcc warning
(but not with -Wall).

> But yes, the kind of "fixes" you pointed out that _remove_ these conditions
> are definitely *not* what we would want to do.

Erm, to qualify my rather strong opinion above: there could perhaps be
exceptions where the condition being removed could be truly redundant,
of course :-)

Satyam

Re: dealing with gcc 'comparison is always false' warnings

[Tilman Schmidt]

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

Am 30.05.2007 17:41 schrieb Satyam Sharma:
> On 5/30/07, Roland Dreier <rdreier@cisco.com> wrote:
>> thanks... I'm wondering if there's a consensus among kernel hackers
>> about changes like:
>>
>>  > -    if (hdr.cmd < 0 || hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
>>  > +    if (hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
>>  >              return -EINVAL;
>>
>> I understand that new gcc sees that hdr.cmd is unsigned and hence
>> can't be < 0, and generates a warning for that, and having a build
>> cluttered with warnings hides bugs and so on.  However the code here
>> looks quite sensible to me -- otherwise we end up with missing range
>> checking if hdr.cmd ever changes to a signed type.  This seems like a
>> good way to introduce bugs: delete valid range checking code to shut
>> up a silly gcc warning, and then change the type of a variable.
> 
> You're *absolutely* correct about the issue that these "fixes" that remove
> such conditions end up remove range-checking making the code more
> flakey / less readable.

I disagree. Changing the type of a variable is a significant
modification. If someone does that, he or she *must* check every
use of that variable, at which point he or she will also modify
any range checks accordingly. Having checks that don't fit with
the previous type *distracts* from that job. "Oh, did I modify
that part already? Guess I can skip checking the rest of that
function then." Oops.

Nor is readability a suitable argument. Checking if hdr.cmd is
less than zero gives the misleading impression that it *could*
be less than zero, thus *impairing* readability.

jm2c
T.

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: dealing with gcc 'comparison is always false' warnings

[Satyam Sharma]

Hi,

On 5/31/07, Tilman Schmidt <tilman@imap.cc> wrote:
> Am 30.05.2007 17:41 schrieb Satyam Sharma:
> > On 5/30/07, Roland Dreier <rdreier@cisco.com> wrote:
> >> thanks... I'm wondering if there's a consensus among kernel hackers
> >> about changes like:
> >>
> >>  > -    if (hdr.cmd < 0 || hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
> >>  > +    if (hdr.cmd >= ARRAY_SIZE(ucma_cmd_table))
> >>  >              return -EINVAL;
> >>
> >> I understand that new gcc sees that hdr.cmd is unsigned and hence
> >> can't be < 0, and generates a warning for that, and having a build
> >> cluttered with warnings hides bugs and so on.  However the code here
> >> looks quite sensible to me -- otherwise we end up with missing range
> >> checking if hdr.cmd ever changes to a signed type.  This seems like a
> >> good way to introduce bugs: delete valid range checking code to shut
> >> up a silly gcc warning, and then change the type of a variable.
> >
> > You're *absolutely* correct about the issue that these "fixes" that remove
> > such conditions end up remove range-checking making the code more
> > flakey / less readable.
>
> I disagree. Changing the type of a variable is a significant
> modification. If someone does that, he or she *must* check every
> use of that variable, at which point he or she will also modify
> any range checks accordingly. Having checks that don't fit with
> the previous type *distracts* from that job. "Oh, did I modify
> that part already? Guess I can skip checking the rest of that
> function then." Oops.

I did not suggest the change-variable-type-from-unsigned-to-signed
thing as a "general" solution to such cases! ... in fact what I said
was that such cases do _not_ have a general solution at all, and
that shutting gcc up might not be a good idea, because a lot of
times such warnings do un-hide bugs. [ BTW when I gave the
change-type-from-unsigned-to-signed example, I had the size_t vs
ssize_t typo/bug in mind, for which changing the type is the proper
fix; and note that similar bugs can occur for non-size_t cases too. ]

> Nor is readability a suitable argument. Checking if hdr.cmd is
> less than zero gives the misleading impression that it *could*
> be less than zero, thus *impairing* readability.

Hmmm, but I tend to agree with the sentiment expressed in:
http://lkml.org/lkml/2006/11/28/206

Satyam

Re: dealing with gcc 'comparison is always false' warnings (was: [PATCH] drivers/infiniband: fix comparsion between unsigned and negative)

[Bill Nottingham]

Satyam Sharma (satyam.sharma@gmail.com) said: 
> But yes, the kind of "fixes" you pointed out that _remove_ these conditions
> are definitely *not* what we would want to do.

I can see that - but I think it should be at least be brought up for each
warning, to determine either:

1) if it should be ignored
2) if a signed type is actually intended
3) if the code should be elided

While not necessarily in the IB instances, there are cases where there
are entire blocks of code (with debugging output, error returns, etc)
that can never get run, and it may make sense to remove those.

Bill

Re: dealing with gcc 'comparison is always false' warnings (was: [PATCH] drivers/infiniband: fix comparsion between unsigned and negative)

[Satyam Sharma]

Hi Bill,

On 5/31/07, Bill Nottingham <notting@redhat.com> wrote:
> Satyam Sharma (satyam.sharma@gmail.com) said:
> > But yes, the kind of "fixes" you pointed out that _remove_ these conditions
> > are definitely *not* what we would want to do.
>
> I can see that - but I think it should be at least be brought up for each
> warning, to determine either:
>
> 1) if it should be ignored
> 2) if a signed type is actually intended
> 3) if the code should be elided

Agreed. The extract you've pointed out above was too strongly worded
unnecessarily / wrong generalization, and I corrected it later.

> While not necessarily in the IB instances, there are cases where there
> are entire blocks of code (with debugging output, error returns, etc)
> that can never get run, and it may make sense to remove those.

Agreed, again.

Satyam

Re: [ofa-general] [PATCH] drivers/infiniband: fix comparsion between unsigned and negative

[Roland Dreier]

I just went through this patch, and all the changes are of the form of
removing the < 0 test from code like

	if (x < 0 || x > MAX) 
		return -ERROR;

which Linus said we don't change in the email <http://lkml.org/lkml/2006/11/28/206>
that Satyam just pointed out.  So I'll drop this patch.

 - R.