[RFC/PATCH] git-compat-util.h: Don't define NORETURN under __clang__

[RFC/PATCH] git-compat-util.h: Don't define NORETURN under __clang__

[Ævar Arnfjörð Bjarmason]

clang version 1.0 on Debian testing x86_64 defines __GNUC__, but barfs
on `void __attribute__((__noreturn__))'. E.g.:

    usage.c:56:1: error: function declared 'noreturn' should not return [-Winvalid-noreturn]
    }
    ^
    1 diagnostic generated.
    make: *** [usage.o] Error 1

There it's dying on `void __attribute__((__noreturn__)) usagef(const
char *err, ...)' in usage.c, which doesn't return.

Change the header to define NORETURN to nothing under clang. This was
the default behavior for non-GNU and non-MSC compilers already. Having
NORETURN_PTR defined to the GNU C value has no effect on clang
however.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I have no experience with Clang so this may not be sane, but on my
system clang compiles with it and passes all tests. It still spews a
lot of warnings though:
    
    GITGUI_VERSION = 0.12.0.64.g89d61
        * new build flags or prefix
    config.c:297:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    connect.c:151:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    date.c:678:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    diff.c:429:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.

These are valid, as control may not return the advertised type if we
call die() inside them.

    log-tree.c:297:21: warning: field width should have type 'int', but argument has type 'unsigned int' [-Wformat]
                             "Subject: [%s %0*d/%d] ",
                                             ^
    1 diagnostic generated.
    notes.c:632:25: warning: field precision should have type 'int', but argument has type 'unsigned int' [-Wformat]
            strbuf_addf(buf, "%o %.*s%c", mode, path_len, path, '\0');
                                   ^            ~~~~~~~~
    1 diagnostic generated.

Should these (and some below) just cast to (int) ?

    object.c:44:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    parse-options.c:155:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    read-cache.c:1361:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    remote.c:658:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    revision.c:253:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    setup.c:79:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    sideband.c:97:25: warning: field precision should have type 'int', but argument has type 'unsigned int' [-Wformat]
                                            fprintf(stderr, "%.*s", brk + sf, b);
                                                               ^    ~~~~~~~~
    1 diagnostic generated.
    transport.c:1133:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    imap-send.c:548:27: warning: more data arguments than '%' conversions [-Wformat-extra-args]
                               cmd->tag, cmd->cmd, cmd->cb.dlen);
                                                   ^
    1 diagnostic generated.
    Writing perl.mak for Git
    GIT_VERSION = 1.7.2.1.7.gb44c1
    builtin/blame.c:1984:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    builtin/bundle.c:67:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    builtin/commit.c:832:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    builtin/fetch-pack.c:209:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    builtin/grep.c:704:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    builtin/help.c:58:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    builtin/pack-redundant.c:584:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    builtin/push.c:252:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    builtin/reflog.c:782:1: warning: control may reach end of non-void function [-Wreturn-type]
    }
    ^
    1 diagnostic generated.
    
 git-compat-util.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 02a73ee..c651cb7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -183,7 +183,10 @@ extern char *gitbasename(char *);
 #define is_dir_sep(c) ((c) == '/')
 #endif
 
-#ifdef __GNUC__
+#ifdef __clang__
+#define NORETURN
+#define NORETURN_PTR __attribute__((__noreturn__))
+#elif __GNUC__
 #define NORETURN __attribute__((__noreturn__))
 #define NORETURN_PTR __attribute__((__noreturn__))
 #elif defined(_MSC_VER)
-- 
1.7.1

Re: [RFC/PATCH] git-compat-util.h: Don't define NORETURN under __clang__

[Michael J Gruber]

