Ramsay Jones wrote:
quoted text
> Since the Git Makefile has a "check" target that uses sparse, I decided=
 to
quoted text
> take a look at the sparse project to see what it was about, and what it=

quoted text
> has to say about the git source code.

Glad to hear it!

quoted text
> Initially, I had many problems because I am using cygwin, but I was abl=
e to
quoted text
> fix most of those problems. (the output from "make check" was about 16k=

quoted text
> lines at one point!). Git also tickled a bug in sparse 0.2, which resul=
ted
quoted text
> in some 120+ lines of bogus warnings; that was fixed in version 0.3 (co=
mmit
quoted text
> 0.2-15-gef25961).  As a result, sparse version 0.3 + my patches, elicit=
s 106
quoted text
> lines of output from "make check".

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
somewhat more GCC compatible.

quoted text
> Naturally, I decided to "fix" the warnings produced by sparse, which re=
sulted
quoted text
> in the following patch series:
>=20
> [PATCH 01/10] Sparse: fix "non-ANSI function declaration" warnings
> [PATCH 02/10] Sparse: fix some "using integer as NULL pointer" warnings=

quoted text
> [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" warn=
ings
quoted text
> [PATCH 10/10] Sparse: fix a "symbol 'weak_match' shadows an earlier one=
" warning

Awesome.  I'll take a look at that patch series.

quoted text
> However, this patch series does not completely remove all warnings; the=
 output
quoted text
> is reduced to:
>=20
> builtin-pack-objects.c:312:31: warning: Using plain integer as NULL poi=
nter
quoted text
> 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
>=20
> The three "undefined identifier 'GIT_...'" are easy to remove, I just d=
idn't
quoted text
> get around to doing it (The GIT_... symbols are macros, defined in indi=
vidual
quoted text
> make rules rather than CFLAGS, an thus not passed to sparse).

You can easily fix that problem if you use cgcc as CC.

quoted text
> The four "Using plain integer...", whilst equally easy to remove, argua=
bly should
quoted text
> not be ;-)  If you look at the code you will find they are all of the f=
orm
quoted text
>     x =3D crc32(0, Z_NULL, 0);
> where the second parameter type is basically (unsigned char *) and the =
Z_NULL
quoted text
> macro is defined in the zlib header file as 0.  It could be said that t=
his is
quoted text
> "idiomatic zlib usage" and should remain as written.  If you don't subs=
cribe
quoted text
> to that view, then the required patch is obvious :P)
[...]
quoted text
> [Note: As far as the NULL pointer warnings are concerned, I don't much =
care either
quoted text
> way. I just used that as an example (also note patch 02). Having said t=
hat, I
quoted text
> do think that the "NULL is the only one true null pointer" brigade need=
 to
quoted text
> chill out a little; in fact I remember when 0 was the *only* null point=
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
-Wno-non-pointer-null.  However, I don't think you should do that.

quoted text
> If you are on Linux and want to play along, then the official version 0=
=2E3
quoted text
> release of sparse should work for you, along with a minor change to the=

quoted text
> Makefile thus:
>=20
> --->8---
> diff --git a/Makefile b/Makefile
> index 19b6da1..ac3e2af 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -184,7 +184,7 @@ export TCL_PATH TCLTK_PATH
> =20
>  # sparse is architecture-neutral, which means that we need to tell it
>  # explicitly what architecture to check for. Fix this up for yours..
> -SPARSE_FLAGS =3D -D__BIG_ENDIAN__ -D__powerpc__
> +SPARSE_FLAGS =3D -D__STDC__=3D1 $(shell cat .sparse_flags)
> =20
> --->8---
>=20
> where the .sparse_flags file is created with a script (gen-sparse-flags=
=2Esh)
quoted text
> as follows:
>=20
> --->8---
> #/bin/sh
>=20
> rm -f /tmp/foo.h .macros; touch /tmp/foo.h
>=20
> gcc -E -dM /tmp/foo.h >.macros
> sed -e "s/^#define /-D'/" -e "s/ /=3D/" -e "s/$/'/" <.macros >.sparse_f=
lags=20
quoted text
>=20
> rm -f /tmp/foo.h
>=20
> --->8---

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

However, see below about using cgcc instead.

quoted text
> Note: setting __STDC__ in the SPARSE_FLAGS should not be necessary (I t=
hought
quoted text
> I needed it at one point...) but I didn't get around to removing it.

Sparse has defined __STDC__ since 2003, well before even version 0.1.

quoted text
> As an alternative, you could clear the SPARSE_FLAGS and change the "che=
ck" target
quoted text
> to call "cgcc -no-compile" in place of sparse.

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 desired machine target.  That may need to happen in th=
e
future, but I don't have a good plan for how to do it yet.  In the meanti=
me,
please run sparse through cgcc.

Also, you might consider just using cgcc to run both GCC and Sparse.  Tha=
t
would handle the issue of target-specific CFLAGS, by ensuring that Sparse=
 and
GCC always see the same CFLAGS.

- Josh Triplett