| Message ID | pull.1375.git.1665085395.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix syntax errors under clang 11.0.0 on MacOS | expand |
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > This patch series fixes three syntax errors that caused compiler errors with > clang 11.0.0 on MacOS. I've included the error/warning messages in the > commit messages. The offending statements did compile successfully under > clang 14.0.0 on MacOS, so I have to assume that this usage is newer than > what clang 11 supports. Ah, this looked familiar, and the last time we saw the one in builtin/unpack-objects.c https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/ It seems that merge-file.c was OK back then but soon after we "broke" it with 480a0e30 (merge-file: refactor for subsequent memory leak fix, 2022-07-01). $ git grep -n -e '{ *{ *0 *} *}' builtin/merge-index.c:15: char oids[3][GIT_MAX_HEXSZ + 1] = {{0}}; builtin/merge-index.c:16: char modes[3][10] = {{0}}; builtin/worktree.c:321: struct config_set cs = { { 0 } }; merge-strategies.c:93: xmparam_t xmp = {{0}}; oidset.h:25:#define OIDSET_INIT { { 0 } } worktree.c:795: struct config_set cs = { { 0 } }; I wonder if we want to introduce some named _INIT for these specific types? Perhaps with something like /* for config_set */ #define CONFIG_SET_INIT {{0}} /* for xmparam_t */ #define XMPARAM_INIT {{0}} /* for mmfile_t */ #define MMFILE_INIT {0} /* for git_zstream */ #define GIT_ZSTREAM_INIT {{0}} Side note: between { { 0 } } and {{0}} forms that appear in the existing code, I picked the "unusual spacing" variant, i.e. without any space, hoping that it would signal the fact that we would prefer if we didn't have to use these to please older compilers to the readers. Unless we plan to soon declare that clang 11 is too old to matter anymore, that is. The rule, tentatively until the compilers that need extra levels of braces die out, can be "if there is <name>_INIT for the type, you should use it, but otherwise you can do { 0 }", or something like that, perhaps?
On Thu, Oct 6, 2022 at 4:41 PM Junio C Hamano <gitster@pobox.com> wrote: > "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > > This patch series fixes three syntax errors that caused compiler errors with > > clang 11.0.0 on MacOS. I've included the error/warning messages in the > > commit messages. The offending statements did compile successfully under > > clang 14.0.0 on MacOS, so I have to assume that this usage is newer than > > what clang 11 supports. > > Ah, this looked familiar, and the last time we saw the one in > builtin/unpack-objects.c > > https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/ > > It seems that merge-file.c was OK back then but soon after we > "broke" it with 480a0e30 (merge-file: refactor for subsequent memory > leak fix, 2022-07-01). For completeness, the merge-file.c instance also got reported: https://lore.kernel.org/git/365e01e93dce582e9d926e83bdc6891310d22699.1659084832.git.congdanhqx@gmail.com/
On Thu, Oct 06 2022, Jeff Hostetler via GitGitGadget wrote: > This patch series fixes three syntax errors that caused compiler errors with > clang 11.0.0 on MacOS. I've included the error/warning messages in the > commit messages. The offending statements did compile successfully under > clang 14.0.0 on MacOS, so I have to assume that this usage is newer than > what clang 11 supports. > > I originally sent these changes in my "Trace2 timers and cleanup and some > cleanup" series on Tuesday, but pulled them into a separate series based on > feedback. I'll omit them from the trace2 series in the next version. The expanded commit messages really help, thanks :) So, to summarize, these don't fix compiler errors, but warnings, but of course they're errors with DEVELOPER=1. We already squash these for an older GCC, per the discussion at [1]. I think we should just replace this series with something like (untested on OSX, but it's just copy/pasting a template above it). diff --git a/config.mak.dev b/config.mak.dev index 4fa19d361b7..9b7bccd154c 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -69,6 +69,14 @@ DEVELOPER_CFLAGS += -Wno-missing-braces endif endif +ifeq ($(uname_S),Darwin) +ifneq ($(filter clang10,$(COMPILER_FEATURES)),) +ifeq ($(filter clang11,$(COMPILER_FEATURES)),) +DEVELOPER_CFLAGS += -Wno-missing-braces +endif +endif +endif + # https://bugzilla.redhat.com/show_bug.cgi?id=2075786 ifneq ($(filter gcc12,$(COMPILER_FEATURES)),) DEVELOPER_CFLAGS += -Wno-error=stringop-overread Or, we can just say that for a <= clang v13 we'll use -Wno-missing-braces, per: * The comment from René at http://lore.kernel.org/git/36cd156b-edb2-062c-9422-bf39aad39a6d@web.de that older vanilla clang is affected. * You having tested Apple clang v14, but not clang v12..v13. I.e. to emit the whole uname_S bit. I think it's not important that we try really hard to opt a given compiler into some maximum set of warnings, we generally want to catch most things here. As long as some compiler (particularly if it's in CI) still covers these we should be good. 1. https://lore.kernel.org/git/220712.864jzm65mk.gmgdl@evledraar.gmail.com/
On 10/6/22 3:43 PM, Jeff Hostetler via GitGitGadget wrote: > This patch series fixes three syntax errors that caused compiler errors with > clang 11.0.0 on MacOS. I've included the error/warning messages in the > commit messages. The offending statements did compile successfully under > clang 14.0.0 on MacOS, so I have to assume that this usage is newer than > what clang 11 supports. [...] I didn't realize that these variable initialization fixes would spawn such a large response here [1]. Nor that it had been already discussed in great length in [2] in July. I'm not sure how to best proceed here. * I'm not sure these fixes are important enough to warrant the engineering time to hack up the Makefile or config.mak.uname to conditionally turn off -Wno-missing-braces based on some {platform-os-version, gcc/clang-version} combination. * While -Wno-missing-braces option may prevent the warning/error (depending on -Werror) for these "{0}" should be "{{0}}" errors, do we know that this won't hide real problems. (I mean we tend to only see it for these {0} / {{0}} false alarms, but I'd hate to lose the protection for non-false alarms.) * The suggestion to use a <type>_INIT macro to hide the {0} or {{0}} may help in the: xmparam_t xmp = XMPARAM_INIT; case, but in the `mmfile_t mmfs[3]` case, we have an array of that type, so we'd need something like: mmfile_t mmfs[3] = { MMFILE_INIT }; or mmfile_t mmfs[3] = MMFILE_INIT_ARRAY; for the macros to make sense. I'm not sure either of these two is better than just writing "{{0}}". * I wasn't sure which compiler versions we *want* to support or want to drop support for. * I've only thought about it in the context of clang on MacOS. * Clang 11 (from what I can tell (without looking too hard)) comes with the version of XCode that runs on MacOS 10.15 Catalina. (In contrast, clang 14 comes with the version of XCode that runs on MacOS 12.6 Monterey.) * I can't comment on other old-ish compilers on other platforms. * I'm not the first one who has stumbled over this and had to rediscover the solution. So I'd hate to just kick this down the road some more, but then again I'd hate to waste a lot of time on it -- for very little actual functional value. * Is "{{0}}" really that ugly ??? So as I said earlier, I'm not sure how to proceed here. Jeff [1] https://lore.kernel.org/git/pull.1375.git.1665085395.gitgitgadget@gmail.com/ [2] https://lore.kernel.org/git/365e01e93dce582e9d926e83bdc6891310d22699.1659084832.git.congdanhqx@gmail.com/
Jeff Hostetler <git@jeffhostetler.com> writes: > I didn't realize that these variable initialization fixes would > spawn such a large response here [1]. Nor that it had been already > discussed in great length in [2] in July. > > I'm not sure how to best proceed here. > > * I'm not sure these fixes are important enough to warrant the > engineering time to hack up the Makefile or config.mak.uname > to conditionally turn off -Wno-missing-braces based on some > {platform-os-version, gcc/clang-version} combination. Developers are expected to be able to cope, but if this something a few easy lines in config.mak.uname can help, we'd better do so, instead of having dozens of folks on the macOS add the same line to their config.mak file. > * While -Wno-missing-braces option may prevent the warning/error > (depending on -Werror) for these "{0}" should be "{{0}}" > errors, do we know that this won't hide real problems. (I mean > we tend to only see it for these {0} / {{0}} false alarms, but > I'd hate to lose the protection for non-false alarms.) It may find real problems, but folks on other platforms are running without the check disabled, so we should be OK. More importantly, folks with a more recent compilers on macOS are running with the check, so we should be able to give a reasonable coverage to platform specific code, too. > * The suggestion to use a <type>_INIT macro to hide the {0} or > {{0}} may help in the: > xmparam_t xmp = XMPARAM_INIT; > case, but in the `mmfile_t mmfs[3]` case, we have an array of > that type, so we'd need something like: > mmfile_t mmfs[3] = { MMFILE_INIT }; or > mmfile_t mmfs[3] = MMFILE_INIT_ARRAY; > for the macros to make sense. > I'm not sure either of these two is better than just writing "{{0}}". I do not like that suggestion at all. I think it is the right approach to use -Wno-missing-braces with older and buggy compilers until they die out. > * I wasn't sure which compiler versions we *want* to support or > want to drop support for. > * I've only thought about it in the context of clang on MacOS. I think that is OK. IIRC, we have more or less good grasp on the cut-off version of clang as the upstream calls it, but the problem is clang shipped with macOS lies and gives a version number more recent than it actually is. > * I'm not the first one who has stumbled over this and had to > rediscover the solution. So I'd hate to just kick this down > the road some more, but then again I'd hate to waste a lot of > time on it -- for very little actual functional value. Exactly. > * Is "{{0}}" really that ugly ??? Ugly is not the issue. Folks on more recent platforms not being able to tell when to use the extra parentheses is, because their compilers do not have this bug. My preference is to flip the -Wno-missing-braces bit in config.mak.uname only for folks who use the version of clang on macOS when that clang claims to be clang11 (my understanding of René's experiment[*] is that versions of (real) clang 9 or newer perfectly well understand that {0} is an accpetable way to specify zero initialization for any structure, with possible nesting). [Reference] * https://lore.kernel.org/git/36cd156b-edb2-062c-9422-bf39aad39a6d@web.de/
Am 07.10.22 um 19:49 schrieb Junio C Hamano: > > My preference is to flip the -Wno-missing-braces bit in > config.mak.uname only for folks who use the version of clang on > macOS when that clang claims to be clang11 (my understanding of > René's experiment[*] is that versions of (real) clang 9 or newer > perfectly well understand that {0} is an accpetable way to specify > zero initialization for any structure, with possible nesting). > > [Reference] > > * https://lore.kernel.org/git/36cd156b-edb2-062c-9422-bf39aad39a6d@web.de/ Wikipedia has a map that says Apple calls the LLVM clang 8 (i.e. the real one) "11.0.0" and clang 9 "11.0.3": https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)_2 Perhaps like this? (No sign-off because I'm not comfortable with that make function syntax, but feel free to steal salvageable parts.) diff --git a/config.mak.dev b/config.mak.dev index 4fa19d361b..4d59c9044f 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -69,6 +69,14 @@ DEVELOPER_CFLAGS += -Wno-missing-braces endif endif +# LLVM clang older than 9 and Apple clang older than 12 complain +# about initializing a struct-within-a-struct using just "{ 0 }" +ifneq ($(filter clang1,$(COMPILER_FEATURES)),) +ifeq ($(filter $(if $(filter Darwin,$(uname_S)),clang12,clang9),$(COMPILER_FEATURES)),) +DEVELOPER_CFLAGS += -Wno-missing-braces +endif +endif + # https://bugzilla.redhat.com/show_bug.cgi?id=2075786 ifneq ($(filter gcc12,$(COMPILER_FEATURES)),) DEVELOPER_CFLAGS += -Wno-error=stringop-overread
On 10/7/22 4:28 PM, René Scharfe wrote: > Am 07.10.22 um 19:49 schrieb Junio C Hamano: >> >> My preference is to flip the -Wno-missing-braces bit in >> config.mak.uname only for folks who use the version of clang on >> macOS when that clang claims to be clang11 (my understanding of >> René's experiment[*] is that versions of (real) clang 9 or newer >> perfectly well understand that {0} is an accpetable way to specify >> zero initialization for any structure, with possible nesting). >> >> [Reference] >> >> * https://lore.kernel.org/git/36cd156b-edb2-062c-9422-bf39aad39a6d@web.de/ > > Wikipedia has a map that says Apple calls the LLVM clang 8 (i.e. the > real one) "11.0.0" and clang 9 "11.0.3": > > https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)_2 > > Perhaps like this? (No sign-off because I'm not comfortable with that > make function syntax, but feel free to steal salvageable parts.) > > diff --git a/config.mak.dev b/config.mak.dev > index 4fa19d361b..4d59c9044f 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -69,6 +69,14 @@ DEVELOPER_CFLAGS += -Wno-missing-braces > endif > endif > > +# LLVM clang older than 9 and Apple clang older than 12 complain > +# about initializing a struct-within-a-struct using just "{ 0 }" > +ifneq ($(filter clang1,$(COMPILER_FEATURES)),) > +ifeq ($(filter $(if $(filter Darwin,$(uname_S)),clang12,clang9),$(COMPILER_FEATURES)),) > +DEVELOPER_CFLAGS += -Wno-missing-braces > +endif > +endif > + So if I understand you correctly, Apple clang 11 is broken and Apple clang 12 is good. I was getting ready to send (as soon as the CI finished) the following a simple to add the -Wno... for clang 11 and below on Darwin. +ifeq ($(uname_S),Darwin) +# Older versions of Apple clang complain about initializing a +# struct-within-a-struct using just "{0}" rather than "{{0}}". +# More recent versions do not. This error is considered a +# false-positive and not worth fixing, so just disable it. +ifeq ($(filter clang12,$(COMPILER_FEATURES)),) +DEVELOPER_CFLAGS += -Wno-missing-braces +endif +endif I'm not sure I understand all of what your suggestion does. Jeff
René Scharfe <l.s.r@web.de> writes: > Perhaps like this? (No sign-off because I'm not comfortable with that > make function syntax, but feel free to steal salvageable parts.) > > diff --git a/config.mak.dev b/config.mak.dev > index 4fa19d361b..4d59c9044f 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -69,6 +69,14 @@ DEVELOPER_CFLAGS += -Wno-missing-braces > endif > endif > > +# LLVM clang older than 9 and Apple clang older than 12 complain > +# about initializing a struct-within-a-struct using just "{ 0 }" > +ifneq ($(filter clang1,$(COMPILER_FEATURES)),) If it is any version of clang, detect-compiler script should give us at last "clang1" in $(COMPILER_FEATURES), and filtering the latter with "clang1" should become "clang1" which should be neq. We fail the ifneq if the compiler is not llvm and do not come into this part. > +ifeq ($(filter $(if $(filter Darwin,$(uname_S)),clang12,clang9),$(COMPILER_FEATURES)),) On Darwin, $(uname_S) is Darwin and filtering the $(uname_S) with Darwin will leave Darwin as the "condition" parameter to $(if). So $(if) evaluates to clang12 on Darwin and clang9 everywhere else. If $(COMPILER_FEATURES) lacks that cut-off version, that means we are using clang older than 12 (on macOS) or 9 (everywhere else) and we come inside this block ... > +DEVELOPER_CFLAGS += -Wno-missing-braces ... which adds this workaround. > +endif > +endif OK. That is dense, but sounds correct.
Jeff Hostetler <git@jeffhostetler.com> writes: > So if I understand you correctly, Apple clang 11 is broken > and Apple clang 12 is good. > > I was getting ready to send (as soon as the CI finished) > the following a simple to add the -Wno... for clang 11 and > below on Darwin. > > +ifeq ($(uname_S),Darwin) > +# Older versions of Apple clang complain about initializing a > +# struct-within-a-struct using just "{0}" rather than "{{0}}". > +# More recent versions do not. This error is considered a > +# false-positive and not worth fixing, so just disable it. > +ifeq ($(filter clang12,$(COMPILER_FEATURES)),) > +DEVELOPER_CFLAGS += -Wno-missing-braces > +endif > +endif > > I'm not sure I understand all of what your suggestion does. I do agree that one is dense, but aims for the same thing, and a bit more. It might be easier to read if written in longhand, but ... ifeq ($(uname_s),Darwin) ifeq ($(filter clang12,$(COMPILER_FEATURES)),) DEVELOPER_CFLAGS += -Wno-missing-braces endif else ifeq ($(filter clang9,$(COMPILER_FEATURES)),) DEVELOPER_CFLAGS += -Wno-missing-braces endif endif ... we'd need to repeat ourselves, so...
On Fri, Oct 7, 2022 at 5:36 PM Junio C Hamano <gitster@pobox.com> wrote: > I do agree that one is dense, but aims for the same thing, and a bit > more. It might be easier to read if written in longhand, but ... > > ifeq ($(uname_s),Darwin) > ifeq ($(filter clang12,$(COMPILER_FEATURES)),) > DEVELOPER_CFLAGS += -Wno-missing-braces > endif > else > ifeq ($(filter clang9,$(COMPILER_FEATURES)),) > DEVELOPER_CFLAGS += -Wno-missing-braces > endif > endif > > ... we'd need to repeat ourselves, so... The repetition is a very minor downside. The big benefit of this version is that it's easy to understand at-a-glance, unlike the "dense" version with its high cognitive load.
Am 08.10.22 um 05:46 schrieb Eric Sunshine: > On Fri, Oct 7, 2022 at 5:36 PM Junio C Hamano <gitster@pobox.com> wrote: >> I do agree that one is dense, but aims for the same thing, and a bit >> more. It might be easier to read if written in longhand, but ... >> >> ifeq ($(uname_s),Darwin) >> ifeq ($(filter clang12,$(COMPILER_FEATURES)),) >> DEVELOPER_CFLAGS += -Wno-missing-braces >> endif >> else >> ifeq ($(filter clang9,$(COMPILER_FEATURES)),) >> DEVELOPER_CFLAGS += -Wno-missing-braces >> endif >> endif >> >> ... we'd need to repeat ourselves, so... > > The repetition is a very minor downside. The big benefit of this > version is that it's easy to understand at-a-glance, unlike the > "dense" version with its high cognitive load. It's certainly easier. It triggers for any compiler that is not clang, though, which is a bit much. Alternative compilers may not even understand that flag. So the whole thing should be wrapped in ifneq ($(filter clang1,$(COMPILER_FEATURES)),) ... endif or ifneq ($(filter clang%,$(COMPILER_FEATURES)),) ... endif René
René Scharfe <l.s.r@web.de> writes: > Am 08.10.22 um 05:46 schrieb Eric Sunshine: >> On Fri, Oct 7, 2022 at 5:36 PM Junio C Hamano <gitster@pobox.com> wrote: >>> I do agree that one is dense, but aims for the same thing, and a bit >>> more. It might be easier to read if written in longhand, but ... >>> >>> ifeq ($(uname_s),Darwin) >>> ifeq ($(filter clang12,$(COMPILER_FEATURES)),) >>> DEVELOPER_CFLAGS += -Wno-missing-braces >>> endif >>> else >>> ifeq ($(filter clang9,$(COMPILER_FEATURES)),) >>> DEVELOPER_CFLAGS += -Wno-missing-braces >>> endif >>> endif >>> >>> ... we'd need to repeat ourselves, so... >> >> The repetition is a very minor downside. The big benefit of this >> version is that it's easy to understand at-a-glance, unlike the >> "dense" version with its high cognitive load. > > It's certainly easier. > > It triggers for any compiler that is not clang, though, which is > a bit much. Yeah, of course you are right. In my "here is a translation to human-language" I did explain what "clang1" was doing in your version, but of course, I forgot all about it when writing the above variant.
This patch series fixes three syntax errors that caused compiler errors with clang 11.0.0 on MacOS. I've included the error/warning messages in the commit messages. The offending statements did compile successfully under clang 14.0.0 on MacOS, so I have to assume that this usage is newer than what clang 11 supports. I originally sent these changes in my "Trace2 timers and cleanup and some cleanup" series on Tuesday, but pulled them into a separate series based on feedback. I'll omit them from the trace2 series in the next version. Jeff Hostetler (2): builtin/merge-file: fix compiler error on MacOS with clang 11.0.0 builtin/unpack-objects.c: fix compiler error on MacOS with clang 11.0.0 builtin/merge-file.c | 4 ++-- builtin/unpack-objects.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1375%2Fjeffhostetler%2Ffix-clang11-warnings-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1375/jeffhostetler/fix-clang11-warnings-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1375