Ævar Arnfjörð Bjarmason venit, vidit, dixit 03.08.2010 15:08:
> clang version 1.0 on Debian testing x86_64 defines __GNUC__, but barfs
> on `void __attribute__((__noreturn__))'. E.g.:
> 
>     usage.c:56:1: error: function declared 'noreturn' should not return [-Winvalid-noreturn]
>     }
>     ^
>     1 diagnostic generated.
>     make: *** [usage.o] Error 1
> 
> There it's dying on `void __attribute__((__noreturn__)) usagef(const
> char *err, ...)' in usage.c, which doesn't return.
> 
> Change the header to define NORETURN to nothing under clang. This was
> the default behavior for non-GNU and non-MSC compilers already. Having
> NORETURN_PTR defined to the GNU C value has no effect on clang
> however.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> I have no experience with Clang so this may not be sane, but on my
> system clang compiles with it and passes all tests. It still spews a
> lot of warnings though:
>     
>     GITGUI_VERSION = 0.12.0.64.g89d61
>         * new build flags or prefix
>     config.c:297:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     connect.c:151:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     date.c:678:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     diff.c:429:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
> 
> These are valid, as control may not return the advertised type if we
> call die() inside them.
> 
>     log-tree.c:297:21: warning: field width should have type 'int', but argument has type 'unsigned int' [-Wformat]
>                              "Subject: [%s %0*d/%d] ",
>                                              ^
>     1 diagnostic generated.
>     notes.c:632:25: warning: field precision should have type 'int', but argument has type 'unsigned int' [-Wformat]
>             strbuf_addf(buf, "%o %.*s%c", mode, path_len, path, '\0');
>                                    ^            ~~~~~~~~
>     1 diagnostic generated.
> 
> Should these (and some below) just cast to (int) ?
> 
>     object.c:44:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     parse-options.c:155:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     read-cache.c:1361:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     remote.c:658:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     revision.c:253:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     setup.c:79:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     sideband.c:97:25: warning: field precision should have type 'int', but argument has type 'unsigned int' [-Wformat]
>                                             fprintf(stderr, "%.*s", brk + sf, b);
>                                                                ^    ~~~~~~~~
>     1 diagnostic generated.
>     transport.c:1133:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     imap-send.c:548:27: warning: more data arguments than '%' conversions [-Wformat-extra-args]
>                                cmd->tag, cmd->cmd, cmd->cb.dlen);
>                                                    ^
>     1 diagnostic generated.
>     Writing perl.mak for Git
>     GIT_VERSION = 1.7.2.1.7.gb44c1
>     builtin/blame.c:1984:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     builtin/bundle.c:67:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     builtin/commit.c:832:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     builtin/fetch-pack.c:209:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     builtin/grep.c:704:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     builtin/help.c:58:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     builtin/pack-redundant.c:584:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     builtin/push.c:252:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     builtin/reflog.c:782:1: warning: control may reach end of non-void function [-Wreturn-type]
>     }
>     ^
>     1 diagnostic generated.
>     
>  git-compat-util.h |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 02a73ee..c651cb7 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -183,7 +183,10 @@ extern char *gitbasename(char *);
>  #define is_dir_sep(c) ((c) == '/')
>  #endif
>  
> -#ifdef __GNUC__
> +#ifdef __clang__
> +#define NORETURN
> +#define NORETURN_PTR __attribute__((__noreturn__))
> +#elif __GNUC__

__GNUC__ should be true if defined, but maybe you still want

+#elif defined(__GNUC__)

instead to make this really equivalent in the GNUC case.

>  #define NORETURN __attribute__((__noreturn__))
>  #define NORETURN_PTR __attribute__((__noreturn__))
>  #elif defined(_MSC_VER)

Michael

Re: [RFC/PATCH] git-compat-util.h: Don't define NORETURN under __clang__

[Matthieu Moy]

Ævar Arnfjörð Bjarmason venit, vidit, dixit 03.08.2010 15:08:

