disable gcc warnings of sign/unsigned comparison

Andrew,

Please consider applying the following patch.

This patch turns off all gcc warnings on comparing signed with unsigned numbers, by setting the gcc option -Wno-sign-compare in the top Makefile.

These warnings state:

warning: comparison between signed and unsigned

This patch is a "personal preference" decision. If you choose to reject it, I seek no justifications.

I like it, and at least with the version of gcc I happen to be using (3.3), find it really helps. This version of gcc dumps out many such complaints otherwise.

And one could make a case that Linus would like this patch, from his remark of a couple months ago, on a thread with the Subject of:

[PATCH] irda: fix type of struct irda_ias_set.attribute.irda_attrib_string.len

But what Linus actually thinks of this, I've no further clues.

The patch was computed against 2.6.0-mm2.

Thank-you for your consideration.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1536 -> 1.1537
# Makefile 1.441 -> 1.442
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/01/01 [email protected] 1.1537

# ignore gcc sign compare warnings
# --------------------------------------------
#
diff -Nru a/Makefile b/Makefile
--- a/Makefile	Thu Jan  1 04:13:04 2004
+++ b/Makefile	Thu Jan  1 04:13:04 2004
@@ -161,7 +161,7 @@

HOSTCC = gcc
HOSTCXX = g++
-HOSTCFLAGS = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer
+HOSTCFLAGS = -Wall -Wstrict-prototypes -Wno-sign-compare -O2 -fomit-frame-pointer
HOSTCXXFLAGS = -O2

# Decide whether to build built-in, modular, or both.
@@ -275,7 +275,7 @@
CPPFLAGS := -D__KERNEL__ -Iinclude \
$(if $(KBUILD_SRC),-Iinclude2 -I$(srctree)/include)

-CFLAGS := -Wall -Wstrict-prototypes -Wno-trigraphs \
+CFLAGS := -Wall -Wstrict-prototypes -Wno-sign-compare -Wno-trigraphs \
-fno-strict-aliasing -fno-common
AFLAGS := -D__ASSEMBLY__

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

På to , 01/01/2004 klokka 07:33, skreiv Paul Jackson:

Ignoring a potential bug is, of course one way of dealing with it. Most of us would prefer to deal with the bug itself, though.... A lot of effort has been put into coaxing gcc into detecting such comparison errors (see, for instance, the somewhat "unconventional" Linux min() and max() macros) precisely because signed comparisons have been a source of kernel bugs in the past...

How about therefore instead sending him a patch which fixes the actual code that is causing these warnings to be generated?

Cheers,
Trond

If such a patch generates more crap code (hence, sooner or later, bugs) than it fixes, that's the wrong choice.

Right now, compiling a 2.6.0-mm1 (what I had handy) with the 3.3 gcc on my Pentium system for arch i386 generates 1386 signed and unsigned warnings, of the two kinds:

warning: signed and unsigned type in conditional expression
warning: comparison between signed and unsigned

A patch that resolved these 1386 warnings would (1) generate more crap than it cleaned up, (2) immediately result in adding more bugs than removed, and (3) due to the crap level rising, generate more bugs long term than it avoided.

