Re: [PATCH] disable gcc warnings of sign/unsigned comparison

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Andi Kleen]

Paul Jackson <pj@sgi.com> writes:

> Right now, compiling a 2.6.0-mm1 (what I had handy) with the 3.3 gcc on

That was a bug in gcc 3.3.0. It had the -Wsign-compare warning 
enabled in -Wall by mistake. Update to gcc 3.3.1, which has this fixed.

-Andi

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Paul Jackson]

Andi wrote:
> That was a bug in gcc 3.3.0. It had the -Wsign-compare warning 
> enabled in -Wall by mistake.

Correct.  Even what is now downloadable as:

  ftp://ftp.gnu.org/gnu/gcc/gcc-3.3

seems to have this fixed, and has a gcc/cp/NEWS file entry stating:

     + -Wall no longer implies -W.  The new warning flag, -Wsign-compare,
        included in -Wall, warns about dangerous comparisons of signed and
        unsigned values. Only the flag is new; it was previously part of
        -W.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Adrian Bunk]

On Fri, Jan 02, 2004 at 02:33:10AM +0100, Andi Kleen wrote:
> Paul Jackson <pj@sgi.com> writes:
> 
> > Right now, compiling a 2.6.0-mm1 (what I had handy) with the 3.3 gcc on
> 
> That was a bug in gcc 3.3.0. It had the -Wsign-compare warning 
> enabled in -Wall by mistake. Update to gcc 3.3.1, which has this fixed.

Wrong.

It was _not_ a bug in gcc 3.3 .

It was a bug in some _prerelease_ versions of gcc 3.3 SuSE decided to 
ship in a release of their distribution.

There is no officially released version of gcc with this problem.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Paul Jackson]

> It was a bug in some _prerelease_ versions of gcc 3.3 SuSE decided to 
> ship in a release of their distribution.

That matches my observations, as I noted an earlier reply on lkml,
when I recommended against accepting my own patch, on the grounds
that we shouldn't pollute the Makefile with exceptions that only
applied to people running the gcc in a particular SuSE distro.

Good.  Thanks for the confirmation.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Paul Jackson]

> It was a bug in some _prerelease_ versions of gcc 3.3 SuSE decided to 
> ship in a release of their distribution.

That matches my observations, as I noted an earlier reply on lkml,
when I recommended against accepting my own patch, on the grounds
that we shouldn't pollute the Makefile with exceptions that only
applied to people running the gcc in a particular SuSE distro.

Good.  Thanks for the confirmation.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Adrian Bunk]

On Fri, Jan 02, 2004 at 02:33:10AM +0100, Andi Kleen wrote:
> Paul Jackson <pj@sgi.com> writes:
> 
> > Right now, compiling a 2.6.0-mm1 (what I had handy) with the 3.3 gcc on
> 
> That was a bug in gcc 3.3.0. It had the -Wsign-compare warning 
> enabled in -Wall by mistake. Update to gcc 3.3.1, which has this fixed.

Wrong.

It was _not_ a bug in gcc 3.3 .

It was a bug in some _prerelease_ versions of gcc 3.3 SuSE decided to 
ship in a release of their distribution.

There is no officially released version of gcc with this problem.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

[PATCH] disable gcc warnings of sign/unsigned comparison

[Paul Jackson]

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

in which Linus wrote:
> That's why I hate the "sign compare" warning of gcc so much - it warns 
> about things that you CANNOT sanely write in any other way. That makes 
> that particular warning _evil_, since it encourages people to write crap 
> code.

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	pj@sgi.com	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 <pj@sgi.com> 1.650.933.1373

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Trond Myklebust]

På to , 01/01/2004 klokka 07:33, skreiv Paul Jackson:
> 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.
> 

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

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Paul Jackson]

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

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).

> (see, for instance, the somewhat "unconventional" Linux min() and
> max() macros)

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 <pj@sgi.com> 1.650.933.1373

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Andrew Morton]

Paul Jackson <pj@sgi.com> wrote:
>
>  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

ugh, that is unacceptable.

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

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Tomas Szepe]

On Jan-01 2004, Thu, 15:33 -0800
Andrew Morton <akpm@osdl.org> wrote:

> Paul Jackson <pj@sgi.com> wrote:
> >
> >  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
> 
> 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 <szepe@pinerecords.com>

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Paul Jackson]

Andrew wrote:
> ugh, that is unacceptable.
> Unless anyone has a better idea, yes, we should apply your patch.

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 <pj@sgi.com> 1.650.933.1373

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Trond Myklebust]

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

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

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Tomas Szepe]

On Jan-01 2004, Thu, 19:46 -0500
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> P? to , 01/01/2004 klokka 18:15, skreiv Paul Jackson:
> 
> > 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 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).

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 <szepe@pinerecords.com>

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Paul Jackson]

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 <pj@sgi.com> 1.650.933.1373

Re: [PATCH] disable gcc warnings of sign/unsigned comparison

[Paul Jackson]

> You mentioned Richard Gooch's name cropping up in the broken code,

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 <pj@sgi.com> 1.650.933.1373