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?
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
-
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]