> clang version 1.0 on Debian testing x86_64 defines __GNUC__, but barfs
> on `void __attribute__((__noreturn__))'. E.g.:
> 
>     usage.c:56:1: error: function declared 'noreturn' should not return [-Winvalid-noreturn]
>     }
>     ^
>     1 diagnostic generated.
>     make: *** [usage.o] Error 1

It doesn't mean that it's not accepting __noreturn__, it means it was
not smart enough to check that the function do not return.

In my git, usage.c:56: leads me to this function:

void usagef(const char *err, ...)
{
	va_list params;

	va_start(params, err);
	usage_routine(err, params);
	va_end(params);
}

The absence of return comes from usage_routine, which is a pointer to
function, and it seems your version of clang doesn't handle
__noreturn__ pointers to functions properly.

On my box:

git$ make -B CC=clang V=yes usage.o                                                                                                        
clang -o usage.o -c   -g -Wall -Werror -I.  -DHAVE_PATHS_H -DSHA1_HEADER='<openssl/sha.h>'  -DNO_STRLCPY -DNO_MKSTEMPS  usage.c
git$ clang --version
clang version 1.1 (branches/release_27)
Target: i386-pc-linux-gnu
Thread model: posix

so, more recent clang do not seem to have this issue.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 02a73ee..c651cb7 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -183,7 +183,10 @@ extern char *gitbasename(char *);
>  #define is_dir_sep(c) ((c) == '/')
>  #endif
>  
> -#ifdef __GNUC__
> +#ifdef __clang__
> +#define NORETURN
> +#define NORETURN_PTR __attribute__((__noreturn__))
> +#elif __GNUC__

If you go for something like this, you should check the version of
clang, and special-case only version < 1.1.

But I'm not sure special-casing old version of a young compiler really
makes sense. We're only talking about warnings here, so I'd say you
should either upgrade clang or remove -Werror from your CFLAGS.

(other than that, it's cool to see someone testing another
compiler ;-) )

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] git-compat-util.h: Don't define NORETURN under __clang__

[Matthieu Moy]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> (other than that, it's cool to see someone testing another
> compiler ;-) )

BTW, the only warnings remaining with -Wall with my clang are:

imap-send.c:548:27: warning: data argument not used by format string [-Wformat-extra-args]
                           cmd->tag, cmd->cmd, cmd->cb.dlen);
                                               ^
imap-send.c:1089:41: warning: conversion specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
                snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
                                                    ~~^   ~~~~~~~~~~
2 diagnostics generated.

and the tests pass.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH] git-compat-util.h: Don't define NORETURN under __clang__

[Ævar Arnfjörð Bjarmason]

Thanks everyone. I should have tested a later version of clang before
I sent the patch. It might still be worthwhile to munge the flags for
old clangs so that git doesn't error out on it, but if 1.0 doesn't
make it into some major OS release and gcc remains the default it's
not much of an issue.

On Tue, Aug 3, 2010 at 14:23, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> (other than that, it's cool to see someone testing another
>> compiler ;-) )
>
> BTW, the only warnings remaining with -Wall with my clang are:
>
> imap-send.c:548:27: warning: data argument not used by format string [-Wformat-extra-args]
>                           cmd->tag, cmd->cmd, cmd->cb.dlen);
>                                               ^

I didn't look into that one. It'd be usefu to check out if the format
string is really incomplete there, or if clang is just failing it its
analysis.

That's some hairy code, in any case.

> imap-send.c:1089:41: warning: conversion specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
>                snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
>                                                    ~~^   ~~~~~~~~~~
> 2 diagnostics generated.

Looks like that needs a cast.

Re: [RFC/PATCH] git-compat-util.h: Don't define NORETURN under __clang__

[Benjamin Kramer]

On 03.08.2010, at 15:08, Ævar Arnfjörð Bjarmason wrote:

> clang version 1.0 on Debian testing x86_64 defines __GNUC__, but barfs
> on `void __attribute__((__noreturn__))'. E.g.:

This is a bug in clang 1.0 that was fixed in the llvm 2.7/clang 1.5 timeframe, it didn't
recognize attributes on function pointers properly. clang tries to be as compatible with GCC as
possible so it obviously has to define __GNUC__ ;)

> 
>    usage.c:56:1: error: function declared 'noreturn' should not return [-Winvalid-noreturn]
>    }
>    ^
>    1 diagnostic generated.
>    make: *** [usage.o] Error 1

It's a "warning which defaults to an error" you can pacify it by passing -Winvalid-noreturn or
\x10-Wno-invalid-noreturn.

I oppose adding workarounds for obsolete versions of clang, Debian should upgrade their packages to
a more recent release. clang 1.0 had all kinds of weird bugs and shouldn't be used anymore.