threads / patch / 46018

patchusage: add NORETURN to BUG() function definitions

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

## tl;dr

9 messages between May 21, 2017 and May 23, 2017. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Ramsay Jones· May 21, 2017, 22:25 UTC · lore

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(-)
Show changes to usage.c +2 −2
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
Junio C Hamano· May 22, 2017, 01:43 UTC · re: Ramsay Jones · lore

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

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
Show 20 quoted lines
> 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.

Show 31 quoted lines
>
> 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);
Ramsay Jones· May 22, 2017, 02:13 UTC · re: Junio C Hamano · lore

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

On 22/05/17 02:43, Junio C Hamano wrote:
Show 27 quoted lines
> 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

Junio C Hamano· May 22, 2017, 02:35 UTC · re: Ramsay Jones · lore

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

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
Show 9 quoted lines
> 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.

Junio C Hamano· May 22, 2017, 02:46 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> 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.
Ramsay Jones· May 22, 2017, 14:02 UTC · re: Junio C Hamano · lore

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

On 22/05/17 03:46, Junio C Hamano wrote:
Show 11 quoted lines
> 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

Junio C Hamano· May 23, 2017, 03:32 UTC · re: Ramsay Jones · lore

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

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
Show 31 quoted lines
> 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.

Ramsay Jones· May 23, 2017, 20:47 UTC · re: Junio C Hamano · lore

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

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

Jeff King· May 22, 2017, 11:19 UTC · re: Ramsay Jones · lore

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

On Sun, May 21, 2017 at 11:25:39PM +0100, Ramsay Jones wrote:
Show 14 quoted lines
> 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

← back to recent threads