On 5/30/07, Satyam Sharma <[email protected]> wrote:
On 5/30/07, Roland Dreier <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
