Re: [RFC][PATCH 00/10] Sparse: Git's "make check" target

Previous thread: [RFC][PATCH 01/10] Sparse: fix "non-ANSI function declaration" warnings by Ramsay Jones on Friday, June 8, 2007 - 3:09 pm. (1 message)

Next thread: [PATCH] git-p4import.py: handle paths with symlinks by Kevin Green on Friday, June 8, 2007 - 3:33 pm. (1 message)
From: Ramsay Jones
Date: Friday, June 8, 2007 - 3:06 pm

Hi Junio,

Since the Git Makefile has a "check" target that uses sparse, I decided to
take a look at the sparse project to see what it was about, and what it
has to say about the git source code.

Initially, I had many problems because I am using cygwin, but I was able to
fix most of those problems. (the output from "make check" was about 16k lines
at one point!). Git also tickled a bug in sparse 0.2, which resulted in some
120+ lines of bogus warnings; that was fixed in version 0.3 (commit 0.2-15-gef25961).
As a result, sparse version 0.3 + my patches, elicits 106 lines of output
from "make check".

Naturally, I decided to "fix" the warnings produced by sparse, which resulted
in the following patch series:

[PATCH 01/10] Sparse: fix "non-ANSI function declaration" warnings
[PATCH 02/10] Sparse: fix some "using integer as NULL pointer" warnings
[PATCH 03/10] Sparse: fix some "symbol not declared" warnings (Part 1)
[PATCH 04/10] Sparse: fix some "symbol not declared" warnings (Part 2)
[PATCH 05/10] Sparse: fix some "symbol not declared" warnings (Part 3)
[PATCH 06/10] Sparse: fix "'add_head' not declared" warning
[PATCH 07/10] Sparse: fix "'merge_file' not declared" warning
[PATCH 08/10] Sparse: fix an "incorrect type in argument n" warning
[PATCH 09/10] Sparse: fix some "symbol 's' shadows an earlier one" warnings
[PATCH 10/10] Sparse: fix a "symbol 'weak_match' shadows an earlier one" warning

However, this patch series does not completely remove all warnings; the output
is reduced to:

builtin-pack-objects.c:312:31: warning: Using plain integer as NULL pointer
csum-file.c:152:22: warning: Using plain integer as NULL pointer
exec_cmd.c:7:40: error: undefined identifier 'GIT_EXEC_PATH'
git.c:209:35: error: undefined identifier 'GIT_VERSION'
http.c:203:46: error: undefined identifier 'GIT_USER_AGENT'
index-pack.c:201:25: warning: Using plain integer as NULL pointer
index-pack.c:538:26: warning: Using plain integer as NULL pointer

The three "undefined identifier 'GIT_...'" ...
From: Josh Triplett
Date: Saturday, June 9, 2007 - 12:08 am

One note about using Sparse with Git: you almost certainly don't want to =
pass
-Wall to sparse, and current Git passes CFLAGS to Sparse which will do ex=
actly
that.  -Wall turns on all possible Sparse warnings, including nitpicky
warnings and warnings with a high false positive rate.  You should start =
from
the default set of Sparse warnings, and add additional warnings as desire=
d, or
turn off those you absolutely can't live with.  Current Sparse from Git (=
post
0.3, after commit e18c1014449adf42520daa9d3e53f78a3d98da34) has a change =
to
cgcc to filter out -Wall, so you can pass -Wall to GCC but not Sparse.  S=
ee
below for other reasons why you should use cgcc.

That said, this suggests that perhaps Sparse should treat -Wall different=
ly
for compatibility with GCC; specifically, perhaps Sparse should just igno=
re
-Wall, as its meaning with GCC (enable a reasonable default set of warnin=
gs)
already occurs by default in Sparse.  The current -Wall could become some=
thing
like -Weverything.  This would make Sparse somewhat less intuitive, but
" warning


er.]

And at one point prototypes didn't exist either. :)

Other valid null pointers exist, such as (void *)0.  You could also use (=
char
*)0 in this particular case.  Sparse complains because you just use the
integer 0.  I suggest just using NULL.

If you want to keep using Z_NULL rather than NULL, you could always undef=
 it
and define it as NULL after including the zlib header files.

If you really want to turn that particular Sparse warning off, you can us=
e

Note that you could do that much more simply by using:
gcc -E -dM -x c /dev/null | sed ...



Please go with that option.  In addition to providing an easy way to use
sparse and GCC together (make CC=3Dcgcc), cgcc defines arch-specific flag=
s that
sparse currently does not.  Ideally sparse should define these flags, but=
 that
would add some architecture-specific logic to sparse, which would then re=
quire
sparse to know the ...
From: Sam Ravnborg
Date: Saturday, June 9, 2007 - 3:56 pm

Is this the recommended way?
I that case I suggest that someone looks into the linux kernel part
and change it to use this method.

	Sam
-

From: Josh Triplett
Date: Saturday, June 9, 2007 - 4:50 pm

The approach taken by Linux allows running sparse on files without recomp=
iling
them.  Using CC=3Dcgcc just makes for less work, but the kernel has that =
work
done now.

- Josh Triplett

From: Ramsay Jones
Date: Tuesday, June 12, 2007 - 10:37 am

I have to say that, my initial reaction, was to disagree; I certainly want to
pass -Wall to sparse! Why not? Did you have any particular warnings in mind?
(I haven't noticed any that were nitpicky or had a high false positive rate!)


Why not "-Wall -Wno-nitpicky -Wno-false-positive" ;-)


Yes, I noticed that. Again, I'm not sure I agree.
I didn't comment on that patch, because my exposure to sparse is very limited.
So far I've only run it on git, so I can hardly claim any great experience with
the output from sparse. However, 105 lines of output (which represents 71 warnings)

Yes, but that was actually an improvement to the language ;-)

(As I say above, I don't really care about the NULL pointer example; I hope
the main point was not lost)

All the Best,

Ramsay Jones


-

From: Josh Triplett
Date: Wednesday, June 13, 2007 - 1:25 pm

rate!)

If you don't mind the set of warnings you get, then sure, use -Wall.

Some of the ones I had in mind:
* -Wshadow.  Not everyone cares.
* -Wptr-subtraction-blows.  This warns any time you do ptr2 - ptr1.
* -Wundefined-preprocessor.  This warns if you ever do
  #if SYMBOL
  when SYMBOL might not actually have a definition.  Many projects do exa=
ctly
  that, and the C standard allows it.
* -Wtypesign.  Off by default for the same reason that GCC doesn't give s=
ign
   mismatches by default: too many codebases with too many sloppy signedn=
ess

If you don't mind that, then sure.  You might have to adjust the warning =
list
to taste from time to time.  But please do use -Wall if you feel comforta=
ble
e.

True; for a project the size of Git, you can reasonably handle all the
warnings as you did.

If you want to use -Wall with sparse, you can always pass -Wall to sparse=

directly, or use CHECK=3D"sparse -Wall" cgcc. =20

- Josh Triplett

Previous thread: [RFC][PATCH 01/10] Sparse: fix "non-ANSI function declaration" warnings by Ramsay Jones on Friday, June 8, 2007 - 3:09 pm. (1 message)

Next thread: [PATCH] git-p4import.py: handle paths with symlinks by Kevin Green on Friday, June 8, 2007 - 3:33 pm. (1 message)