git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Ramsay Jones <ramsay@ramsayjones.plus.com>
To: Junio C Hamano <gitster@pobox.com>,
	GIT Mailing-list <git@vger.kernel.org>
Subject: Re: [PATCH] Makefile: enable -Wsparse-error for DEVELOPER build
Date: Mon, 2 Nov 2020 00:05:30 +0000	[thread overview]
Message-ID: <6676a08d-0c25-14ba-4ea6-677bc7ab0e20@ramsayjones.plus.com> (raw)
In-Reply-To: <xmqqsg9uxchb.fsf@gitster.c.googlers.com>

Hi Junio,

On 31/10/2020 22:22, Junio C Hamano wrote:
> With -Wsparse-error, "make sparse" would fail, instead of just
> giving a warning message.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  config.mak.dev | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git c/config.mak.dev w/config.mak.dev
> index 3126a5364d..022fb58218 100644
> --- c/config.mak.dev
> +++ w/config.mak.dev
> @@ -1,5 +1,6 @@
>  ifeq ($(filter no-error,$(DEVOPTS)),)
>  DEVELOPER_CFLAGS += -Werror
> +SPARSE_FLAGS += -Wsparse-error
>  endif
>  ifneq ($(filter pedantic,$(DEVOPTS)),)
>  DEVELOPER_CFLAGS += -pedantic
> 

I certainly wouldn't object to such a patch, but I'm not sure who it
would actually help. ;-)

As you may already know, I 'make sparse >sp-out 2>&1' on the master
branch, use vim to check for warnings/errors, and then diff the
corresponding files for 'next' and 'seen' branches (nsp-out, ssp-out).
Similarly, I 'make -k hdr-check >hcout 2>&1' on the master branch,
use vim ... etc. Note, however, the use of '-k' on the make invocation
to make sure I catch all warnings/errors, since '-Werror' is given with
the DEVELOPER variable set. So, I would have to do the same with the
'make sparse' invocation after this patch [1].

Yes, anybody who just does 'make sparse' will notice the failure, so
that would be a definite improvement. (How many people run 'make sparse'
though?). It probably would help anybody using 'cgcc' to develop, but
this doesn't quite work (I just tried this for the first time in ages
and was surprised to find it almost works for me, YMMV):

  $ git branch -v | grep '^\*'
  * seen         4141ae2199 Merge branch 'dd/upload-pack-stateless-eof' into seen
  $ make clean >/dev/null
  $ 

  $ make CC=cgcc >sout2 2>&1
  $ ./git version
  git version 2.29.2.389.g4141ae2199
  $ git describe
  v2.29.2-389-g4141ae2199
  $ 

  $ grep 'warn' sout2
  pack-revindex.c:66:23: warning: memset with byte count of 262144
  upload-pack.c:1114:86: warning: Using plain integer as NULL pointer
  $ 

So, this would have failed, because some extra flags ('SP_EXTRA_FLAGS' to
be precise) are not being passed to sparse. We can see that:

  $ grep SP_EXTRA_FLAGS Makefile
  SP_EXTRA_FLAGS = -Wno-universal-initializer
  http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SP_EXTRA_FLAGS += \
  pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count
  compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null
  		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $<
  $ 

(Note that you can't see -DCURL_DISABLE_TYPECHECK being set for all the
libcurl using files). So, either sparse has improved to the point that
the 'typecheck' shenanigans used in the libcurl headers don't cause it
problems anymore, or the headers have changed sufficiently. (Hmm, that
header doesn't seem to have changed much).

I don't build with nedmalloc, but last time I did it was littered with
uses of '0' as a NULL pointer (hence the -Wno-non-pointer-null). Given
that we will probably not re-import that code, I suppose all of those
sparse warnings could be fixed.

Which leaves 'pack-revindex.c'. Unfortunately, I don't see an easy fix
for that (-Wno-universal-initializer is now the default, so we don't
need that anymore). Well, I suppose anybody could add 'CFLAGS+=-Wno-\
memcpy-max-count' to their config.mak file (this '-W' argument is
filtered out by cgcc and not passed to gcc). [EDIT: I just tried this
and, yes, this does work! I'm still surprised about curl headers ;-) ]

Note also that my build slowed down by 14% using CC=cgcc, since it is
running both sparse and gcc on each source file.

I guess it would be most useful on a CI build, but I don't know what
would be involved in setting that up.

ATB,
Ramsay Jones

[1] It took about one month for me to get used to the 'pu'->'seen' change,
    but that was mostly training my fingers to type all of the output
    files that are keyed by the branch name: pout->sout, psp-out->ssp-out,
    psc->ssc, phcout->shcout and ptest-out->stest-out. :D


  reply	other threads:[~2020-11-02  0:05 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-31 21:04 [PATCH] upload-pack.c: fix a sparse warning Ramsay Jones
2020-10-31 22:22 ` [PATCH] Makefile: enable -Wsparse-error for DEVELOPER build Junio C Hamano
2020-11-02  0:05   ` Ramsay Jones [this message]
2020-11-02 18:55     ` Junio C Hamano
2020-11-03  2:04       ` Ramsay Jones
2020-11-03  2:50         ` Junio C Hamano
2020-11-04 13:43         ` Johannes Schindelin
2020-11-04 16:57           ` Ramsay Jones
2020-11-04 18:11             ` Junio C Hamano
2020-11-04 20:05               ` Ramsay Jones
2020-11-02 22:21 ` [PATCH] upload-pack.c: fix a sparse warning Josh Steadmon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6676a08d-0c25-14ba-4ea6-677bc7ab0e20@ramsayjones.plus.com \
    --to=ramsay@ramsayjones.plus.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).