{"thread":{"id":"8513","subject":"[RFC][PATCH 00/10] Sparse: Git's \"make check\" target","startedAt":"2007-06-08T22:06:42Z","lastAt":"2007-06-13T20:25:08Z","messageCount":6,"participants":["Ramsay Jones","Josh Triplett","Sam Ravnborg"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"44388","messageId":"4669D2F2.90801@ramsay1.demon.co.uk","threadId":"8513","inReplyTo":null,"subject":"[RFC][PATCH 00/10] Sparse: Git's \"make check\" target","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2007-06-08T22:06:42Z","receivedAt":"2007-06-08T22:06:42Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Junio,\n\nSince the Git Makefile has a \"check\" target that uses sparse, I decided to\ntake a look at the sparse project to see what it was about, and what it\nhas to say about the git source code.\n\nInitially, I had many problems because I am using cygwin, but I was able to\nfix most of those problems. (the output from \"make check\" was about 16k lines\nat one point!). Git also tickled a bug in sparse 0.2, which resulted in some\n120+ lines of bogus warnings; that was fixed in version 0.3 (commit 0.2-15-gef25961).\nAs a result, sparse version 0.3 + my patches, elicits 106 lines of output\nfrom \"make check\".\n\nNaturally, I decided to \"fix\" the warnings produced by sparse, which resulted\nin the following patch series:\n\n[PATCH 01/10] Sparse: fix \"non-ANSI function declaration\" warnings\n[PATCH 02/10] Sparse: fix some \"using integer as NULL pointer\" warnings\n[PATCH 03/10] Sparse: fix some \"symbol not declared\" warnings (Part 1)\n[PATCH 04/10] Sparse: fix some \"symbol not declared\" warnings (Part 2)\n[PATCH 05/10] Sparse: fix some \"symbol not declared\" warnings (Part 3)\n[PATCH 06/10] Sparse: fix \"'add_head' not declared\" warning\n[PATCH 07/10] Sparse: fix \"'merge_file' not declared\" warning\n[PATCH 08/10] Sparse: fix an \"incorrect type in argument n\" warning\n[PATCH 09/10] Sparse: fix some \"symbol 's' shadows an earlier one\" warnings\n[PATCH 10/10] Sparse: fix a \"symbol 'weak_match' shadows an earlier one\" warning\n\nHowever, this patch series does not completely remove all warnings; the output\nis reduced to:\n\nbuiltin-pack-objects.c:312:31: warning: Using plain integer as NULL pointer\ncsum-file.c:152:22: warning: Using plain integer as NULL pointer\nexec_cmd.c:7:40: error: undefined identifier 'GIT_EXEC_PATH'\ngit.c:209:35: error: undefined identifier 'GIT_VERSION'\nhttp.c:203:46: error: undefined identifier 'GIT_USER_AGENT'\nindex-pack.c:201:25: warning: Using plain integer as NULL pointer\nindex-pack.c:538:26: warning: Using plain integer as NULL pointer\n\nThe three \"undefined identifier 'GIT_...'\" are easy to remove, I just didn't\nget around to doing it (The GIT_... symbols are macros, defined in individual\nmake rules rather than CFLAGS, an thus not passed to sparse).\n\nThe four \"Using plain integer...\", whilst equally easy to remove, arguably should\nnot be ;-)  If you look at the code you will find they are all of the form\n    x = crc32(0, Z_NULL, 0);\nwhere the second parameter type is basically (unsigned char *) and the Z_NULL\nmacro is defined in the zlib header file as 0.  It could be said that this is\n\"idiomatic zlib usage\" and should remain as written.  If you don't subscribe\nto that view, then the required patch is obvious :P)\n\nThe above is one reason why this is an RFC series.  With one notable exception,\nthe patches do not fix a bug, add a feature or alter the program behaviour.\nThe only thing they do is remove sparse warnings; so you really have to believe\nthat this is a worthwhile thing to do, otherwise they are worthless!\n\n[Note: As far as the NULL pointer warnings are concerned, I don't much care either\nway. I just used that as an example (also note patch 02). Having said that, I\ndo think that the \"NULL is the only one true null pointer\" brigade need to\nchill out a little; in fact I remember when 0 was the *only* null pointer.]\n\nAnother reason for the RFC, is that the patches are against the v1.5.2 tar-ball.\nSince this was released a few weeks ago, the patches may not apply cleanly to\ncurrent git. (Yes, I really need to bite the bullet and get a broadband\nconnection so that I can clone the git repo.)\n\nAlso, that \"one notable exception\" is patch 10, which fixes a bug caused by a\n\"shadow\" declaration.  Unfortunately, it also crashes my laptop when running\nthe test-suite. Yes, the machine freezes solid, with no option but to pull\nthe power-cord, and then the battery ... (I blush to have to admit that it\nwasn't until the third time this happened that I realized that it might be\nan idea not to put the battery back ... ;-))\n\nThe first nine patches all pass \"make test\" no problem, but patch 10 causes\ntotally unpredictable mayhem. The crash seems to occur randomly, on any test,\neven those which can't possibly be affected by the change (when have you\nheard that before...). As far as I can see, only git-http-push, git-send-pack\nand git-push should be affected, which in turn means only t5400-send-pack.sh,\nt5401-update-hooks.sh and t9400-git-cvsserver-server.sh.  None of those tests\nhave caused a crash. (actually, since I don't have cvs installed, t9400* is\nnot even running)\n\nThe nature of the crash has made debugging this such a nightmare that I've\njust given up!  Hopefully, somebody on a system which does not grind to a\nhalt (ie. not using cygwin) can find the bug. (I think that cygwin may be\npart of the problem here)\n\nIf the patches are whitespace damaged, please let me know and I will resend\nas attachments.  (As josh noticed on the sparse list, my thunderbird\nconfiguration was causing WS damage to the patches. I hope I have finally\nfixed that, but I can't guarantee it!)\n\nI've included a few more notes below, if you want to reproduce the\nsparse output.\n\nHopefully someone will find this useful.\n\nAll the best,\n\nRamsay Jones\n\nIf you are on Linux and want to play along, then the official version 0.3\nrelease of sparse should work for you, along with a minor change to the\nMakefile thus:\n\n--->8---\ndiff --git a/Makefile b/Makefile\nindex 19b6da1..ac3e2af 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -184,7 +184,7 @@ export TCL_PATH TCLTK_PATH\n \n # sparse is architecture-neutral, which means that we need to tell it\n # explicitly what architecture to check for. Fix this up for yours..\n-SPARSE_FLAGS = -D__BIG_ENDIAN__ -D__powerpc__\n+SPARSE_FLAGS = -D__STDC__=1 $(shell cat .sparse_flags)\n \n \n \n--->8---\n\nwhere the .sparse_flags file is created with a script (gen-sparse-flags.sh)\nas follows:\n\n--->8---\n#/bin/sh\n\nrm -f /tmp/foo.h .macros; touch /tmp/foo.h\n\ngcc -E -dM /tmp/foo.h >.macros\nsed -e \"s/^#define /-D'/\" -e \"s/ /=/\" -e \"s/$/'/\" <.macros >.sparse_flags \n\nrm -f /tmp/foo.h\n\n--->8---\n\nNote: setting __STDC__ in the SPARSE_FLAGS should not be necessary (I thought\nI needed it at one point...) but I didn't get around to removing it.\n\nAs an alternative, you could clear the SPARSE_FLAGS and change the \"check\" target\nto call \"cgcc -no-compile\" in place of sparse.\n\nBTW sparse Git repo: git://git.kernel.org/pub/scm/devel/sparse/sparse.git\n"},{"id":"44434","messageId":"466A5204.6060200@freedesktop.org","threadId":"8513","inReplyTo":"4669D2F2.90801@ramsay1.demon.co.uk","subject":"Re: [RFC][PATCH 00/10] Sparse: Git's \"make check\" target","fromName":"Josh Triplett","fromEmail":"josh@freedesktop.org","sentAt":"2007-06-09T07:08:52Z","receivedAt":"2007-06-09T07:08:52Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"Ramsay Jones wrote:\n> Since the Git Makefile has a \"check\" target that uses sparse, I decided to\n> take a look at the sparse project to see what it was about, and what it\n> has to say about the git source code.\n\nGlad to hear it!\n\n> Initially, I had many problems because I am using cygwin, but I was able to\n> fix most of those problems. (the output from \"make check\" was about 16k\n> lines at one point!). Git also tickled a bug in sparse 0.2, which resulted\n> in some 120+ lines of bogus warnings; that was fixed in version 0.3 (commit\n> 0.2-15-gef25961).  As a result, sparse version 0.3 + my patches, elicits 106\n> lines of output from \"make check\".\n\nOne note about using Sparse with Git: you almost certainly don't want to pass\n-Wall to sparse, and current Git passes CFLAGS to Sparse which will do exactly\nthat.  -Wall turns on all possible Sparse warnings, including nitpicky\nwarnings and warnings with a high false positive rate.  You should start from\nthe default set of Sparse warnings, and add additional warnings as desired, or\nturn off those you absolutely can't live with.  Current Sparse from Git (post\n0.3, after commit e18c1014449adf42520daa9d3e53f78a3d98da34) has a change to\ncgcc to filter out -Wall, so you can pass -Wall to GCC but not Sparse.  See\nbelow for other reasons why you should use cgcc.\n\nThat said, this suggests that perhaps Sparse should treat -Wall differently\nfor compatibility with GCC; specifically, perhaps Sparse should just ignore\n-Wall, as its meaning with GCC (enable a reasonable default set of warnings)\nalready occurs by default in Sparse.  The current -Wall could become something\nlike -Weverything.  This would make Sparse somewhat less intuitive, but\nsomewhat more GCC compatible.\n\n> Naturally, I decided to \"fix\" the warnings produced by sparse, which resulted\n> in the following patch series:\n> \n> [PATCH 01/10] Sparse: fix \"non-ANSI function declaration\" warnings\n> [PATCH 02/10] Sparse: fix some \"using integer as NULL pointer\" warnings\n> [PATCH 03/10] Sparse: fix some \"symbol not declared\" warnings (Part 1)\n> [PATCH 04/10] Sparse: fix some \"symbol not declared\" warnings (Part 2)\n> [PATCH 05/10] Sparse: fix some \"symbol not declared\" warnings (Part 3)\n> [PATCH 06/10] Sparse: fix \"'add_head' not declared\" warning\n> [PATCH 07/10] Sparse: fix \"'merge_file' not declared\" warning\n> [PATCH 08/10] Sparse: fix an \"incorrect type in argument n\" warning\n> [PATCH 09/10] Sparse: fix some \"symbol 's' shadows an earlier one\" warnings\n> [PATCH 10/10] Sparse: fix a \"symbol 'weak_match' shadows an earlier one\" warning\n\nAwesome.  I'll take a look at that patch series.\n\n> However, this patch series does not completely remove all warnings; the output\n> is reduced to:\n> \n> builtin-pack-objects.c:312:31: warning: Using plain integer as NULL pointer\n> csum-file.c:152:22: warning: Using plain integer as NULL pointer\n> exec_cmd.c:7:40: error: undefined identifier 'GIT_EXEC_PATH'\n> git.c:209:35: error: undefined identifier 'GIT_VERSION'\n> http.c:203:46: error: undefined identifier 'GIT_USER_AGENT'\n> index-pack.c:201:25: warning: Using plain integer as NULL pointer\n> index-pack.c:538:26: warning: Using plain integer as NULL pointer\n> \n> The three \"undefined identifier 'GIT_...'\" are easy to remove, I just didn't\n> get around to doing it (The GIT_... symbols are macros, defined in individual\n> make rules rather than CFLAGS, an thus not passed to sparse).\n\nYou can easily fix that problem if you use cgcc as CC.\n\n> The four \"Using plain integer...\", whilst equally easy to remove, arguably should\n> not be ;-)  If you look at the code you will find they are all of the form\n>     x = crc32(0, Z_NULL, 0);\n> where the second parameter type is basically (unsigned char *) and the Z_NULL\n> macro is defined in the zlib header file as 0.  It could be said that this is\n> \"idiomatic zlib usage\" and should remain as written.  If you don't subscribe\n> to that view, then the required patch is obvious :P)\n[...]\n> [Note: As far as the NULL pointer warnings are concerned, I don't much care either\n> way. I just used that as an example (also note patch 02). Having said that, I\n> do think that the \"NULL is the only one true null pointer\" brigade need to\n> chill out a little; in fact I remember when 0 was the *only* null pointer.]\n\nAnd at one point prototypes didn't exist either. :)\n\nOther valid null pointers exist, such as (void *)0.  You could also use (char\n*)0 in this particular case.  Sparse complains because you just use the\ninteger 0.  I suggest just using NULL.\n\nIf you want to keep using Z_NULL rather than NULL, you could always undef it\nand define it as NULL after including the zlib header files.\n\nIf you really want to turn that particular Sparse warning off, you can use\n-Wno-non-pointer-null.  However, I don't think you should do that.\n\n> If you are on Linux and want to play along, then the official version 0.3\n> release of sparse should work for you, along with a minor change to the\n> Makefile thus:\n> \n> --->8---\n> diff --git a/Makefile b/Makefile\n> index 19b6da1..ac3e2af 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -184,7 +184,7 @@ export TCL_PATH TCLTK_PATH\n>  \n>  # sparse is architecture-neutral, which means that we need to tell it\n>  # explicitly what architecture to check for. Fix this up for yours..\n> -SPARSE_FLAGS = -D__BIG_ENDIAN__ -D__powerpc__\n> +SPARSE_FLAGS = -D__STDC__=1 $(shell cat .sparse_flags)\n>  \n> --->8---\n> \n> where the .sparse_flags file is created with a script (gen-sparse-flags.sh)\n> as follows:\n> \n> --->8---\n> #/bin/sh\n> \n> rm -f /tmp/foo.h .macros; touch /tmp/foo.h\n> \n> gcc -E -dM /tmp/foo.h >.macros\n> sed -e \"s/^#define /-D'/\" -e \"s/ /=/\" -e \"s/$/'/\" <.macros >.sparse_flags \n> \n> rm -f /tmp/foo.h\n> \n> --->8---\n\nNote that you could do that much more simply by using:\ngcc -E -dM -x c /dev/null | sed ...\n\nHowever, see below about using cgcc instead.\n\n> Note: setting __STDC__ in the SPARSE_FLAGS should not be necessary (I thought\n> I needed it at one point...) but I didn't get around to removing it.\n\nSparse has defined __STDC__ since 2003, well before even version 0.1.\n\n> As an alternative, you could clear the SPARSE_FLAGS and change the \"check\" target\n> to call \"cgcc -no-compile\" in place of sparse.\n\nPlease go with that option.  In addition to providing an easy way to use\nsparse and GCC together (make CC=cgcc), cgcc defines arch-specific flags that\nsparse currently does not.  Ideally sparse should define these flags, but that\nwould add some architecture-specific logic to sparse, which would then require\nsparse to know the desired machine target.  That may need to happen in the\nfuture, but I don't have a good plan for how to do it yet.  In the meantime,\nplease run sparse through cgcc.\n\nAlso, you might consider just using cgcc to run both GCC and Sparse.  That\nwould handle the issue of target-specific CFLAGS, by ensuring that Sparse and\nGCC always see the same CFLAGS.\n\n- Josh Triplett\n\n\n"},{"id":"44509","messageId":"20070609225630.GC3008@uranus.ravnborg.org","threadId":"8513","inReplyTo":"466A5204.6060200@freedesktop.org","subject":"Re: [RFC][PATCH 00/10] Sparse: Git's \"make check\" target","fromName":"Sam Ravnborg","fromEmail":"sam@ravnborg.org","sentAt":"2007-06-09T22:56:30Z","receivedAt":"2007-06-09T22:56:30Z","isPatch":true,"sender":{"key":"sam@ravnborg.org","avatar":"https://gravatar.com/avatar/168a912606ed0742d840bb365e3cc21db390c36531a58341dc7a069cc1f15f62?d=mp&s=160"},"body":"> \n> Also, you might consider just using cgcc to run both GCC and Sparse.  That\n> would handle the issue of target-specific CFLAGS, by ensuring that Sparse and\n> GCC always see the same CFLAGS.\n\nIs this the recommended way?\nI that case I suggest that someone looks into the linux kernel part\nand change it to use this method.\n\n\tSam\n"},{"id":"44513","messageId":"466B3CC8.4010508@freedesktop.org","threadId":"8513","inReplyTo":"20070609225630.GC3008@uranus.ravnborg.org","subject":"Re: [RFC][PATCH 00/10] Sparse: Git's \"make check\" target","fromName":"Josh Triplett","fromEmail":"josh@freedesktop.org","sentAt":"2007-06-09T23:50:32Z","receivedAt":"2007-06-09T23:50:32Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"Sam Ravnborg wrote:\n>> Also, you might consider just using cgcc to run both GCC and Sparse.  That\n>> would handle the issue of target-specific CFLAGS, by ensuring that Sparse and\n>> GCC always see the same CFLAGS.\n> \n> Is this the recommended way?\n> I that case I suggest that someone looks into the linux kernel part\n> and change it to use this method.\n\nThe approach taken by Linux allows running sparse on files without recompiling\nthem.  Using CC=cgcc just makes for less work, but the kernel has that work\ndone now.\n\n- Josh Triplett\n\n"},{"id":"44850","messageId":"466ED9CE.3000800@ramsay1.demon.co.uk","threadId":"8513","inReplyTo":"466A5204.6060200@freedesktop.org","subject":"Re: [RFC][PATCH 00/10] Sparse: Git's \"make check\" target","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2007-06-12T17:37:18Z","receivedAt":"2007-06-12T17:37:18Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Josh Triplett wrote:\n> Ramsay Jones wrote:\n>> fix most of those problems. (the output from \"make check\" was about 16k\n>> lines at one point!). Git also tickled a bug in sparse 0.2, which resulted\n>> in some 120+ lines of bogus warnings; that was fixed in version 0.3 (commit\n>> 0.2-15-gef25961).  As a result, sparse version 0.3 + my patches, elicits 106\n>> lines of output from \"make check\".\n> \n> One note about using Sparse with Git: you almost certainly don't want to pass\n> -Wall to sparse, and current Git passes CFLAGS to Sparse which will do exactly\n> that.  -Wall turns on all possible Sparse warnings, including nitpicky\n> warnings and warnings with a high false positive rate.\n\nI have to say that, my initial reaction, was to disagree; I certainly want to\npass -Wall to sparse! Why not? Did you have any particular warnings in mind?\n(I haven't noticed any that were nitpicky or had a high false positive rate!)\n\n...  You should start from\n> the default set of Sparse warnings, and add additional warnings as desired, or\n> turn off those you absolutely can't live with.  \n\nWhy not \"-Wall -Wno-nitpicky -Wno-false-positive\" ;-)\n\n... Current Sparse from Git (post\n> 0.3, after commit e18c1014449adf42520daa9d3e53f78a3d98da34) has a change to\n> cgcc to filter out -Wall, so you can pass -Wall to GCC but not Sparse.  \n\nYes, I noticed that. Again, I'm not sure I agree.\nI didn't comment on that patch, because my exposure to sparse is very limited.\nSo far I've only run it on git, so I can hardly claim any great experience with\nthe output from sparse. However, 105 lines of output (which represents 71 warnings)\nfor 72,974 lines of C (in 179 .c files) did not seem at all unreasonable.\n\n>> [Note: As far as the NULL pointer warnings are concerned, I don't much care either\n>> way. I just used that as an example (also note patch 02). Having said that, I\n>> do think that the \"NULL is the only one true null pointer\" brigade need to\n>> chill out a little; in fact I remember when 0 was the *only* null pointer.]\n> \n> And at one point prototypes didn't exist either. :)\n\nYes, but that was actually an improvement to the language ;-)\n\n(As I say above, I don't really care about the NULL pointer example; I hope\nthe main point was not lost)\n\nAll the Best,\n\nRamsay Jones\n"},{"id":"44998","messageId":"467052A4.6080706@freedesktop.org","threadId":"8513","inReplyTo":"466ED9CE.3000800@ramsay1.demon.co.uk","subject":"Re: [RFC][PATCH 00/10] Sparse: Git's \"make check\" target","fromName":"Josh Triplett","fromEmail":"josh@freedesktop.org","sentAt":"2007-06-13T20:25:08Z","receivedAt":"2007-06-13T20:25:08Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"Ramsay Jones wrote:\n> Josh Triplett wrote:\n>> Ramsay Jones wrote:\n>>> fix most of those problems. (the output from \"make check\" was about 16k\n>>> lines at one point!). Git also tickled a bug in sparse 0.2, which resulted\n>>> in some 120+ lines of bogus warnings; that was fixed in version 0.3 (commit\n>>> 0.2-15-gef25961).  As a result, sparse version 0.3 + my patches, elicits 106\n>>> lines of output from \"make check\".\n>> One note about using Sparse with Git: you almost certainly don't want to pass\n>> -Wall to sparse, and current Git passes CFLAGS to Sparse which will do exactly\n>> that.  -Wall turns on all possible Sparse warnings, including nitpicky\n>> warnings and warnings with a high false positive rate.\n> \n> I have to say that, my initial reaction, was to disagree; I certainly want to\n> pass -Wall to sparse! Why not? Did you have any particular warnings in mind?\n> (I haven't noticed any that were nitpicky or had a high false positive rate!)\n\nIf you don't mind the set of warnings you get, then sure, use -Wall.\n\nSome of the ones I had in mind:\n* -Wshadow.  Not everyone cares.\n* -Wptr-subtraction-blows.  This warns any time you do ptr2 - ptr1.\n* -Wundefined-preprocessor.  This warns if you ever do\n  #if SYMBOL\n  when SYMBOL might not actually have a definition.  Many projects do exactly\n  that, and the C standard allows it.\n* -Wtypesign.  Off by default for the same reason that GCC doesn't give sign\n   mismatches by default: too many codebases with too many sloppy signedness\n   issues that drown out other issues.\n\n> ...  You should start from\n>> the default set of Sparse warnings, and add additional warnings as desired, or\n>> turn off those you absolutely can't live with.  \n> \n> Why not \"-Wall -Wno-nitpicky -Wno-false-positive\" ;-)\n\nIf you don't mind that, then sure.  You might have to adjust the warning list\nto taste from time to time.  But please do use -Wall if you feel comfortable\nwith the warnings it produces.\n\n> ... Current Sparse from Git (post\n>> 0.3, after commit e18c1014449adf42520daa9d3e53f78a3d98da34) has a change to\n>> cgcc to filter out -Wall, so you can pass -Wall to GCC but not Sparse.  \n> \n> Yes, I noticed that. Again, I'm not sure I agree.\n> I didn't comment on that patch, because my exposure to sparse is very limited.\n> So far I've only run it on git, so I can hardly claim any great experience with\n> the output from sparse. However, 105 lines of output (which represents 71 warnings)\n> for 72,974 lines of C (in 179 .c files) did not seem at all unreasonable.\n\nTrue; for a project the size of Git, you can reasonably handle all the\nwarnings as you did.\n\nIf you want to use -Wall with sparse, you can always pass -Wall to sparse\ndirectly, or use CHECK=\"sparse -Wall\" cgcc.  \n\n- Josh Triplett\n\n"}]}