If Andrew accepted a patch from me that messed with all 1386 code locations (and that's just for arch = i386) generating these warnings, then I'd recommend that he be relieved of 2.6 kernel duties for incompetence. (Have no fear, Andrew - I would not make such a patch, I trust you would not accept it, and Linus would not listen to my recommendation).

There's more going on in min and max than just checking for signed versus unsigned comparison. Other type mismatches are seen as well.

==

Let me step back a minute. There is a useful lesson here.

The single most important technical key to the long term health of Linux is "good code" - can the competent kernel hacker read and understand and successfully modify the code.

This probably even outweighs the current 'bug' count, and certainly outweighs the count of warnings from the compiler.

If a big chunk of intrusive code looked like it had been written in Richard Gooch's idiosyncratic style, and then run through the Nvidia obfuscator, then even if it was 'perfect' (mathematically proven to have zero bugs and zero gcc warnings), we would not want it.

Normally I would agree with your sentiment - change the code to remove the warning, even if I have to do something a little bit ugly or silly. Many a time in my 25 years of serious coding, I have done just that.

But there's always a tradeoff here - the probability of a bug due to an ignored warning versus the probability of a bug due to the slight increase in human confusion due to the extra code crap.

If a warning is not seen that often, and has a half decent chance of catching a real bug when it does fire, and doesn't result in that much extra code crap when 'worked around', then on net, the warning is worth dealing with, even at the occassional expense of a little bit of silly code crap.

But if a warning is seen many times, very few of which catch bugs, and forces wide spread instances of unnatural coding acts to work around, then on net, it hurts more than it helps.

What we have here, with this warning, is one of the clearest examples of this later case that I've seen.

Linus is right - it's a bad warning.

==

There are only two possible resolutions that I can imagine myself endorsing here. Either we turn off the warning (-Wno-sign-compare, as in my patch) or more recent versions of gcc don't complain nearly as much, making it a mute point.

Have you knowledge that more recent versions of gcc don't complain nearly as much of this warning? If so, I will upgrade my gcc and shut up.

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

ugh, that is unacceptable.

Unless anyone has a better idea, yes, we should apply your patch.

Hmm, this doesn't happen for me with gcc 3.3.2. Paul, could you please send me your .config? (Off-list, preferably.)

Tomas Szepe <[email protected]>

It seems that the only place I can find the gcc with this bug (that -Wall implies -Wsign-compare) is the gcc 3.3 that came with my SuSE Linux 8.2 distribution. Each of the 3.3, 3.3.1 and 3.3.2 versions available at ftp.gnu.org/gnu/gcc are ok - no such bug.

So it's not a big deal either way whether to apply this patch.

In the short term, it helps a very specific version of gcc.

In the longer term, it clutters the top Makefile with a pimple to address a transient glitch.

Technically it is a no-op for other gcc versions, since sign-compare is off in all other cases anyway.

I vote that you:

==> Drop my patch in the trash

in the interests of avoiding Makefile clutter. Tell people like me that if it hurts to use this gcc 3.3, then don't use it ;).

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

På to , 01/01/2004 klokka 18:15, skreiv Paul Jackson:

I haven't checked with Andrew's '-mm' kernels, but AFAICS I'm not seeing any lingering signed/unsigned warnings in the stock 2.6.0 kernel - certainly not as many as 1386... (GCC version is latest 3.3.3 prerelease from Debian).

You mentioned Richard Gooch's name cropping up in the broken code, does that perhaps mean devfs? If so then the warnings may just reflect the fact that this code has been unmaintained for too long (devfs has after all been marked as "obsolete").

Cheers,
Trond

I'm not sure, but wasn't this kind of warning moved from under '-Wall' to under '-W'?

gcc 3.3.2's manpage says:

-W Print extra warning messages for these events:

...

o A comparison between signed and unsigned values
could produce an incorrect result when the signed value is converted to unsigned. (But don't warn if -Wno-sign-compare is also specified.)

Tomas Szepe <[email protected]>

My suspicion is that Tomas is right - and that something changed in gcc between 3.3 and 3.3.2, to affectively remove the sign compare warning from -Wall.

I will look into that more carefully now.

If so, then:

1) The alternative to my patch would be telling everyone to not use

     gcc 3.3 or 3.3.1.

2) On the other hand, my patch would still be desirable on the grounds that it makes gcc 3.3 and 3.3.1 usable for kernel builds, and is essentially a no-op otherwise, for kernel builds.

I'll post again after I compare some gcc code.

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

No, no, no. I was just using his name in a hypothetical example, as the most famous author of code we all love to hate.

The warnings were everywhere. See Andi Kleen's post stating that a bug in gcc 3.3.0 enabled that warning in -Wall by mistake.

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373