[PATCH] usage: add NORETURN to BUG() function definitions

[PATCH] usage: add NORETURN to BUG() function definitions

[Ramsay Jones]

Commit d8193743e0 ("usage.c: add BUG() function", 12-05-2017) added the
BUG() functions and macros as a replacement for calls to die("BUG: ..").
The use of NORETURN on the declarations (in git-compat-util.h) and the
lack of NORETURN on the function definitions, however, leads sparse to
complain thus:

      SP usage.c
  usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
  (originally declared at git-compat-util.h:1074) - different modifiers

In order to suppress the sparse error, add the NORETURN to the function
definitions.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Junio,

This is built on 'next', which has now merged the 'jk/bug-to-abort'
branch.

Thanks!

ATB,
Ramsay Jones

 usage.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/usage.c b/usage.c
index 7e6cb2028..1f63e033e 100644
--- a/usage.c
+++ b/usage.c
@@ -217,7 +217,7 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
 }
 
 #ifdef HAVE_VARIADIC_MACROS
-void BUG_fl(const char *file, int line, const char *fmt, ...)
+NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
 {
 	va_list ap;
 	va_start(ap, fmt);
@@ -225,7 +225,7 @@ void BUG_fl(const char *file, int line, const char *fmt, ...)
 	va_end(ap);
 }
 #else
