patch, 2 partsFix syntax errors under clang 11.0.0 on MacOS

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

From: Jeff Hostetler <jeffhost@microsoft.com>

Add extra set of braces around zero initialization of two array/structure variables to resolve compiler errors/warnings from clang 11.0.0 on MacOS. This is not needed on clang 14.0.

$ uname -a
Darwin jeffhost-mbp.local 19.6.0 Darwin Kernel Version 19.6.0: \
       Mon Apr 18 21:50:40 PDT 2022; \
       root:xnu-6153.141.62~1/RELEASE_X86_64 x86_64
$ clang -v
Apple clang version 11.0.0 (clang-1100.0.33.17)
Target: x86_64-apple-darwin19.6.0
[...]

$ make builtin/merge-file.o
    CC builtin/merge-file.o
builtin/merge-file.c:29:23: error: suggest braces around initialization \
			    of subobject [-Werror,-Wmissing-braces]
        mmfile_t mmfs[3] = { 0 };
                             ^
                             {}
builtin/merge-file.c:31:20: error: suggest braces around initialization \
			    of subobject [-Werror,-Wmissing-braces]
        xmparam_t xmp = { 0 };
                          ^
                          {}
2 errors generated.
make: *** [builtin/merge-file.o] Error 1

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/merge-file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Am 06.10.22 um 21:43 schrieb Jeff Hostetler via GitGitGadget:

Not sure how the Apple version numbers map to LLVM versions. I can reproduce it with Godbolt's Compiler Explorer with clang 8, but not with clang 9 or higher: https://godbolt.org/z/f7f7s9xxz

Funny that type of the "inner" member seems to affect the warning.

{0} is an idiom to zero-initialize any struct, no matter how deeply nested. It's valid C and the compiler warning about it is not helpful. Shouldn't we rather silence it with -Wno-missing-braces?

On the other hand: Uglifying just three initializations in total isn't that bad. The issue may reappear, though, because most people use compilers that don't issue that spurious warning, I imagine.

On Thu, Oct 6, 2022 at 5:25 PM René Scharfe <l.s.r@web.de> wrote:

That was indeed Ævar's suggestion in [1]:

    Since this is only a warning, and only a practical issue with
    -Werror I wonder if a config.mak.dev change wouldn't be better,
    i.e. to provide a -Wno-missing-braces for this older clang
    version.

The problem is that Apple seems to invent their version numbers out of thin air with no relation to reality[2,3], so it may take some effort to work out suitable "version mapping rules" for config.mak.dev, and nobody has done it yet.

[1]: https://lore.kernel.org/git/220712.86lesy6cri.gmgdl@evledraar.gmail.com/ [2]: https://lore.kernel.org/git/Ys0hhYjPExuNWynE@coredump.intra.peff.net/ [3]: https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/

Eric Sunshine <sunshine@sunshineco.com> writes:

I agree that telling affected users to squelch the warning is a good first step, and automating it, if there is an easy implementation to do so, would be a good thing to have on top.

Thanks for digging up the thread.

From: Jeff Hostetler <jeffhost@microsoft.com>

Add an extra set of braces around zero initialization of the `zstream` variable to resolve compiler error/warning from clang 11.0.0 on MacOS. This is not needed on clang 14.0.

$ make builtin/unpack-objects.o
    CC builtin/unpack-objects.o
builtin/unpack-objects.c:388:26: error: suggest braces around \
		initialization of subobject [-Werror,-Wmissing-braces]
        git_zstream zstream = { 0 };
                                ^
                                {}
1 error generated.
make: *** [builtin/unpack-objects.o] Error 1

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 builtin/unpack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

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:

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:

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:

[...]

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:

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.

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.

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 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.

Exactly.

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:

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

On 10/7/22 4:28 PM, René Scharfe wrote:

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

Jeff Hostetler <git@jeffhostetler.com> writes:

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:

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:

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:

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.

René Scharfe <l.s.r@web.de> writes:

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.

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 ...

... which adds this workaround.

OK. That is dense, but sounds correct.

From: Jeff Hostetler <jeffhost@microsoft.com>

Add the "-Wno-missing-braces" option when building with an old version of clang to suppress the "suggest braces around initialization" error in developer mode.

For example, using an old version of clang gives the following errors (when in DEVELOPER=1 mode):

$ make builtin/merge-file.o
    CC builtin/merge-file.o
builtin/merge-file.c:29:23: error: suggest braces around initialization \
			    of subobject [-Werror,-Wmissing-braces]
        mmfile_t mmfs[3] = { 0 };
                             ^
                             {}
builtin/merge-file.c:31:20: error: suggest braces around initialization \
			    of subobject [-Werror,-Wmissing-braces]
        xmparam_t xmp = { 0 };
                          ^
                          {}
2 errors generated.

This example compiles without error/warning with updated versions of clang. Since this is an obsolete error, use the -Wno-missing-braces option to silence the warning when using an older compiler. This avoids the need to update the code to use "{{0}}" style initializations.

Upstream clang version 8 has the problem.  It was fixed in version 9.

The version of clang distributed by Apple with XCode has its own unique set of version numbers. Apple clang version 11 has the problem. It was fixed in version 12.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
    Fix syntax errors under clang 11.0.0 on MacOS
    
    Here is version 2. This version adds the "-Wno-missing-braces" compiler
    flag when we are using an old version of clang -- rather than updating
    the source files to use the "{{0}}" syntax that was expected by older
    versions of clang.
    
    I've expanded the scope to include fixes for Apple's clang 11 and below
    and non-Apple clang 8 and below.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1375%2Fjeffhostetler%2Ffix-clang11-warnings-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1375/jeffhostetler/fix-clang11-warnings-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1375

Range-diff vs v1:

 1:  7cee38788a7 < -:  ----------- builtin/merge-file: fix compiler error on MacOS with clang 11.0.0
 2:  e5009a325f2 < -:  ----------- builtin/unpack-objects.c: fix compiler error on MacOS with clang 11.0.0
 -:  ----------- > 1:  c6708ed9459 config.mak.dev: disable suggest braces error on old clang versions

 config.mak.dev | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

Looks very straight-forward, albeit a tad verbose, and simple to follow. I think this is equivalent to René's original one-liner.

Will queue.  Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Funny that during the whole time we were discussing this patch, I never recalled b53a5f24 (config.mak.dev: squelch -Wno-missing-braces for older gcc, 2022-07-29), which was a moral equivalent of this change for older GCC, until I needed to decide the name of the topic branch to queue this patch.

I had the topic branch jk/struct-zero-init-with-older-gcc for b53a5f24 still lying around. I reused a part of its name and queued it on jh/struct-zero-init-with-older-clang branch.