Ramsay Jones wrote: > 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. Glad to hear it! > 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". 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 exactly 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 desired, 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. See below for other reasons why you should use cgcc. That said, this suggests that perhaps Sparse should treat -Wall differently for compatibility with GCC; specifically, perhaps Sparse should just ignore -Wall, as its meaning with GCC (enable a reasonable default set of warnings) already occurs by default in Sparse. The current -Wall could become something like -Weverything. This would make Sparse somewhat less intuitive, but somewhat more GCC compatible. > 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 Awesome. I'll take a look at that patch series. > 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_...'" are easy to remove, I just didn't > get around to doing it (The GIT_... symbols are macros, defined in individual > make rules rather than CFLAGS, an thus not passed to sparse). You can easily fix that problem if you use cgcc as CC. > The four "Using plain integer...", whilst equally easy to remove, arguably should > not be ;-) If you look at the code you will find they are all of the form > x = crc32(0, Z_NULL, 0); > where the second parameter type is basically (unsigned char *) and the Z_NULL > macro is defined in the zlib header file as 0. It could be said that this is > "idiomatic zlib usage" and should remain as written. If you don't subscribe > to that view, then the required patch is obvious :P) [...] > [Note: As far as the NULL pointer warnings are concerned, I don't much care either > way. I just used that as an example (also note patch 02). Having said that, I > do think that the "NULL is the only one true null pointer" brigade need to > chill out a little; in fact I remember when 0 was the *only* null pointer.] 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 use -Wno-non-pointer-null. However, I don't think you should do that. > If you are on Linux and want to play along, then the official version 0.3 > release of sparse should work for you, along with a minor change to the > Makefile thus: > > --->8--- > diff --git a/Makefile b/Makefile > index 19b6da1..ac3e2af 100644 > --- a/Makefile > +++ b/Makefile > @@ -184,7 +184,7 @@ export TCL_PATH TCLTK_PATH > > # 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 = -D__BIG_ENDIAN__ -D__powerpc__ > +SPARSE_FLAGS = -D__STDC__=1 $(shell cat .sparse_flags) > > --->8--- > > where the .sparse_flags file is created with a script (gen-sparse-flags.sh) > as follows: > > --->8--- > #/bin/sh > > rm -f /tmp/foo.h .macros; touch /tmp/foo.h > > gcc -E -dM /tmp/foo.h >.macros > sed -e "s/^#define /-D'/" -e "s/ /=/" -e "s/$/'/" <.macros >.sparse_flags > > rm -f /tmp/foo.h > > --->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. > Note: setting __STDC__ in the SPARSE_FLAGS should not be necessary (I thought > 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. > As an alternative, you could clear the SPARSE_FLAGS and change the "check" target > 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=cgcc), cgcc defines arch-specific flags that sparse currently does not. Ideally sparse should define these flags, but that would add some architecture-specific logic to sparse, which would then require sparse to know the desired machine target. That may need to happen in the future, but I don't have a good plan for how to do it yet. In the meantime, please run sparse through cgcc. Also, you might consider just using cgcc to run both GCC and Sparse. That would handle the issue of target-specific CFLAGS, by ensuring that Sparse and GCC always see the same CFLAGS. - Josh Triplett
Attachment:
signature.asc
Description: OpenPGP digital signature