-void BUG(const char *fmt, ...)
+NORETURN void BUG(const char *fmt, ...)
 {
 	va_list ap;
 	va_start(ap, fmt);
-- 
2.13.0

Re: [PATCH] usage: add NORETURN to BUG() function definitions

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> Commit d8193743e0 ("usage.c: add BUG() function", 12-05-2017) added the
> BUG() functions and macros as a replacement for calls to die("BUG: ..").
> The use of NORETURN on the declarations (in git-compat-util.h) and the
> lack of NORETURN on the function definitions, however, leads sparse to
> complain thus:
>
>       SP usage.c
>   usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
>   (originally declared at git-compat-util.h:1074) - different modifiers
>
> In order to suppress the sparse error, add the NORETURN to the function
> definitions.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Junio,
>
> This is built on 'next', which has now merged the 'jk/bug-to-abort'
> branch.

Thanks.  

I cannot seem to cause sparse (v0.5.0-207-g14964df) to complain the
same way, though.

>
> Thanks!
>
> ATB,
> Ramsay Jones
>
>  usage.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/usage.c b/usage.c
> index 7e6cb2028..1f63e033e 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -217,7 +217,7 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
>  }
>  
>  #ifdef HAVE_VARIADIC_MACROS
> -void BUG_fl(const char *file, int line, const char *fmt, ...)
> +NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
>  {
>  	va_list ap;
>  	va_start(ap, fmt);
> @@ -225,7 +225,7 @@ void BUG_fl(const char *file, int line, const char *fmt, ...)
>  	va_end(ap);
>  }
>  #else
> -void BUG(const char *fmt, ...)
> +NORETURN void BUG(const char *fmt, ...)
>  {
>  	va_list ap;
>  	va_start(ap, fmt);

Re: [PATCH] usage: add NORETURN to BUG() function definitions

[Ramsay Jones]

On 22/05/17 02:43, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> Commit d8193743e0 ("usage.c: add BUG() function", 12-05-2017) added the
>> BUG() functions and macros as a replacement for calls to die("BUG: ..").
>> The use of NORETURN on the declarations (in git-compat-util.h) and the
>> lack of NORETURN on the function definitions, however, leads sparse to
>> complain thus:
>>
>>       SP usage.c
>>   usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
>>   (originally declared at git-compat-util.h:1074) - different modifiers
>>
>> In order to suppress the sparse error, add the NORETURN to the function
>> definitions.
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> ---
>>
>> Hi Junio,
>>
>> This is built on 'next', which has now merged the 'jk/bug-to-abort'
>> branch.
> 
> Thanks.  
> 
> I cannot seem to cause sparse (v0.5.0-207-g14964df) to complain the
> same way, though.

Hmm, interesting... I have an older version installed:

  $ sparse --version
  v0.5.0-60-g6c283a0
  $ sparse usage.c
  usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
  (originally declared at git-compat-util.h:1074) - different modifiers
  $ 

... but a more up-to-date version agrees:

  $ ~/sparse/sparse --version
  v0.5.0-237-g51de1cc
  $ ~/sparse/sparse usage.c
  usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
  (originally declared at git-compat-util.h:1074) - different modifiers
  $ 

So, I don't know. Wait let me try your specific version:

$ ~/sparse/sparse --version
v0.5.0-207-g14964df
$ ~/sparse/sparse usage.c
usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type (originally declared at git-compat-util.h:1074) - different modifiers
$ 

Er, dunno. (This is on Linux Mint 18.1).

ATB,
Ramsay Jones

Re: [PATCH] usage: add NORETURN to BUG() function definitions

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> So, I don't know. Wait let me try your specific version:
>
> $ ~/sparse/sparse --version
> v0.5.0-207-g14964df
> $ ~/sparse/sparse usage.c
> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type (originally declared at git-compat-util.h:1074) - different modifiers
> $ 
>
> Er, dunno. (This is on Linux Mint 18.1).

Oh, I don't question your expertise or competence.  There must be
something I am doing wrong, and the version of sparse I happened to
have run was the easiest thing to point a finger at, but that does
not seem to be it.

Thanks for helping.  I'll find time to dig deeper to find what's
breaking it for me.

Re: [PATCH] usage: add NORETURN to BUG() function definitions

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> So, I don't know. Wait let me try your specific version:
>>
>> $ ~/sparse/sparse --version
>> v0.5.0-207-g14964df
>> $ ~/sparse/sparse usage.c
>> usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type (originally declared at git-compat-util.h:1074) - different modifiers
>> $ 
>>
>> Er, dunno. (This is on Linux Mint 18.1).
>
> Oh, I don't question your expertise or competence.  There must be
> something I am doing wrong, and the version of sparse I happened to
> have run was the easiest thing to point a finger at, but that does
> not seem to be it.
>
> Thanks for helping.  I'll find time to dig deeper to find what's
> breaking it for me.

Hmph.  I do not know what went wrong.  The one I had in /usr/bin
that came from the distro was too old that it didn't give any useful
result and failed, and that was why I got v0.5.0-207-g14964df
installed in ~/gitstuff/bin/ which is early on my $PATH; I do not
think I did any other updates but now I am seeing happy results.

        $ git checkout jk/bug-to-abort^1
        $ make SP_OBJ=usage.sp sparse
        GIT_VERSION = 2.13.0.3.g25cd291963
            SP usage.c
        usage.c:220:6: error: symbol 'BUG_fl' redeclared with diff...

And then with your fix, of course,

        $ git checkout jk/bug-to-abort
        $ make SP_OBJ=usage.sp sparse
        GIT_VERSION = 2.13.0.4.g3d7dd2d3b6
            SP usage.c

I am still puzzled but anyway now the problem is clearly on my end
and no longer reproduces, there is no reason to waste your time.

Sorry for the noise, and thanks for a fix again.

Re: [PATCH] usage: add NORETURN to BUG() function definitions

[Ramsay Jones]

On 22/05/17 03:46, Junio C Hamano wrote:
> Hmph.  I do not know what went wrong.  The one I had in /usr/bin
> that came from the distro was too old that it didn't give any useful
> result and failed, and that was why I got v0.5.0-207-g14964df
> installed in ~/gitstuff/bin/ which is early on my $PATH; I do not
> think I did any other updates but now I am seeing happy results.
> 
>         $ git checkout jk/bug-to-abort^1
>         $ make SP_OBJ=usage.sp sparse
>         GIT_VERSION = 2.13.0.3.g25cd291963
>             SP usage.c
>         usage.c:220:6: error: symbol 'BUG_fl' redeclared with diff...

To save yourself a little typing, if you want to run sparse over a
single file:

    $ make usage.sp  # ie simply replace '.c' with '.sp'

... is the recommended way to do it. This makes sure that the same
flags are passed to cgcc as are passed to gcc as part of the build.

Note that I would not normally run sparse directly on a source file
(except when messing around with different versions!), the idea is
to use cgcc as a frontend (as the Makefile does).

Having said that, I rarely run sparse over just one file (except
when fixing a sparse error/warning). On each branch (master->next->pu)
I do

    $ make sparse >sp-out 2>&1 # nsp-out on 'next', psp-out on 'pu'

... so that I can diff the files from branch to branch. (I check the
master branch file by hand. There is a single warning on Linux that
is actually a sparse problem).

Just FYI, for today's fetch:

    $ diff sp-out nsp-out
    $ diff nsp-out psp-out
    12a13
    >     SP blame.c
    42a44,46
    > diff.c:813:6: warning: symbol 'emit_line' was not declared. Should it be static?
    > diff.c:828:6: warning: symbol 'emit_line_fmt' was not declared. Should it be static?
    > diff.c:1865:6: warning: symbol 'print_stat_summary_0' was not declared. Should it be static?
    54a59
    >     SP fsmonitor.c
    137a143
    >     SP sub-process.c
    170a177
    >     SP compat/fopen.c
    276a284
    > builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer
    296a305
    >     SP t/helper/test-dir-iterator.c
    $ 
    

ATB,
Ramsay Jones

Re: [PATCH] usage: add NORETURN to BUG() function definitions

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> Having said that, I rarely run sparse over just one file (except
> when fixing a sparse error/warning). On each branch (master->next->pu)
> I do
>
>     $ make sparse >sp-out 2>&1 # nsp-out on 'next', psp-out on 'pu'
>
> ... so that I can diff the files from branch to branch. (I check the
> master branch file by hand. There is a single warning on Linux that
> is actually a sparse problem).
>
> Just FYI, for today's fetch:
>
>     $ diff sp-out nsp-out
>     $ diff nsp-out psp-out
>     12a13
>     >     SP blame.c
>     42a44,46
>     > diff.c:813:6: warning: symbol 'emit_line' was not declared. Should it be static?
>     > diff.c:828:6: warning: symbol 'emit_line_fmt' was not declared. Should it be static?
>     > diff.c:1865:6: warning: symbol 'print_stat_summary_0' was not declared. Should it be static?
>     54a59
>     >     SP fsmonitor.c
>     137a143
>     >     SP sub-process.c
>     170a177
>     >     SP compat/fopen.c
>     276a284
>     > builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer
>     296a305
>     >     SP t/helper/test-dir-iterator.c
>     $ 

Interesting.  One thing that I found somewhat suboptimal is that we
do not get signalled by non-zero exit.  Otherwise it would make a
good addition to the "Static Analysis" task in .travis.yml file.

Re: [PATCH] usage: add NORETURN to BUG() function definitions

[Ramsay Jones]

On 23/05/17 04:32, Junio C Hamano wrote:
> Interesting.  One thing that I found somewhat suboptimal is that we
> do not get signalled by non-zero exit.

Warnings don't lead to non-zero exit, but similarly to -Werror, you can
provide a -Wsparse-error to turn warnings into errors:

  $ make builtin/worktree.sp
      SP builtin/worktree.c
  builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer
  $ 
  $ make SPARSE_FLAGS=-Wsparse-error builtin/worktree.sp
      SP builtin/worktree.c
  builtin/worktree.c:539:38: error: Using plain integer as NULL pointer
  Makefile:2370: recipe for target 'builtin/worktree.sp' failed
  make: *** [builtin/worktree.sp] Error 1
  $ 

Unfortunately, that does not help too much because, as I mentioned before,
one warning is actually a sparse problem (and you can't turn it off):

  $ make pack-revindex.sp
      SP pack-revindex.c
  pack-revindex.c:64:23: warning: memset with byte count of 262144
  $ 

This is caused by sparse _unconditionally_ complaining about the byte count
used in calls to memset(), memcpy(), copy_to_user() and copy_from_user().
In addition, the byte count limits are hard-coded (v <= 0 || v > 100000).
About a decade ago, I wrote a patch to enable/set the limit value from the
command line, but didn't get around to sending the patch upstream. :-D
   
[There is actually another problem warning, if you build with NO_REGEX=1].

Since cgcc was intended to be used as proxy for gcc, you might think you
could use CC=cgcc on a regular build, but that has problems of it's own:

  $ make clean >/dev/null 2>&1  # on 'pu' branch, build output in 'pout'
  $ make CC=cgcc >pout1 2>&1
  $ diff pout pout1
  99a100
  > pack-revindex.c:64:23: warning: memset with byte count of 262144
  199a201
  > imap-send.c:1439:9: warning: expression using sizeof on a function
  200a203,207
  > http.c:675:9: warning: expression using sizeof on a function
  > http.c:1676:25: warning: expression using sizeof on a function
  > http.c:1681:25: warning: expression using sizeof on a function
  > http.c:2082:9: warning: expression using sizeof on a function
  > http.c:2249:9: warning: expression using sizeof on a function
  219a227
  > http-walker.c:377:9: warning: expression using sizeof on a function
  222a231,233
  > http-push.c:189:9: warning: expression using sizeof on a function
  > http-push.c:200:9: warning: expression using sizeof on a function
  > http-push.c:202:9: warning: expression using sizeof on a function
  228a240,243
  > remote-curl.c:524:9: warning: expression using sizeof on a function
  > remote-curl.c:605:17: warning: expression using sizeof on a function
  > remote-curl.c:608:17: warning: expression using sizeof on a function
  > remote-curl.c:676:9: warning: expression using sizeof on a function
  374a390
  > builtin/worktree.c:539:38: warning: Using plain integer as NULL pointer
  
  ...

  $ 

See commit 9371322a60 (sparse: suppress some "using sizeof on a function"
warnings, 06-10-2013) for an explanation of the additional warnings.
I chose the SPARSE_FLAGS method to suppress those warnings, precisely
because I don't build git that way. (git grep -n SPARSE_FLAGS).

So, using CC='cgcc -Wsparse-error' as it stands isn't much help:
  
  $ make clean >/dev/null 2>&1
  $ make CC='cgcc -Wsparse-error'
  GIT_VERSION = 2.13.0.530.g896b4ae59
      * new build flags
      CC credential-store.o
      * new link flags
      CC common-main.o
 
  ...
 
      CC pack-objects.o
      CC pack-revindex.o
  pack-revindex.c:64:23: error: memset with byte count of 262144
  Makefile:2036: recipe for target 'pack-revindex.o' failed
  make: *** [pack-revindex.o] Error 1
  $ 

>                                         Otherwise it would make a
> good addition to the "Static Analysis" task in .travis.yml file.

Unfortunately, some additional work required. :-P

ATB,
Ramsay Jones

Re: [PATCH] usage: add NORETURN to BUG() function definitions

[Jeff King]

On Sun, May 21, 2017 at 11:25:39PM +0100, Ramsay Jones wrote:

> Commit d8193743e0 ("usage.c: add BUG() function", 12-05-2017) added the
> BUG() functions and macros as a replacement for calls to die("BUG: ..").
> The use of NORETURN on the declarations (in git-compat-util.h) and the
> lack of NORETURN on the function definitions, however, leads sparse to
> complain thus:
> 
>       SP usage.c
>   usage.c:220:6: error: symbol 'BUG_fl' redeclared with different type
>   (originally declared at git-compat-util.h:1074) - different modifiers
> 
> In order to suppress the sparse error, add the NORETURN to the function
> definitions.
> 
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>

Thanks, I never ended up re-rolling the original, but from our previous
discussion this is obviously fine by me.

-Peff