| Message ID | YbEMnksMEuAz3Nt0@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5f46385309b7a67d20a8471c27ccc1d5fdd09d53 |
| Headers | show |
| Series | config.mak.dev: specify -std=gnu99 for gcc/clang | expand |
On Wed, Dec 08 2021, Jeff King wrote: > 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/
On Thu, Dec 09, 2021 at 01:05:44PM +0100, Ævar Arnfjörð Bjarmason wrote: > > + > > +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang7,$(COMPILER_FEATURES))),) > > +DEVELOPER_CFLAGS += -std=gnu99 > > +endif > [...] > > 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
On 2022-01-13 at 10:45:49, Ævar Arnfjörð Bjarmason wrote: > [I'm aware that Jeff probably won't read this in his sabbatical, just > replying to the relevant thread] > > Something neither of us considered at the time, but which is pretty > obvious in retrospect, is that this new -std=gnu99 dosen't only apply to > our own code, but any system and library includes that we need. > > Thus e.g. FreeBSD 13.0 which previously was happy to compile under > DEVELOPER=1 will now die because its core libraries use C11-specific > code: > > archive.c:337:35: error: '_Generic' is a C11 extension [-Werror,-Wc11-extensions] > strbuf_addstr(&path_in_archive, basename(path)); > ^ > /usr/include/libgen.h:61:21: note: expanded from macro 'basename' > #define basename(x) __generic(x, const char *, __old_basename, basename)(x) > ^ > /usr/include/sys/cdefs.h:325:2: note: expanded from macro '__generic' > _Generic(expr, t: yes, default: no) > ^ I think we had this discussion about FreeBSD before and that's why I specifically dropped that option from the main makefile. We can either drop that patch, or we can set it to -std=gnu11 and tell folks setting DEVELOPER to use a system released in the last five years. I think we can be a little stricter with what we require in the case of DEVELOPER than we might be otherwise.
On Fri, Jan 14 2022, brian m. carlson wrote: > [[PGP Signed Part:Undecided]] > On 2022-01-13 at 10:45:49, Ævar Arnfjörð Bjarmason wrote: >> [I'm aware that Jeff probably won't read this in his sabbatical, just >> replying to the relevant thread] >> >> Something neither of us considered at the time, but which is pretty >> obvious in retrospect, is that this new -std=gnu99 dosen't only apply to >> our own code, but any system and library includes that we need. >> >> Thus e.g. FreeBSD 13.0 which previously was happy to compile under >> DEVELOPER=1 will now die because its core libraries use C11-specific >> code: >> >> archive.c:337:35: error: '_Generic' is a C11 extension [-Werror,-Wc11-extensions] >> strbuf_addstr(&path_in_archive, basename(path)); >> ^ >> /usr/include/libgen.h:61:21: note: expanded from macro 'basename' >> #define basename(x) __generic(x, const char *, __old_basename, basename)(x) >> ^ >> /usr/include/sys/cdefs.h:325:2: note: expanded from macro '__generic' >> _Generic(expr, t: yes, default: no) >> ^ > > I think we had this discussion about FreeBSD before and that's why I > specifically dropped that option from the main makefile. We can either > drop that patch, or we can set it to -std=gnu11 and tell folks setting > DEVELOPER to use a system released in the last five years. I think we > can be a little stricter with what we require in the case of DEVELOPER > than we might be otherwise. I guess, yeah. As a practical matter the changes in this cycle have made DEVELOPER=1 much more fragile as a cross-platform facility. I test on various platforms/OS's (across the GCC farm), and before this cycle only AIX and Solaris were something I had to pay special attention to. Now we've got things like this breaking (by default) non-obscure things like FreeBSD. Of course I can manually set -Wall etc. which is what I previously got out of this, but before e.g. pedantic was opt-in, now an effective C-version-a-pedantic is the default, and doesn't have a tweaking knob at all. Anyway, we'll see how much of a hassle it is, and it's not too painful for me right now, so I don't think anything needs to be done in the RC period. But I wonder if the X-Y problem we're trying to solve is making sure we don't move past C99 unintentionally whether this wouldn't be better solved by dropping this -std=gnu99 approach in the Makefile, and instead just do that in one of the CI jobs (whose OS includes would be known to be C99-OK).
"brian m. carlson" <sandals@crustytoothpaste.net> writes: >> DEVELOPER=1 will now die because its core libraries use C11-specific >> code: >> >> archive.c:337:35: error: '_Generic' is a C11 extension [-Werror,-Wc11-extensions] >> strbuf_addstr(&path_in_archive, basename(path)); >> ^ >> /usr/include/libgen.h:61:21: note: expanded from macro 'basename' >> #define basename(x) __generic(x, const char *, __old_basename, basename)(x) >> ^ >> /usr/include/sys/cdefs.h:325:2: note: expanded from macro '__generic' >> _Generic(expr, t: yes, default: no) >> ^ Wow, that sounds horribly broken. > I think we had this discussion about FreeBSD before and that's why I > specifically dropped that option from the main makefile. We can either > drop that patch, or we can set it to -std=gnu11 and tell folks setting > DEVELOPER to use a system released in the last five years. I think we > can be a little stricter with what we require in the case of DEVELOPER > than we might be otherwise. But that is not being stricter, but looser, no? I thought that the point of -std=gnu99 was to allow us to use C99 features while catching use of language features newer than that, and use of -std=gnu11 will defeat half the point, wouldn't it?
On Fri, Jan 14 2022, Junio C Hamano wrote: > "brian m. carlson" <sandals@crustytoothpaste.net> writes: > >>> DEVELOPER=1 will now die because its core libraries use C11-specific >>> code: >>> >>> archive.c:337:35: error: '_Generic' is a C11 extension [-Werror,-Wc11-extensions] >>> strbuf_addstr(&path_in_archive, basename(path)); >>> ^ >>> /usr/include/libgen.h:61:21: note: expanded from macro 'basename' >>> #define basename(x) __generic(x, const char *, __old_basename, basename)(x) >>> ^ >>> /usr/include/sys/cdefs.h:325:2: note: expanded from macro '__generic' >>> _Generic(expr, t: yes, default: no) >>> ^ > > Wow, that sounds horribly broken. Yes, but it's also working as designed :) We're erroring because the C library headers on the OS aren't C99-compliant. That it would apply to only git.git's sources was only ever wishful thinking. >> I think we had this discussion about FreeBSD before and that's why I >> specifically dropped that option from the main makefile. We can either >> drop that patch, or we can set it to -std=gnu11 and tell folks setting >> DEVELOPER to use a system released in the last five years. I think we >> can be a little stricter with what we require in the case of DEVELOPER >> than we might be otherwise. > > But that is not being stricter, but looser, no? I thought that the > point of -std=gnu99 was to allow us to use C99 features while catching > use of language features newer than that, and use of -std=gnu11 will > defeat half the point, wouldn't it? Indeed. But also as noted in my side-thread reply in <220114.86a6fytte9.gmgdl@evledraar.gmail.com> I think that being able to use it like this was always wishful thinking. I.e. to brian's "tell folks setting DEVELOPER to use a system released in the last five years". This is the exact opposite of that. We are implicitly expecting that the OS will forever be using the 20+-year old C99 standard, and not trying out fetures in C11 standardized 10+ years ago. The FreeBSD 13.0 release is less than a year old[1]. When I went looking I found that the change to use __generic in its libgen.h just so happens to have been made in FreeBSD sources around 5 years ago: [2]. 1. https://www.freebsd.org/releases/13.0R/announce/ 2. https://github.com/freebsd/freebsd-src/commit/34168b28e99b0bb328c7bb49cd91cb942181056f
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> Wow, that sounds horribly broken. > > Yes, but it's also working as designed :) We're erroring because the C > library headers on the OS aren't C99-compliant. That it would apply to > only git.git's sources was only ever wishful thinking. No, C library supporting only C11 is perfectly fine. On such a system, the compiler shouldn't even support -std=gnu99. That is what I consider broken.
Junio C Hamano <gitster@pobox.com> writes: >> I think we had this discussion about FreeBSD before and that's why I >> specifically dropped that option from the main makefile. We can either >> drop that patch, or we can set it to -std=gnu11 and tell folks setting >> DEVELOPER to use a system released in the last five years. I think we >> can be a little stricter with what we require in the case of DEVELOPER >> than we might be otherwise. > > But that is not being stricter, but looser, no? I thought that the > point of -std=gnu99 was to allow us to use C99 features while catching > use of language features newer than that, and use of -std=gnu11 will > defeat half the point, wouldn't it? If FreeBSD (or any other platform) cannot do "reject features beyond C99", I am perfectly OK to drop -std=gnu99 on such a place. DEVELOPER=YesPlease ought to be a collection of settings that helps the developers the most. So on platforms that *can* do "reject features beyond C99", I prefer to have it enabled when DEVELOPER=YesPlease is given. It seems that -std=gnu99 is only added conditionally even in today's config.mak.dev, so it is fine if we dropped -std=gnu99 from there. Which means that developers on FreeBSD cannot participate in vetting use of features beyond C99, but there are developers on other platforms who will, so it's not too bad, I would say. As config.mak.dev is included after config.mak.uname, something like this may be sufficient, perhaps? config.mak.dev | 5 +++++ 1 file changed, 5 insertions(+) diff --git i/config.mak.dev w/config.mak.dev index d4afac6b51..3deb076d5e 100644 --- i/config.mak.dev +++ w/config.mak.dev @@ -20,9 +20,14 @@ endif endif endif +ifneq ($(uname_S),FreeBSD) ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang7,$(COMPILER_FEATURES))),) DEVELOPER_CFLAGS += -std=gnu99 endif +else +# FreeBSD cannot limit to C99 because its system headers unconditionally +# rely on C11 features. +endif DEVELOPER_CFLAGS += -Wdeclaration-after-statement DEVELOPER_CFLAGS += -Wformat-security
On Fri, Jan 14 2022, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >>> I think we had this discussion about FreeBSD before and that's why I >>> specifically dropped that option from the main makefile. We can either >>> drop that patch, or we can set it to -std=gnu11 and tell folks setting >>> DEVELOPER to use a system released in the last five years. I think we >>> can be a little stricter with what we require in the case of DEVELOPER >>> than we might be otherwise. >> >> But that is not being stricter, but looser, no? I thought that the >> point of -std=gnu99 was to allow us to use C99 features while catching >> use of language features newer than that, and use of -std=gnu11 will >> defeat half the point, wouldn't it? > > If FreeBSD (or any other platform) cannot do "reject features beyond > C99", I am perfectly OK to drop -std=gnu99 on such a place. > > DEVELOPER=YesPlease ought to be a collection of settings that helps > the developers the most. So on platforms that *can* do "reject > features beyond C99", I prefer to have it enabled when > DEVELOPER=YesPlease is given. > > It seems that -std=gnu99 is only added conditionally even in today's > config.mak.dev, so it is fine if we dropped -std=gnu99 from there. > Which means that developers on FreeBSD cannot participate in vetting > use of features beyond C99, but there are developers on other > platforms who will, so it's not too bad, I would say. > > As config.mak.dev is included after config.mak.uname, something like > this may be sufficient, perhaps? Wasn't the initial goal here to check whether we'd accidentally include C99? Just checking on GCC/Clang somewhere seems sufficient enough, I.e. something like: diff --git a/ci/lib.sh b/ci/lib.sh index 9d28ab50fb4..94d0d4127b9 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -209,6 +209,9 @@ linux-leaks) export SANITIZE=leak export GIT_TEST_PASSING_SANITIZE_LEAK=true ;; +linux-gcc|linux-clang) + MAKEFLAGS="$MAKEFLAGS CFLAGS=-std=gnu99" + ;; esac MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}" diff --git a/config.mak.dev b/config.mak.dev index d4afac6b51f..216f92493fc 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -20,10 +20,6 @@ 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 We're just starting to get into the whack-a-mole of hardcoding OS's and compiler versions, it's not all versions of FreeBSD, probably not just that OS etc. I've also wondered why we can't just ship our own super-light version of autoconf to run some real probes, and as a result eventually get rid of configure.ac, ./detect-compiler, the version hardcoding in config.mak.dev etc. Just something as simple as extending this: diff --git a/Makefile b/Makefile index 5580859afdb..f197a07c100 100644 --- a/Makefile +++ b/Makefile @@ -2557,6 +2557,15 @@ else $(OBJECTS): $(LIB_H) $(GENERATED_H) endif +ifdef DEVELOPER +probe/std.mak: + @mkdir -p $(@D) + $(QUIET_GEN)if $(CC) -E -std=gnu99 - </dev/null >/dev/null; then \ + echo 'DEVELOPER_CFLAGS += -std=gnu99'; \ + fi >$@ +include probe/std.mak +endif + ifeq ($(GENERATE_COMPILATION_DATABASE),yes) all:: compile_commands.json compile_commands.json:
Junio C Hamano <gitster@pobox.com> writes: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> Wow, that sounds horribly broken. >> >> Yes, but it's also working as designed :) We're erroring because the C >> library headers on the OS aren't C99-compliant. That it would apply to >> only git.git's sources was only ever wishful thinking. > > No, C library supporting only C11 is perfectly fine. On such a > system, the compiler shouldn't even support -std=gnu99. That is > what I consider broken. Or, the system headers should be arranged in such a way that depending on __STDC_VERSION__, it should refrain from using features of the language that is not supported. So supporting -std=gnu99 in their compilers may not be a bug---but in that case, their system headers are buggy. Anyway. I think <xmqqzgny7xo7.fsf@gitster.g> shows a viable way forward.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Wasn't the initial goal here to check whether we'd accidentally include > C99? Just checking on GCC/Clang somewhere seems sufficient enough, What do you mean by "accidentally include"? The goal here is that developer build should give developer a set of options that help the most---"not going beyond C99" is something we want them to be checking, if able (and we have established that users on FreeBSD are not capable). Doesn't your "something like" limits the check to CI? Developers with compilers that _can_ help ensuring that we do not go beyond C99 should be able to do so by their local developer build. > I.e. something like: > > diff --git a/ci/lib.sh b/ci/lib.sh > index 9d28ab50fb4..94d0d4127b9 100755 > --- a/ci/lib.sh > +++ b/ci/lib.sh > @@ -209,6 +209,9 @@ linux-leaks) > export SANITIZE=leak > export GIT_TEST_PASSING_SANITIZE_LEAK=true > ;; > +linux-gcc|linux-clang) > + MAKEFLAGS="$MAKEFLAGS CFLAGS=-std=gnu99" > + ;; > esac > > MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}" > diff --git a/config.mak.dev b/config.mak.dev > index d4afac6b51f..216f92493fc 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -20,10 +20,6 @@ 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
On Fri, Jan 14 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Wasn't the initial goal here to check whether we'd accidentally include >> C99? Just checking on GCC/Clang somewhere seems sufficient enough, > > What do you mean by "accidentally include"? > > The goal here is that developer build should give developer a set of > options that help the most---"not going beyond C99" is something we > want them to be checking, if able (and we have established that > users on FreeBSD are not capable). The reason we ended up with -std=gnu99 is because of this thread where Han-Wen accidentally used an unnamed structs/unions: https://lore.kernel.org/git/xmqqlf0w5bbc.fsf@gitster.g/ He then pointed out that it would be nice if CI caught this, which would have been sufficient: https://lore.kernel.org/git/CAFQ2z_NuOy+-pfSoNAYjJhS9jZCYOfoFue10=k=iyPVsPYrB3g@mail.gmail.com/ > Doesn't your "something like" limits the check to CI? Developers > with compilers that _can_ help ensuring that we do not go beyond C99 > should be able to do so by their local developer build. Yes, I'm suggesting that what we're trying to catch here is rare enough that that may be sufficient, and which would sidestep the issue of wasting time on trying to make this portable everywhere. The DEVELOPER=1 feature has recently gone from something I used on a wide varity on of platforms and the worst I got was notices about unknown -W flags in a couple of them, to something where recently due to -fno-common and now more widely with -std=gnu99 I've ended up having to manually blacklist it on a few boxes where it Just Worked. So maybe you think we absolutely need to spend the effort to run less portable checks like that locally if at all possible, with all the config.mak.uname etc. work that entails, if so OK. I'm just suggesting that perhaps a more practical solution is to side-step those portability issues by checking this in CI, which I think would be Good Enough on this case.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I'm just suggesting that perhaps a more practical solution is to > side-step those portability issues by checking this in CI, which I think > would be Good Enough on this case. Use of GitHub PR + GGG is required to take advantage of that, no? I do not think I want to see a change that makes it _more_ likely that a patch I pick up from the list will have problems that my local integration work will not catch (because DEVELOPER=YesPlease build is castrated at the original contributor side, and also on my box) and will only be caught after I push out 'seen'.
Hi Junio, On Fri, 14 Jan 2022, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > >> I think we had this discussion about FreeBSD before and that's why I > >> specifically dropped that option from the main makefile. We can either > >> drop that patch, or we can set it to -std=gnu11 and tell folks setting > >> DEVELOPER to use a system released in the last five years. I think we > >> can be a little stricter with what we require in the case of DEVELOPER > >> than we might be otherwise. > > > > But that is not being stricter, but looser, no? I thought that the > > point of -std=gnu99 was to allow us to use C99 features while catching > > use of language features newer than that, and use of -std=gnu11 will > > defeat half the point, wouldn't it? > > If FreeBSD (or any other platform) cannot do "reject features beyond > C99", I am perfectly OK to drop -std=gnu99 on such a place. > > DEVELOPER=YesPlease ought to be a collection of settings that helps > the developers the most. So on platforms that *can* do "reject > features beyond C99", I prefer to have it enabled when > DEVELOPER=YesPlease is given. > > It seems that -std=gnu99 is only added conditionally even in today's > config.mak.dev, so it is fine if we dropped -std=gnu99 from there. > Which means that developers on FreeBSD cannot participate in vetting > use of features beyond C99, but there are developers on other > platforms who will, so it's not too bad, I would say. Plus, we have CI runs that do help us, thanks to setting `DEVELOPER=1` (see https://github.com/git/git/blob/v2.35.0-rc1/ci/lib.sh#L154). > As config.mak.dev is included after config.mak.uname, something like > this may be sufficient, perhaps? > > config.mak.dev | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git i/config.mak.dev w/config.mak.dev > index d4afac6b51..3deb076d5e 100644 > --- i/config.mak.dev > +++ w/config.mak.dev > @@ -20,9 +20,14 @@ endif > endif > endif > > +ifneq ($(uname_S),FreeBSD) > ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang7,$(COMPILER_FEATURES))),) > DEVELOPER_CFLAGS += -std=gnu99 > endif > +else > +# FreeBSD cannot limit to C99 because its system headers unconditionally > +# rely on C11 features. > +endif > > DEVELOPER_CFLAGS += -Wdeclaration-after-statement > DEVELOPER_CFLAGS += -Wformat-security > I applied this patch on top of the current tip of `seen`, opened a PR at https://github.com/gitgitgadget/git/pull/1116. The corresponding FreeBSD run is here: https://cirrus-ci.com/task/5867558132776960, and it succeeded. In addition, I concur that this is the best we can do, and I really would like to have that patch as soon as possible because it confuses new contributors when their PR builds fail out of no mistake of their own. Please count this as a vote for including this patch in -rc2. Thank you, Dscho
On Tue, Jan 18 2022, Johannes Schindelin wrote: > Hi Junio, > > On Fri, 14 Jan 2022, Junio C Hamano wrote: > >> Junio C Hamano <gitster@pobox.com> writes: >> >> >> I think we had this discussion about FreeBSD before and that's why I >> >> specifically dropped that option from the main makefile. We can either >> >> drop that patch, or we can set it to -std=gnu11 and tell folks setting >> >> DEVELOPER to use a system released in the last five years. I think we >> >> can be a little stricter with what we require in the case of DEVELOPER >> >> than we might be otherwise. >> > >> > But that is not being stricter, but looser, no? I thought that the >> > point of -std=gnu99 was to allow us to use C99 features while catching >> > use of language features newer than that, and use of -std=gnu11 will >> > defeat half the point, wouldn't it? >> >> If FreeBSD (or any other platform) cannot do "reject features beyond >> C99", I am perfectly OK to drop -std=gnu99 on such a place. >> >> DEVELOPER=YesPlease ought to be a collection of settings that helps >> the developers the most. So on platforms that *can* do "reject >> features beyond C99", I prefer to have it enabled when >> DEVELOPER=YesPlease is given. >> >> It seems that -std=gnu99 is only added conditionally even in today's >> config.mak.dev, so it is fine if we dropped -std=gnu99 from there. >> Which means that developers on FreeBSD cannot participate in vetting >> use of features beyond C99, but there are developers on other >> platforms who will, so it's not too bad, I would say. > > Plus, we have CI runs that do help us, thanks to setting `DEVELOPER=1` > (see https://github.com/git/git/blob/v2.35.0-rc1/ci/lib.sh#L154). > >> As config.mak.dev is included after config.mak.uname, something like >> this may be sufficient, perhaps? >> >> config.mak.dev | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git i/config.mak.dev w/config.mak.dev >> index d4afac6b51..3deb076d5e 100644 >> --- i/config.mak.dev >> +++ w/config.mak.dev >> @@ -20,9 +20,14 @@ endif >> endif >> endif >> >> +ifneq ($(uname_S),FreeBSD) >> ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang7,$(COMPILER_FEATURES))),) >> DEVELOPER_CFLAGS += -std=gnu99 >> endif >> +else >> +# FreeBSD cannot limit to C99 because its system headers unconditionally >> +# rely on C11 features. >> +endif >> >> DEVELOPER_CFLAGS += -Wdeclaration-after-statement >> DEVELOPER_CFLAGS += -Wformat-security >> > > I applied this patch on top of the current tip of `seen`, opened a PR at > https://github.com/gitgitgadget/git/pull/1116. The corresponding FreeBSD > run is here: https://cirrus-ci.com/task/5867558132776960, and it > succeeded. > > In addition, I concur that this is the best we can do, and I really would > like to have that patch as soon as possible because it confuses new > contributors when their PR builds fail out of no mistake of their own. I just submitted a more narrow fix in the side-thread[1]. I've tested it on the FreeBSD 13.0 box I have access to, but perhaps you'd like to test it too? > Please count this as a vote for including this patch in -rc2. I'd like to have it too, but for context needing to add NO_UNCOMPRESS2=Y (which Junio's punted on for the final[2]) is a much more widespread issue of needing new post-install build tweaking than this issue that only affects developer builds on FreeBSD. What makes that FreeBSD run a part of the GGG CI? Another and more narrow workaround for that in particular is to have whatever's driving it add -Wno-c11-extensions to the CFLAGS. 1. https://lore.kernel.org/git/patch-1.1-06cc12be94d-20220118T151234Z-avarab@gmail.com/ 2. https://lore.kernel.org/git/patch-1.1-9cea01a1395-20220117T170457Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I'd like to have it too, but for context needing to add NO_UNCOMPRESS2=Y > (which Junio's punted on for the final[2]) is a much more widespread > issue of needing new post-install build tweaking than this issue that > only affects developer builds on FreeBSD. I hate it when people misrepresent what I said, even in an attempt to spite me. For the "check ZLIB_VERSION_NUM" topic, I gave you specific things that needs to be different from the version posted with reasons, fully expecting that a better version of the patch to materialize soon (knowing how proliferate you can be when you want to) to give us enough time to assess the potential damage. I wouldn't call that "punted". For this one, config.mak.dev patch WOULD only affect developer builds, which is a much better solution than overriding what their system headers want to do and force compiling with C99 mode. With the status quo with today's code, with or without the patch Dscho wants in this thread, means ANYBODY will be stopped when they attempt to build with -std=gnu99 on FreeBSD, which is a GOOD thing. The reason why it is a much better solution to PUNT on using C99 mode on FreeBSD is because this episode makes it very clear that nobody tested building anything that use basename(), dirname(), etc. with C99 mode on the platform. I do not trust such a build, even if we could work around the system header breakage. This time we got lucky and wereq stopped by a compilation error, but I have a strong suspicion that C99-only mode of compiler on this platform is not as well battle tested as their standard mode of compilation that allows C11. I simply do not think we want to waste developer's time with C99-only mode on this platform which may end up debugging the platform and not our program. Next bug that will hit us may not be so friendly to clearly break compilation. Instead, the symptom may be a subtle runtime breakage that wastes people's time. After developers who work on FreeBSD (not Git developers who uses FreeBSD) ships an updated system headers so that more programs, not just us, are built and testsd with C99-only mode, perhaps it becomes usable as a platform to catch use of language features beyond C99, like everybody else. But with such a clearly broken system headers, I do not think today's FreeBSD is ready for that. I do trust their default settings that allows C11, a lot more than C99-only compilation with the "their libgen is broken, so here is a user side workaround" patch.
On Tue, Jan 18 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> I'd like to have it too, but for context needing to add NO_UNCOMPRESS2=Y >> (which Junio's punted on for the final[2]) is a much more widespread >> issue of needing new post-install build tweaking than this issue that >> only affects developer builds on FreeBSD. > > I hate it when people misrepresent what I said, even in an attempt > to spite me. Let's assume some mutual good faith here. I'm just trying to help debug last-minute issues in this release to help get it out the door. If I've misrepresented your views that's clearly true if you're the one claiming it, but it's not intentional. I was trying to read the tea leaves here in terms of helping patch triage along. My assessment of that NO_UNCOMPRESS2=Y issue from the boxes I've tested on is that it's a much more widespread problem new in this release (this is purely from testing on the GCC farm, however representative that is). It'll impact multiple OS's, linux distros & versions. Whereas the C11 warning is "just" recent FreeBSD && DEVELOPER=1. So I assumed if you weren't interested in the former before the final you probably wouldn't be in the latter, but wanted to provide a more narrow fix in case you were. > For the "check ZLIB_VERSION_NUM" topic, I gave you > specific things that needs to be different from the version posted > with reasons, fully expecting that a better version of the patch to > materialize soon (knowing how proliferate you can be when you want > to) to give us enough time to assess the potential damage. I can re-roll it sooner than later if you'd like, but figured per your "I like the basic idea of the patch, but I am afraid that it is way too late in the cycle"[1] that the resulting on-list distraction wouldn't be worth it at this point, if it wasn't being considered for being applied in the release window. Aside from "I can re-roll it". I also think that the point of making that change mostly (but not entirely) disappears if it isn't included in v2.35.0. I.e. the point of doing it is to avoid the one-time pain of anyone building new releases of git on $OLD_OS/$OLD_DISTRO not having to run into the compilation error that's fixed with NO_UNCOMPRESS2=Y. So if we put out a release without it anyone who's compiling git releases will need to adjust their build system for that anyway (or they're using autoconf, where it'll Just Work). If we then get this into v2.36.0 there'll be someone somewhere that benefits, but I'd think the ship has sailed for most of those who'd avoid the needless flag twiddling (git-packagers@ et al). So I don't know if it's even worth it to pursue that patch in that case... > I wouldn't call that "punted". I don't think I've used or heard that word outside of this ML. FWIW I'm using it in the sense of "kicking the can down the road" or "deferred it". Maybe that's incorrect, and I certainly stand corrected if I implied something that wasn't true about our views by using that word. > For this one, config.mak.dev patch WOULD only affect developer > builds, which is a much better solution than overriding what their > system headers want to do and force compiling with C99 mode. With > the status quo with today's code, with or without the patch Dscho > wants in this thread, means ANYBODY will be stopped when they > attempt to build with -std=gnu99 on FreeBSD, which is a GOOD thing. > > The reason why it is a much better solution to PUNT on using C99 > mode on FreeBSD is because this episode makes it very clear that > nobody tested building anything that use basename(), dirname(), > etc. with C99 mode on the platform. I do not trust such a build, > even if we could work around the system header breakage. Aside from anything else I think you're assuming a lot about warning hygiene in a typical C project. Git's use of even optional -Werror is fairly unusual. If you Google search that error you'll find numerous past reports of it, it just hasn't been solved. I also think we discussed around the -std=gnu99 change that the "C99 mode" in compilers we tested isn't impacting generated code (although of course it will if it's conditional on __STDC_VERSION___). Otherwise the generated objects are the same, it's just what warnings/errors you get that changes. In this case the fallback case already existed without my __generic hack, so forcing that codepath for libgen.h isn't new. > This time we got lucky and wereq stopped by a compilation error, but > I have a strong suspicion that C99-only mode of compiler on this > platform is not as well battle tested as their standard mode of > compilation that allows C11. I simply do not think we want to waste > developer's time with C99-only mode on this platform which may end > up debugging the platform and not our program. Next bug that will > hit us may not be so friendly to clearly break compilation. > Instead, the symptom may be a subtle runtime breakage that wastes > people's time. I'd share that opinion if this was aCC on HP/UX or xlc on AIX or whatever. But in this case it's just vanilla clang, not some scary platform-specific compiler. Its "C99-only mode" isn't anything different on FreeBSD than Linux. We're just talking about one specific and avoidable warning in /usr/include/libgen.h on that OS, once we're past that its "C99-mode" works just fine. > After developers who work on FreeBSD (not Git developers who uses > FreeBSD) ships an updated system headers so that more programs, not > just us, are built and testsd with C99-only mode, perhaps it becomes > usable as a platform to catch use of language features beyond C99, > like everybody else. But with such a clearly broken system headers, > I do not think today's FreeBSD is ready for that. I do trust their > default settings that allows C11, a lot more than C99-only > compilation with the "their libgen is broken, so here is a user side > workaround" patch. Fair enough. I'm happy for us to get past this one way or the other, whether it's with your patch or mine. I took your previous reservations about my suggestions to just do this C99 checking in CI-only to mean that you'd like to keep -std=gnu99 on DEVELOPER=1 if at all possible, which my approach does. 1. https://lore.kernel.org/git/xmqq7dayw732.fsf@gitster.g/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Whereas the C11 warning is "just" recent FreeBSD && DEVELOPER=1. > > So I assumed if you weren't interested in the former before the final > you probably wouldn't be in the latter, but wanted to provide a more > narrow fix in case you were. If we muck with the inclusion of libgen.h, it then becomes a problem for everybody who builds on FreeBSD, not just the developer builds. IOW, it is not even narrower to begin with. Giving the same potential breakage to everybody will make it easier to diagnose it, but because I do not trust -std=gnu99 on today's FreeBSD, I think it is a problem we do not even have to solve. > I.e. the point of doing it is to avoid the one-time pain of anyone > building new releases of git on $OLD_OS/$OLD_DISTRO not having to run > into the compilation error that's fixed with NO_UNCOMPRESS2=Y. > ... > If we then get this into v2.36.0 there'll be someone somewhere that > benefits, but I'd think the ship has sailed for most of those who'd > avoid the needless flag twiddling (git-packagers@ et al). I actually think it is a good thing. It is what they brought onto themselves. They can follow David's example next time.
On Tue, Jan 18 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Whereas the C11 warning is "just" recent FreeBSD && DEVELOPER=1. >> >> So I assumed if you weren't interested in the former before the final >> you probably wouldn't be in the latter, but wanted to provide a more >> narrow fix in case you were. > > If we muck with the inclusion of libgen.h, it then becomes a problem > for everybody who builds on FreeBSD, not just the developer builds. > IOW, it is not even narrower to begin with. Giving the same > potential breakage to everybody will make it easier to diagnose it, > but because I do not trust -std=gnu99 on today's FreeBSD, I think it > is a problem we do not even have to solve. The patch I submitted could trivially have an additional "#ifdef DEVELOPER" or equivalent, which would narrow it down even further. The reason it doesn't is because we provide -std=gnu99 in config.mak.dev, but that doesn't mean that we don't have to deal with e.g. a user-supplied -std=gnu99. Except of course if we declare that we're not going to generally support such a thing, and you should only provide such flags via DEVELOPER/DEVOPTS, not manually in CFLAGS, which is fair enough. >> I.e. the point of doing it is to avoid the one-time pain of anyone >> building new releases of git on $OLD_OS/$OLD_DISTRO not having to run >> into the compilation error that's fixed with NO_UNCOMPRESS2=Y. >> ... >> If we then get this into v2.36.0 there'll be someone somewhere that >> benefits, but I'd think the ship has sailed for most of those who'd >> avoid the needless flag twiddling (git-packagers@ et al). > > I actually think it is a good thing. It is what they brought onto > themselves. They can follow David's example next time. Do you mean David Aguilar's addition of "NO_UNCOMPRESS2 = YesPlease" in [1]? Isn't that mutually exclusive with pursuing my change to auto-detect it[2] instead? As noted I'm a bit "meh" on my own patch if it's not in the release. I'm just trying to see where you stand, since you seemed to want a re-roll of it post-release.... 1. https://lore.kernel.org/git/20220116020520.26895-1-davvid@gmail.com/ 2. https://lore.kernel.org/git/patch-1.1-9cea01a1395-20220117T170457Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > As noted I'm a bit "meh" on my own patch if it's not in the release. I'm > just trying to see where you stand, since you seemed to want a re-roll > of it post-release.... I was hoping to see a quick reroll that would be in time for the release, but it seems it was overly optimistic back when I gave my initial review. I do not see it happening given that I'd be tagging the -rc2 tomorrow. I do think in the longer term, we should aim to retire the current mechanism and depend more on ZLIB_VERSION_NUM. I am not optimistic that we can have a reasonable version before the final.
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