From: Junio C Hamano <hidden> Date: 2021-12-08 04:13:48
Jeff King [off-list ref] writes:
quoted
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.
On Wed, Dec 8, 2021 at 5:13 AM Junio C Hamano [off-list ref] wrote:
quoted
quoted
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 understand that the default build should be lenient rather than
strict for portability reasons. However, it would be good if the CI
was strict with this.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
From: Jeff King <hidden> Date: 2021-12-08 19:50:56
On Tue, Dec 07, 2021 at 08:13:43PM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
quoted
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 <redacted>
---
config.mak.dev | 5 +++++
1 file changed, 5 insertions(+)
On Tue, Dec 07, 2021 at 08:13:43PM -0800, Junio C Hamano wrote:
quoted
Jeff King [off-list ref] writes:
quoted
quoted
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 <redacted>
---
config.mak.dev | 5 +++++
1 file changed, 5 insertions(+)
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/
[...]
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.
Thanks for looking into that. The thought crossed my mind, too, but I
didn't have any actual data. I think some of this is due to the standard
committee keeping backwards compatibility. I.e., it's unlikely for there
to be a case where the N standard says "you must do X", and N+1 says
"that's dumb, you can do Y instead". Usually it is about new features
that were syntactically invalid or created undefined behavior in version
N.
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.
Right. I think it forces us to be more deliberate, but that's probably
OK.
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.
You'd lose one property that the version-check has, which is that we
don't ever _upgrade_ to gnu99 from gnu89. I'm not sure how important
that is, though. It would have been bad if we took something like
brian's c99 patch, and developers and CI both failed to realize that it
was breaking the out-of-the-box build for older compilers (because
behind the scenes they were getting -std=gnu99 that non-developers do
not). But now that we've decided on that direction anyway, I'm not sure
there's anything left to be learned.
-Peff