RFC patchgit-compat-util.h: Don't define NORETURN under __clang__

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(-)

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

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

+#elif defined(__GNUC__)

instead to make this really equivalent in the GNUC case.

Michael

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

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.

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 <Matthieu.Moy@grenoble-inp.fr> writes:

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.

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:

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.

Looks like that needs a cast.

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

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__ ;)

It's a "warning which defaults to an error" you can pacify it by passing -Winvalid-noreturn or -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.