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@xxxxxxxxx> wrote:
> On 5/30/07, Roland Dreier <rdreier@xxxxxxxxx> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/