Fix 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

gitgitgadget

From: Jeff Hostetler <[email protected]>

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

---
 builtin/merge-file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index c923bbf2abb..607c3d3f9e1 100644
@@ -26,9 +26,9 @@ static int label_cb(const struct option *opt, const char *arg, int unset)
 int cmd_merge_file(int argc, const char **argv, const char *prefix)
 {
 	const char *names[3] = { 0 };
-	mmfile_t mmfs[3] = { 0 };
+	mmfile_t mmfs[3] = { { 0 } };
 	mmbuffer_t result = { 0 };
-	xmparam_t xmp = { 0 };
+	xmparam_t xmp = { { 0 } };
 	int ret = 0, i = 0, to_stdout = 0;
 	int quiet = 0;
 	struct option options[] = {

gitgitgadget

From: Jeff Hostetler <[email protected]>

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

---
 builtin/unpack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 43789b8ef29..4b16f1592ba 100644
@@ -385,7 +385,7 @@ static const void *feed_input_zstream(struct input_stream *in_stream,
 
 static void stream_blob(unsigned long size, unsigned nr)
 {
-	git_zstream zstream = { 0 };
+	git_zstream zstream = { { 0 } };
 	struct input_zstream_data data = { 0 };
 	struct input_stream in_stream = {
 		.read = feed_input_zstream,

gitgitgadget

Ah, this looked familiar, and the last time we saw the one in builtin/unpack-objects.c

https://lore.kernel.org/git/[email protected]/

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?

For completeness, the merge-file.c instance also got reported:
https://lore.kernel.org/git/365e01e93dce582e9d926e83bdc6891310d22699.1659084832.git.congdanhqx@gmail.com/

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.

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/[email protected]/
[2]: https://lore.kernel.org/git/[email protected]/
[3]: https://lore.kernel.org/git/[email protected]/

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.

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/[email protected]
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/[email protected]/

[...]

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/[email protected]/

[2] https://lore.kernel.org/git/365e01e93dce582e9d926e83bdc6891310d22699.1659084832.git.congdanhqx@gmail.com/

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/[email protected]/

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

index 4fa19d361b..4d59c9044f 100644
@@ -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

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:

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é

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.

From: Jeff Hostetler <[email protected]>

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.

---
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(+)

index 4fa19d361b7..981304727c5 100644
@@ -69,6 +69,31 @@ DEVELOPER_CFLAGS += -Wno-missing-braces
 endif
 endif
 
+# Old versions of clang complain about initializaing a
+# struct-within-a-struct using just "{0}" rather than "{{0}}".  This
+# error is considered a false-positive and not worth fixing, because
+# new clang versions do not, so just disable it.
+#
+# The "bug" was fixed in upstream clang 9.
+#
+# Complicating this is that versions of clang released by Apple have
+# their own version numbers (associated with the corresponding version
+# of XCode) unrelated to the official clang version numbers.
+#
+# The bug was fixed in Apple clang 12.
+#
+ifneq ($(filter clang1,$(COMPILER_FEATURES)),)     # if we are using clang
+ifeq ($(uname_S),Darwin)                           # if we are on darwin
+ifeq ($(filter clang12,$(COMPILER_FEATURES)),)     # if version < 12
+DEVELOPER_CFLAGS += -Wno-missing-braces
+endif
+else                                               # not darwin
+ifeq ($(filter clang9,$(COMPILER_FEATURES)),)      # if version < 9
+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

base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164

gitgitgadget

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.