Re: [PATCH] config.mak.dev: specify -std=gnu99 for gcc/clang

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 9, 2021, 12:05 UTC
Message-ID: <211209.867dcekm9h.gmgdl@evledraar.gmail.com>
In-Reply-To: <YbEMnksMEuAz3Nt0@coredump.intra.peff.net>

On Wed, Dec 08 2021, Jeff King wrote:

Show 70 quoted lines

> On Tue, Dec 07, 2021 at 08:13:43PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >> error: ISO C99 doesn't support unnamed structs/unions [-Werror=pedantic]
>> >
>> > Hmm. It's interesting that the regular DEVELOPER=1 doesn't catch this.
>> > It's because we don't specify -std there, and newer gcc defaults to
>> > gnu17 (unnamed unions appeared in c11, I think). I wonder if it would be
>> > helpful to teach config.mak.dev to pass -std=c99.
>> 
>> FWIW, I use -std=gnu99 as our Makefile suggests.
>
> I think the patch below would have detected this both locally for
> Han-Wen, but also in C.
>
> -- >8 --
> Subject: [PATCH] config.mak.dev: specify -std=gnu99 for gcc/clang
>
> The point of DEVELOPER=1 is to turn up the warnings so we can catch
> portability or correctness mistakes at the compiler level. But since
> modern compilers tend to default to modern standards like gnu17, we
> might miss warnings about older standards, even though we expect Git to
> build with compilers that use them.
>
> So it's helpful for developer builds to set the -std argument to our
> lowest-common denominator. Traditionally this was c89, but since we're
> moving to assuming c99 in 7bc341e21b (git-compat-util: add a test
> balloon for C99 support, 2021-12-01) that seems like a good spot to
> land. And as explained in that commit, we want "gnu99" because we still
> want to take advantage of some extensions when they're available.
>
> The new argument kicks in only for clang and gcc (which we know to
> support "-std=" and "gnu" standards). And only for compiler versions
> which default to a newer standard. That will avoid accidentally
> silencing any build problems that non-developers would run into on older
> compilers that default to c89.
>
> My digging found that the default switched to gnu11 in gcc 5.1.0.
> Clang's documentation is less clear, but has done so since at least
> clang-7. So that's what I put in the conditional here. It's OK to err on
> the side of not-enabling this for older compilers. Most developers (as
> well as CI) are using much more recent versions, so any warnings will
> eventually surface.
>
> A concrete example is anonymous unions, which became legal in c11.
> Without this patch, "gcc -pedantic" will not complain about them, but
> will if we add in "-std=gnu99".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  config.mak.dev | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/config.mak.dev b/config.mak.dev
> index 7673fed114..d4afac6b51 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -19,6 +19,11 @@ endif
>  endif
>  endif
>  endif
> +
> +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang7,$(COMPILER_FEATURES))),)
> +DEVELOPER_CFLAGS += -std=gnu99
> +endif
> +
>  DEVELOPER_CFLAGS += -Wdeclaration-after-statement
>  DEVELOPER_CFLAGS += -Wformat-security
>  DEVELOPER_CFLAGS += -Wold-style-definition

This approach looks good & the rationale make sense.

I mentioned in [1] that this might be a bad idea because:

    And as you note it's not only that older or non-gcc non-clang compilers
    may not understand this at all, but are we getting worse behavior on
    modern versions of those two because they're forced into some 20 year
    old C99 standard mode, instead of the current preferred default?

But from some short testing of GCC it will generate the exact same <file>.s regardless of -std=* option, so I think this indeed only impacts the warnings we'll emit. So pinning this seems to categorically be a good thing.

A bad thing about this is that we'll explicitly avoid happy accidents where we start relying on some newer C standard, and discover N releases later that it was no big deal, and can thus just use that feature.

On the other hand having this means less back & forth churn of adding such a dependency only to find it breaks on one of our platforms etc. Overall I think this makes sense, just say'n.

I don't think this needs to change, but FWIW this would benefit from the same sort of "let's just compile it" step as [2]. I.e. I think you'll find that we could just check the exit code of:

    $(CC) -E -std=gnu99 - </dev/null

This works on GCC/Clang, and will die on xlc/suncc, and I assume any other compiler that doesn't grok this. But I think it's better to avoid a $(shell) here just for that, and any such change can wait until we have some proper "compile this once and cache it" probing for config.mak.dev.

1. https://lore.kernel.org/git/211116.86pmr0p82k.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/211118.86lf1m5h1y.gmgdl@evledraar.gmail.com/