ci: detect more warnings via `-Og`

Hi,

this patch series is triggered by the thread at [1], where Peff noted a potentially-uninitialized variable. In theory, GCC is able to diagnose such issues via -Wmaybe-uninitialized. But in practice, this warning is dependent on the optimization level used by GCC, where the issue in the thread is only noticed when compiling with -Og.

The first patch is just a small fix to our CI I found while at it. The second patch adapts our "pedantic" job to compile with -Og so that we overall surface more issues. This would've catched the issue.

Patrick

[1]: <[email protected]>

Patrick Steinhardt (2):
ci: fix check for Ubuntu 20.04
ci: let pedantic job compile with -Og

 ci/lib.sh                 | 2 +-
 ci/run-build-and-tests.sh | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

2.45.2.409.g7b0defb391.dirty

In 5ca0c455f1 (ci: fix Python dependency on Ubuntu 24.04, 2024-05-06), we made the use of Python 2 conditional on whether or not the CI job runs Ubuntu 20.04. There was a brown-paper-bag-style bug though, where the condition forgot to invoke the test builtin. The result of it is that the check always fails, and thus all of our jobs run with Python 3 by accident.

Fix this.

---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1f4059b1b8..814578ffc6 100755
@@ -328,7 +328,7 @@ ubuntu-*)
 	# Python 2 is end of life, and Ubuntu 23.04 and newer don't actually
 	# have it anymore. We thus only test with Python 2 on older LTS
 	# releases.
-	if "$distro" = "ubuntu-20.04"
+	if test "$distro" = "ubuntu-20.04"
 	then
 		PYTHON_PACKAGE=python2
 	else

2.45.2.409.g7b0defb391.dirty

We have recently noticed that our CI does not always notice variables that may be used uninitialized. While it is expected that compiler warnings aren't perfect, this one was a but puzzling because it was rather obvious that the variable can be uninitialized.

Many compiler warnings unfortunately depend on the optimization level used by the compiler. While -O0 for example will disable a lot of warnings altogether because optimization passes go away, -O2, which is our default optimization level used in CI, may optimize specific code away or even double down on undefined behaviour. Interestingly, this specific instance that triggered the investigation does get noted by GCC when using -Og.

While we could adapt all jobs to compile with -Og now, that would potentially mask other warnings that only get diagnosed with -O2. Instead, only adapt the "pedantic" job to compile with -Og.

---
 ci/run-build-and-tests.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

index 98dda42045..e78e19e4a6 100755
@@ -44,6 +44,15 @@ pedantic)
 	# Don't run the tests; we only care about whether Git can be
 	# built.
 	export DEVOPTS=pedantic
+	# Warnings generated by compilers are unfortunately specific to the
+	# optimization level. With `-O0`, many warnings won't be shown at all,
+	# whereas the optimizations performed by our default optimization level
+	# `-O2` will mask others. We thus use `-Og` here just so that we have
+	# at least one job with a different optimization level so that we can
+	# overall surface more warnings.
+	cat >config.mak <<-EOF
+	export CFLAGS=-Og
+	EOF
 	run_tests=
 	;;
 esac

2.45.2.409.g7b0defb391.dirty

Hmm. This is the first time I've ever seen lower optimization levels produce more warnings. It is almost always the opposite case in my experience. So it's not clear to me that moving to "-Og" will generally find more warning spots, and that this isn't a bit of a fluke.

As you note, we'll still compile with -O2 in other jobs. But isn't the point of the pedantic job to enable a bunch of extra warnings that aren't available elsewhere? We wouldn't have any coverage of those.

So for the pedantic warnings, we're left with a guess as to whether -Og or -O2 will yield more results. And in my experience it is probably -O2.

If we want to get coverage of -Og, I'd suggest doing it in a job that is otherwise overlapping with another (maybe linux-TEST-vars, which I think is otherwise a duplicate build?).

  O ?= 2
  CFLAGS = -g -Wall -O$(O)

Yikes. This looks obviously correct. Though I guess nobody noticed or cared that we were not using python 2? It sounds like it is a nice-to-have to get more coverage, but the platform in question is happy to use python 3).

-Peff

I don't think linux-TEST-vars would be a good candidate for this because it uses Ubuntu 20.04. Ideally, we'd want to have a test run with an up-to-date version of Ubuntu so that we also get a recent version of the compiler toolchain.

I kind of wonder whether we should revamp this pedantic job in the first place. The consequence of that job is that our codebase needs to be compile cleanly with -Wpedantic. So if that is a requirement anyway, why don't we run all jobs with DEVOPTS=pedantic and just drop this job altogether? This may surface some additional warnings on platforms where we currently don't set that, but is that a bad thing?

The only downside I can think of is that we stop compiling on Fedora, which may have a more up-to-date GCC version than Ubuntu. But if the goal of this job was to _really_ get an up-to-date compiler toolchain, then we should rather pick a rolling release distro like Arch. Otherwise I find this to be of dubious benefit.

If we merge it into the other jobs, then I'd just pick any random job that uses "ubuntu:latest" like "linux-gcc-default" to compile with -Og.

Yeah, I went the easy route because setting it via the environment is not enough, as you correctly mention.

Sure, I can do something along these lines.

Patrick

Yeah, the reason for this check really only is to get more coverage while Python 2 is still available on some of the distros that users may reasonably use. It's kind of a best effort check to keep it compatible, even though we will likely eventually drop that guarantee once Python 2 is getting phased out by distros.

Patrick

Oof, yeah, I agree that if the point is to find obscure warnings, newer is going to be better.

Yeah, if we always compile cleanly with pedantic, then I don't see why it wouldn't just be the default for DEVELOPER=1. The point of that flag is to be as picky as possible so that we catch things early. If some platform can't handle it (let's imagine Windows or something), then I think we should be explicitly _disabling_ pedantic there.

There may be some value in general in compiling on multiple distros, as a sort of "unknown unknowns" thing. We don't know what we might turn up, but exposing ourselves to more variables may lead to catching failures before users see them.

I don't know if Fedora was specifically chosen for recent gcc there, or if it was simply for variety.

Once again, these overlapping variables within various jobs make it hard to reason about (but I don't propose normalizing all of them; that would increase the amount of CPU work by a lot; I am just grumbling).

But yeah, between Arch and Fedora, I don't have an opinion. Doing both might even be valuable, if we are shoving random variations into random jobs. ;)

That would be OK with me. I also think it would be OK to do nothing for now. We saw one case where "-Og" found a warning that "-O2" didn't. We could wait to see if it happens twice before acting. I dunno.

-Peff

True enough. But even so, I think the better solution here would be to drop one of the Ubuntu-based jobs and then convert the Fedora one into a fully-fledged job that also runs tests.

That's something for a later iteration, though.

No arguing there, it certainly is hard to discover overall. I don't really think there's a way around this if we want to have different combinations while not running a full matrix of jobs, because the combinations are always going to be arbitrary.

True.

I don't think it hurts to have a job with -Og, and if it detects some corner cases that we otherwise don't I think there's a real benefit. In the best case, I'd want to give everyone the tools to avoid sending patch series to the mailing list which are broken and where that could have been detected earlier. In the end, it saves everybody's time.

Patrick

Hi,

this is the second version of my patch series that modifies one of our CI jobs to compile with -Og. We have noticed that it may surface more warnings that we do not see with -O2, so this should help to find more bugs up front.

Changes compared to v1:

- Instead of adapting the "pedantic" job, we now adapt "linux-gcc-default" to compile with -Og. This is because that job uses ubuntu:latest and thus a recent compiler, and there are other jobs with ubuntu:latest that continue to compile with -O2. So this is a strict improvement of coverage for diagnostics.

- Add a way to override the optimization level to our Makefile, as suggested by Peff.

Patrick

Patrick Steinhardt (2):
ci: fix check for Ubuntu 20.04
ci: compile "linux-gcc-default" job with -Og

 Makefile                  | 3 ++-
 ci/lib.sh                 | 2 +-
 ci/run-build-and-tests.sh | 9 +++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

Range-diff against v1:

1:  f91004a438 = 1:  f91004a438 ci: fix check for Ubuntu 20.04
2:  351dec4a4d ! 2:  bdf0e40a77 ci: let pedantic job compile with -Og

@@ Metadata
Author: Patrick Steinhardt <[email protected]>

## Commit message ##
- ci: let pedantic job compile with -Og
+ ci: compile "linux-gcc-default" job with -Og

We have recently noticed that our CI does not always notice variables that may be used uninitialized. While it is expected that compiler
@@ Commit message

While we could adapt all jobs to compile with -Og now, that would potentially mask other warnings that only get diagnosed with -O2.
- Instead, only adapt the "pedantic" job to compile with -Og.
+ Instead, adapt the "linux-gcc-default" job to compile with -Og. This
+ job is chosen because it uses the "ubuntu:latest" image and should thus
+ have a comparatively recent compiler toolchain, and because we have
+ other jobs that use "ubuntu:latest" so that we do not loose coverage for
+ warnings diagnosed only on -O2 level.
+
+ To make it easier to set up the optimization level in our CI, add
+ support in our Makefile to specify the level via an environment
+ variable.

+ ## Makefile ##
+@@ Makefile: endif
+ # tweaked by config.* below as well as the command-line, both of
+ # which'll override these defaults.
+ # Older versions of GCC may require adding "-std=gnu99" at the end. +-CFLAGS = -g -O2 -Wall
++O ?= 2
++CFLAGS = -g -O$(O) -Wall
+ LDFLAGS =
+ CC_LD_DYNPATH = -Wl,-rpath,
+ BASIC_CFLAGS = -I.
+
## ci/run-build-and-tests.sh ##
-@@ ci/run-build-and-tests.sh: pedantic)
- # Don't run the tests; we only care about whether Git can be
- # built.
- export DEVOPTS=pedantic
+@@ ci/run-build-and-tests.sh: esac
+ run_tests=t
+
+ case "$jobname" in
++linux-gcc-default)
+ # Warnings generated by compilers are unfortunately specific to the
+ # optimization level. With -O0, many warnings won't be shown at all,
+ # whereas the optimizations performed by our default optimization level
+ # -O2 will mask others. We thus use -Og here just so that we have
+ # at least one job with a different optimization level so that we can
+ # overall surface more warnings.
-+ cat >config.mak <<-EOF
-+ export CFLAGS=-Og
-+ EOF
- run_tests=
++ export O=g
++ ;;
+ linux-gcc)
+ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
;;
- esac

2.45.2.409.g7b0defb391.dirty

In 5ca0c455f1 (ci: fix Python dependency on Ubuntu 24.04, 2024-05-06), we made the use of Python 2 conditional on whether or not the CI job runs Ubuntu 20.04. There was a brown-paper-bag-style bug though, where the condition forgot to invoke the test builtin. The result of it is that the check always fails, and thus all of our jobs run with Python 3 by accident.

Fix this.

---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1f4059b1b8..814578ffc6 100755
@@ -328,7 +328,7 @@ ubuntu-*)
 	# Python 2 is end of life, and Ubuntu 23.04 and newer don't actually
 	# have it anymore. We thus only test with Python 2 on older LTS
 	# releases.
-	if "$distro" = "ubuntu-20.04"
+	if test "$distro" = "ubuntu-20.04"
 	then
 		PYTHON_PACKAGE=python2
 	else

2.45.2.409.g7b0defb391.dirty

We have recently noticed that our CI does not always notice variables that may be used uninitialized. While it is expected that compiler warnings aren't perfect, this one was a but puzzling because it was rather obvious that the variable can be uninitialized.

Many compiler warnings unfortunately depend on the optimization level used by the compiler. While -O0 for example will disable a lot of warnings altogether because optimization passes go away, -O2, which is our default optimization level used in CI, may optimize specific code away or even double down on undefined behaviour. Interestingly, this specific instance that triggered the investigation does get noted by GCC when using -Og.

While we could adapt all jobs to compile with -Og now, that would potentially mask other warnings that only get diagnosed with -O2. Instead, adapt the "linux-gcc-default" job to compile with -Og. This job is chosen because it uses the "ubuntu:latest" image and should thus have a comparatively recent compiler toolchain, and because we have other jobs that use "ubuntu:latest" so that we do not loose coverage for warnings diagnosed only on -O2 level.

To make it easier to set up the optimization level in our CI, add support in our Makefile to specify the level via an environment variable.

---

I was a little torn whether we really want to name the variable O here because it feels so easy to set it by accident. We could rename this to OPTIMIZATION or OPTIMIZATION_LEVEL, but that's quite a mouthful.

Alternatively, if we don't want to have this variable in the first place, then I'm also happy to adapt the script itself to pass the optimization level via an argument.

 Makefile                  | 3 ++-
 ci/run-build-and-tests.sh | 9 +++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

index 59d98ba688..ff57c94fdf 100644
@@ -1357,7 +1357,8 @@ endif
 # tweaked by config.* below as well as the command-line, both of
 # which'll override these defaults.
 # Older versions of GCC may require adding "-std=gnu99" at the end.
-CFLAGS = -g -O2 -Wall
+O ?= 2
+CFLAGS = -g -O$(O) -Wall
 LDFLAGS =
 CC_LD_DYNPATH = -Wl,-rpath,
 BASIC_CFLAGS = -I.

index 98dda42045..0f00dbd289 100755
@@ -13,6 +13,15 @@ esac
 run_tests=t
 
 case "$jobname" in
+linux-gcc-default)
+	# Warnings generated by compilers are unfortunately specific to the
+	# optimization level. With `-O0`, many warnings won't be shown at all,
+	# whereas the optimizations performed by our default optimization level
+	# `-O2` will mask others. We thus use `-Og` here just so that we have
+	# at least one job with a different optimization level so that we can
+	# overall surface more warnings.
+	export O=g
+	;;
 linux-gcc)
 	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 	;;

2.45.2.409.g7b0defb391.dirty

s/but/bit/

s/loose/lose/

I think the variable itself is fine, but for a name I think that either OPTIMIZATION or OPTIMIZATION_LEVEL would be a better pick. We have some other lengthy environment varible names so I don't think its too bad.

Otherwise, this looks good to me :)

-Justin

The same knee-jerk reaction came to me.

Speaking of variants, is there any interest in migrating one or some of the existing x86-64 CI jobs to arm64 CI jobs GitHub introduced recently? I suspect that we won't find any endianness bugs (I expect they are configured to do little endian just like everybody else) and there may no longer be lurking unaligned read bugs (but "git log --grep=unaligned" finds surprising number of them we have seen and fixed), so the returns may be very small.

I do something similar, but my $(O) replaces the whole -O2 thing, so I can say something silly like

    make O="-O2 -g -Wall"

The latter is much more preferrable. It is too easy to stomp on people's established workflow that already uses that variable for other purposes or expects slightly different syntax.

Note that we already run arm64 via GitLab's macOS runners. That's not Linux of course, but I guess that any architectural issues should still be caught by that.

Not to say that we shouldn't adapt GitHub.

Patrick

We're unlikely to break existing workflows though if we name this variable something like OPTIMIZATION_LEVEL. The workflows of both you and Peff would keep on working without any modification. Also, doesn't the fact that both of you have similar workarounds hint that this is something that other devs might want to have, as well?

We could also generalize this a bit and introduce CFLAGS_APPEND instead. Optimization levels are last-one-wins anyway, so people can use that to append their own flags without overriding existing values. It would also mean that we can avoid repeating the CFLAGS that we already have in our Makefile in our CI scripts.

I'll send a version along these lines. If people are still unhappy with that direction, then I'll give up and just repeat the whole CFLAGS in "ci/run-build-and-tests.sh".

Patrick

Hi,

this is the third version of this patch series that adapts one of our CI jobs to compile with -Og for improved coverage of warnings.

Changes compared to v2:

- Typo fixes for commit messages.

- Replaced the O variable with CFLAGS_APPEND. If that isn't palatable to folks then I'll drop this altogether and will inline it into the CI script, duplicating the default CFLAGS there.

- Start compiling with V=1 so that the change can actually be seen. It also shouldn't clutter the job output too much given that the build is in a collapsible section on both GitHub and GitLab.

Patrick

Patrick Steinhardt (4):
ci: fix check for Ubuntu 20.04
Makefile: add ability to append to CFLAGS and LDFLAGS
ci: compile code with V=1
ci: compile "linux-gcc-default" job with -Og

 Makefile                  |  2 ++
 ci/lib.sh                 |  2 +-
 ci/run-build-and-tests.sh | 11 ++++++++++-
 3 files changed, 13 insertions(+), 2 deletions(-)

Range-diff against v2:

1:  f91004a438 = 1:  70fa755b4f ci: fix check for Ubuntu 20.04
-:  ---------- > 2:  d68539834f Makefile: add ability to append to CFLAGS and LDFLAGS
-:  ---------- > 3:  a3dfb7092a ci: compile code with V=1
2:  bdf0e40a77 ! 4:  c7b5b62d9c ci: compile "linux-gcc-default" job with -Og

@@ Commit message

We have recently noticed that our CI does not always notice variables that may be used uninitialized. While it is expected that compiler
- warnings aren't perfect, this one was a but puzzling because it was
+ warnings aren't perfect, this one was a bit puzzling because it was
rather obvious that the variable can be uninitialized.

Many compiler warnings unfortunately depend on the optimization level
@@ Commit message
Instead, adapt the "linux-gcc-default" job to compile with -Og. This job is chosen because it uses the "ubuntu:latest" image and should thus have a comparatively recent compiler toolchain, and because we have
- other jobs that use "ubuntu:latest" so that we do not loose coverage for
+ other jobs that use "ubuntu:latest" so that we do not lose coverage for
warnings diagnosed only on -O2 level.

To make it easier to set up the optimization level in our CI, add
@@ Commit message

- ## Makefile ##
-@@ Makefile: endif
- # tweaked by config.* below as well as the command-line, both of
- # which'll override these defaults.
- # Older versions of GCC may require adding "-std=gnu99" at the end. --CFLAGS = -g -O2 -Wall
-+O ?= 2
-+CFLAGS = -g -O$(O) -Wall
- LDFLAGS =
- CC_LD_DYNPATH = -Wl,-rpath,
- BASIC_CFLAGS = -I.
-
## ci/run-build-and-tests.sh ##
@@ ci/run-build-and-tests.sh: esac
run_tests=t
@@ ci/run-build-and-tests.sh: esac
+ # -O2 will mask others. We thus use -Og here just so that we have
+ # at least one job with a different optimization level so that we can
+ # overall surface more warnings.
-+ export O=g
++ export CFLAGS_APPEND=-Og
+ ;;
linux-gcc)
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

2.45.2.436.gcd77e87115.dirty

In 5ca0c455f1 (ci: fix Python dependency on Ubuntu 24.04, 2024-05-06), we made the use of Python 2 conditional on whether or not the CI job runs Ubuntu 20.04. There was a brown-paper-bag-style bug though, where the condition forgot to invoke the test builtin. The result of it is that the check always fails, and thus all of our jobs run with Python 3 by accident.

Fix this.

---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1f4059b1b8..814578ffc6 100755
@@ -328,7 +328,7 @@ ubuntu-*)
 	# Python 2 is end of life, and Ubuntu 23.04 and newer don't actually
 	# have it anymore. We thus only test with Python 2 on older LTS
 	# releases.
-	if "$distro" = "ubuntu-20.04"
+	if test "$distro" = "ubuntu-20.04"
 	then
 		PYTHON_PACKAGE=python2
 	else

2.45.2.436.gcd77e87115.dirty

There are some usecases where we may want to append CFLAGS to the default CFLAGS set by Git. This could for example be to enable or disable specific compiler warnings or to change the optimization level that code is compiled with. This cannot be done without overriding the complete CFLAGS value though and thus requires the user to redeclare the complete defaults used by Git.

Introduce a new variable CFLAGS_APPEND that gets appended to the default value of CFLAGS. As compiler options are last-one-wins, this fulfills both of the usecases mentioned above. It's also common practice across many other projects to have such a variable.

While at it, also introduce a matching LDFLAGS_APPEND variable. While there isn't really any need for this variable as there are no default LDFLAGS, users may expect this variable to exist, as well.

Note that we have to use the override directive here such that the flags get appended when compiling with make CFLAGS=x CFLAGS_APPEND=y.

---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

index 2f5f16847a..b5252bed3d 100644
@@ -1359,7 +1359,9 @@ endif
 # which'll override these defaults.
 # Older versions of GCC may require adding "-std=gnu99" at the end.
 CFLAGS = -g -O2 -Wall
+override CFLAGS += $(CFLAGS_APPEND)
 LDFLAGS =
+override LDFLAGS += $(LDFLAGS_APPEND)
 CC_LD_DYNPATH = -Wl,-rpath,
 BASIC_CFLAGS = -I.
 BASIC_LDFLAGS =

2.45.2.436.gcd77e87115.dirty

It's hard to see which compiler flags our CI uses because we do not compile with V=1, and thus we only see beautified output. This information can be important at times though and may help developers to see what's going on in a CI job. Furthermore, both GitLab and GitHub have sections in their output such that the build output can be neatly hidden away, so it doesn't clutter the output, either.

Compile code with V=1 to improve debuggability.

---
 ci/run-build-and-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 98dda42045..2aaaf40f94 100755
@@ -48,7 +48,7 @@ pedantic)
 	;;
 esac
 
-group Build make
+group Build make V=1
 if test -n "$run_tests"
 then
 	group "Run tests" make test ||

2.45.2.436.gcd77e87115.dirty

We have recently noticed that our CI does not always notice variables that may be used uninitialized. While it is expected that compiler warnings aren't perfect, this one was a bit puzzling because it was rather obvious that the variable can be uninitialized.

Many compiler warnings unfortunately depend on the optimization level used by the compiler. While -O0 for example will disable a lot of warnings altogether because optimization passes go away, -O2, which is our default optimization level used in CI, may optimize specific code away or even double down on undefined behaviour. Interestingly, this specific instance that triggered the investigation does get noted by GCC when using -Og.

While we could adapt all jobs to compile with -Og now, that would potentially mask other warnings that only get diagnosed with -O2. Instead, adapt the "linux-gcc-default" job to compile with -Og. This job is chosen because it uses the "ubuntu:latest" image and should thus have a comparatively recent compiler toolchain, and because we have other jobs that use "ubuntu:latest" so that we do not lose coverage for warnings diagnosed only on -O2 level.

To make it easier to set up the optimization level in our CI, add support in our Makefile to specify the level via an environment variable.

---
 ci/run-build-and-tests.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

index 2aaaf40f94..e5fbe7f531 100755
@@ -13,6 +13,15 @@ esac
 run_tests=t
 
 case "$jobname" in
+linux-gcc-default)
+	# Warnings generated by compilers are unfortunately specific to the
+	# optimization level. With `-O0`, many warnings won't be shown at all,
+	# whereas the optimizations performed by our default optimization level
+	# `-O2` will mask others. We thus use `-Og` here just so that we have
+	# at least one job with a different optimization level so that we can
+	# overall surface more warnings.
+	export CFLAGS_APPEND=-Og
+	;;
 linux-gcc)
 	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 	;;

2.45.2.436.gcd77e87115.dirty

The more the merrier and it is nice to know we have extra variety over there.

;-)

Yeah, being more explicit is always good.

Yup, Peff's $(O) cannot serve as such, but my $(O) is already being used as such. Naming the variable that gives additional CFLAGS as such is probably a good way to go.

Thanks.

It seems that we are already triggering for 'seen'. I haven't checked if that is a false positive or a real problem, though.

https://github.com/git/git/actions/runs/9407652417/job/25913907166#step:4:139

pack-mtimes.c: In function ‘load_pack_mtimes_file’:
Error: pack-mtimes.c:89:25: ‘mtimes_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

     89 |                         munmap(data, mtimes_size);
        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~
  cc1: all warnings being treated as errors
  make: *** [Makefile:2757: pack-mtimes.o] Error 1
  make: *** Waiting for unfinished jobs....
  pack-revindex.c: In function ‘load_revindex_from_disk’:
  Error: pack-revindex.c:260:25: ‘revindex_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
    260 |                         munmap(data, revindex_size);
        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~

cc1: all warnings being treated as errors
make: *** [Makefile:2757: pack-revindex.o] Error 1
cat: exit.status: No such file or directory

The use on line 89 is guarded with "if (data)" and data can become non-NULL only after mtimes_size is computed, so this is benign.

They have excuse for a false positive because the warning is about "maybe" uninitialized, but that does not help our annoyance factor X-<.

This follows exactly the same pattern established by the other one (or perhaps the other one copied from here). It is another false positive.

I am not sure what the right fix would be. For example, if we were interested in avoiding to incur too much resources for revindex, we might do something like this

@@ -258,6 +258,8 @@ static int load_revindex_from_disk(char *revindex_name,
 	if (ret) {
 		if (data)
 			munmap(data, revindex_size);
+		fprintf(stderr, "would have fit %d revindex in 10MB\n",
+			10 * 1024 * 1024 / revindex_size);
 	} else {
 		*len_p = revindex_size;
 		*data_p = (const uint32_t *)data;

without even guarding with "if (data)".

If we "initialize" revindex_size to a meaningless dummy value like 0 like the attached would _hide_ such a real bug from the compiler, so I dunno.

@@ -206,7 +206,7 @@ static int load_revindex_from_disk(char *revindex_name,
int fd, ret = 0;
struct stat st;
void *data = NULL;
- size_t revindex_size;
+ size_t revindex_size = 0;
struct revindex_header *hdr;

if (git_env_bool(GIT_TEST_REV_INDEX_DIE_ON_DISK, 0))

I've taken the "Python 2 - missing 'test'" on a separate topic and will merge it down to 'next' and 'master' to fast track.

The "override CFLAGS += CFLAGS_APPEND" thing looks good.

I am not sure how annoying people will find the V=1 output. It is irrelevant that it is in a collapsible section. What matters is if it helps those who *need* to expand that collapsible section to take a look, or if it clutteres what they have to wade through.

When studying a build failure, I rarely found the exact command line given by V=1 helpful, but YMMV---while I am not 100% convinced, let's take the series as-is, because not losing information may sometimes help even when we need to visually filter out extra clutter.

Thanks.

One of the reasons I used the very short "O" for mine is that I often specify it by hand. I actually leave it as O=0 by default, since the majority of my builds are about developing and debugging (so speed of compilation is much more important than speed of the resulting executable). And then when I am interested in performance, I run "make O=2".

So OPTIMIZATION_LEVEL defeats the purpose. ;)

I have something like this in my config.mak, too. ;) But I call it EXTRA_CFLAGS. That seems less grammatically awkward to me than CFLAGS_APPEND, but that may be entering bikeshed territory.

And you can tell from the length name that I do not use it all that often.

-Peff

Another way to do this is just:

index 2f5f16847a..9cd3b252ff 100644
@@ -1446,8 +1446,8 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
 ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)
 endif
 
-ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)
-ALL_LDFLAGS = $(LDFLAGS)
+ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_APPEND)
+ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)
 
 ifdef SANITIZE
 SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))

I can't think offhand of any way that your override would not do the right thing, but:

- this is roughly the same problem faced by DEVELOPER_CFLAGS, etc, so handling it in the same way makes sense to me

- I always get nervous around make features like "override", as there are sometimes corner cases lurking

-Peff

I had the same thought. I have used V=1 for debugging, but usually debugging Makefile changes locally (i.e., why is my option not being passed correctly). I don't think I've ever wanted it for a CI run.

And I do think people see the output. It may be in a collapsible section on the site, but:

- you'd uncollapse that section if there is a build failure, and now your error messages are that much harder to find

- if you look at the output outside of the site, you'll see the uncollapsed sections. And I usually view them in a local pager using curl[1].

I guess I won't know until I see it in action, but I have a pretty strong suspicion that it will be annoying.

-Peff

[1] For the curious, I use the script below to grab the logs from the last failed workflow run. It's pretty old, and I won't be surprised if there's a less ugly way to do it with the "gh" CLI tool or similar these days.

-- >8 --
repo=peff/git
GITHUB_TOKEN=...your.pat.here...

actions() {

  curl -sL \
    -H 'Accept: application/vnd.github.v3+json' \
    -H "Authorization: token $GITHUB_TOKEN" \
    "https://api.github.com/repos/$repo/actions/$1"
}

run=$(
actions runs?status=failure |
jq -r '.workflow_runs | .[] | .id' |
head -1
)

actions runs/$run/jobs?per_page=100 |
jq -r '.jobs | .[] | select(.conclusion == "failure") | "\(.id) \(.name)"' | while read id name; do

  {
    echo "==> $name"
    actions jobs/$id/logs

} | less '+/not.ok|##\[error\]'
done

https://github.com/git/git/actions/runs/9424299208/job/25964282150#step:6:573

I _knew_ that this run will fail compiling the updated timestamp parsing logic in date.c but it still took me a while to find the exact error.

I typed "date.o" in the search box, which showed 5 hits (first two are false hits to fuzz-date.o and test-date.o), with

3rd hit on l.566 "gcc -o date.o ... long long command line" 4th hit on l.594 "Makefile:2758: recipe for target 'date.o' failed" 5th hit on l.595 "make: *** [date.o] Error 1"

Nitice that the error message with "date.c" is on 571 but with each line being very bloated to around 10 physical lines on screen, it is very far from either 3rd or 4th hit.

So, this time it was annoying. But I suspect I'd be praising the wisdom of using V=1 if I were hunting for some breakage caused by tweaks in command line generation that broke the build or something, so I dunno.

I'll just drop this patch for now.

Patrick

Hi,

this is the fourth version of my patch series that starts to compile with -Og in one of our CI jobs. This is done to surface more warnings from the compiler that might not show with other optimization levels.

Changes compared to v3:

- Remove the first patch as Junio has moved it into a separate topic so that it can be fast-tracked.

- Drop the patch that changes CI jobs to compile with V=1.

- Drop the override statement and instead implement CFLAGS_APPEND and LDFLAGS_APPEND via ALL_CFLAGS and ALL_LDFLAGS, respectively, as suggested by Peff.

The range diff is not all that useful for this version, and the patch series is tiny, so I dropped it.

Thanks!

Patrick

Patrick Steinhardt (2):
Makefile: add ability to append to CFLAGS and LDFLAGS
ci: compile "linux-gcc-default" job with -Og

 Makefile                  | 4 ++--
 ci/run-build-and-tests.sh | 9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

2.45.2.436.gcd77e87115.dirty

There are some usecases where we may want to append CFLAGS to the default CFLAGS set by Git. This could for example be to enable or disable specific compiler warnings or to change the optimization level that code is compiled with. This cannot be done without overriding the complete CFLAGS value though and thus requires the user to redeclare the complete defaults used by Git.

Introduce a new variable CFLAGS_APPEND that gets appended to the default value of CFLAGS. As compiler options are last-one-wins, this fulfills both of the usecases mentioned above. It's also common practice across many other projects to have such a variable.

While at it, also introduce a matching LDFLAGS_APPEND variable. While there isn't really any need for this variable as there are no default LDFLAGS, users may expect this variable to exist, as well.

---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 2f5f16847a..9cd3b252ff 100644
@@ -1446,8 +1446,8 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
 ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)
 endif
 
-ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)
-ALL_LDFLAGS = $(LDFLAGS)
+ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_APPEND)
+ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)
 
 ifdef SANITIZE
 SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))

2.45.2.436.gcd77e87115.dirty

We have recently noticed that our CI does not always notice variables that may be used uninitialized. While it is expected that compiler warnings aren't perfect, this one was a bit puzzling because it was rather obvious that the variable can be uninitialized.

Many compiler warnings unfortunately depend on the optimization level used by the compiler. While -O0 for example will disable a lot of warnings altogether because optimization passes go away, -O2, which is our default optimization level used in CI, may optimize specific code away or even double down on undefined behaviour. Interestingly, this specific instance that triggered the investigation does get noted by GCC when using -Og.

While we could adapt all jobs to compile with -Og now, that would potentially mask other warnings that only get diagnosed with -O2. Instead, adapt the "linux-gcc-default" job to compile with -Og. This job is chosen because it uses the "ubuntu:latest" image and should thus have a comparatively recent compiler toolchain, and because we have other jobs that use "ubuntu:latest" so that we do not lose coverage for warnings diagnosed only on -O2 level.

To make it easier to set up the optimization level in our CI, add support in our Makefile to specify the level via an environment variable.

---
 ci/run-build-and-tests.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

index 98dda42045..40106c6c36 100755
@@ -13,6 +13,15 @@ esac
 run_tests=t
 
 case "$jobname" in
+linux-gcc-default)
+	# Warnings generated by compilers are unfortunately specific to the
+	# optimization level. With `-O0`, many warnings won't be shown at all,
+	# whereas the optimizations performed by our default optimization level
+	# `-O2` will mask others. We thus use `-Og` here just so that we have
+	# at least one job with a different optimization level so that we can
+	# overall surface more warnings.
+	export CFLAGS_APPEND=-Og
+	;;
 linux-gcc)
 	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 	;;

2.45.2.436.gcd77e87115.dirty

Agreed, this is much nicer. Will adapt, thanks!

Patrick

I would say it is not just "potentially". It is likely that higher optimization levels will diagnose breakage more correctly, while lower optimization levels may trigger warnings more often. I suspect a large part of those that lower optimization levels alone find may be due to false positives.

So, ... I had a strong knee-jerk reaction against "While we could adapt all" with "why would anybody sane would even want to do such a thing in the first place?" If it were phrased like "as we have plenty of jobs compiling with -O2, we can afford to switch a few of them to different optimization levels, and we may find bugs that the exiting level did not notice, as the compilers' understanding of the data and control flow is affected by these -O levels", I would have understood it, and I think that is what happens in this patch.

I just tried compiling pack-mtimes.c (untouced between maint and seen) with -Og using gcc-11, gcc-12, and gcc-13, using the exact copy of the command line with V=1 output I see at the GitHub CI [*1*] (after all, this was a good use case, even though it was dropped from this round ;-)

The earlier versions of the compiler mark the use of mtimes_size that is guarded in "if (data)" as maybe-uninitialized:

        pack-mtimes.c: In function ‘load_pack_mtimes_file’:
        pack-mtimes.c:89:25: error: ‘mtimes_size’ may be used uninitialized [-Werror=maybe-uninitialized]
           89 |                         munmap(data, mtimes_size);
              |                         ^~~~~~~~~~~~~~~~~~~~~~~~~
        pack-mtimes.c:32:16: note: ‘mtimes_size’ was declared here
           32 |         size_t mtimes_size, expected_size;
              |                ^~~~~~~~~~~
        cc1: all warnings being treated as errors
        make: *** [Makefile:2750: pack-mtimes.o] Error 1

But as I already reported elsewhere, this is a false positive. The place "data" can possibly become non-NULL happens only after the control passes the place mtimes_size get assigned a meaningful value. If the control reached here and data were non-NULL, the control flow guarantees that mtimes_size has been assigned xsize_t(st.st_size) and cannot be uninitialized.

We can squelch the warning by initializing the variable to a meaningless value, like 0, but I consider that such a change makes the resulting code a lot worse than the original, and that is not because I do not want an extra and meaningless assignment emitted [*2*] in the resulting binary whose performance impact cannot be measured by anybody.

It is bad because it defeats efforts by compilers that understand the data flow a bit better to diagnose a true "used uninitialized" bugs we may introduce in the future. Adding such a workaround goes against the longer-term health of the codebase.

For example, I could add another use of mtimes_size that is not guarded by "if (data)" right next to it, i.e.

        @@ -87,6 +87,7 @@ static int load_pack_mtimes_file(char *mtimes_file,
                if (ret) {
                        if (data)
                                munmap(data, mtimes_size);
        +		printf("debug %d\n", (int)mtimes_size);
                } else {
                        *len_p = mtimes_size;
                        *data_p = data;

gcc-13 does notice that we are now truly using the variable uninitialized and flags it for us (gcc-12 also does notice but the important difference is that gcc-13 refrained from warning at the safe use of the variable that is guarded by "if (data)").

Once we initialize the variable to meaninless 0 as a workaround, however, the compiler cannot help us with the "used uninitialized" warning, and there is no "the program uses the variable what was initialized with meaningless value before it get set a meaningful value" warning, unfortunately X-<. And that, not an extra assignment, is why such a workaround goes against the longer-term health of the codebase.

By the way, it is not just pack-mtimes.c [*3*]; there are quite a few obvious false positives, e.g.

builtin/branch.c: In function ‘print_current_branch_name’:
builtin/branch.c:509:17: error: ‘shortname’ may be used uninitialized [-Werror=maybe-uninitialized]

  509 |                 puts(shortname);
      |                 ^~~~~~~~~~~~~~~

where after skip_prefix() computed shortname like so:

	else if (skip_prefix(refname, "refs/heads/", &shortname))
		puts(shortname);

and the compiler at that low optimization level does not even notice that the static inline function skip_prefix() never returns true without updating its third parameter to a valid pointer X-<.

And adding a workaround for each and every uses of variable that are set by skip_prefix() like so? I am not sure if that is a good move

If you saw some diagnosis that "-Og" made but not with higher optimization levels (with better understanding of the data and control flow on the compiler's part), I am somewhat curious.

With gcc-13 everything seems to pass locally for me, so I personally do not mind this patch, but from what I've seen "gcc-12 -Wall -Og" so far, especially given that GitHub CI (and GitLab CI as well? otherwise you wouldn't be sending this change, I would imagine) seems to use a version of compiler whose "-Og -Wall" notices and flags these "bugs", I am not sure if we want to enable this option for us. At least, -Werror and more stupid compilers is not a mix I would want to live with and I am sure I would not want to add workaround for such a mix to make our code worse (to end users and builders who want to use older comiplers with different optimization levels, we can tell them not to enable -Werror in their build).

Also, with "-Os", we seem to find different set of "bugs" that "-Og" did not see. For example, "gcc-13 -Os -Wall" flags that ppid may be uninitialized in compat/linux/procinfo.c:push_ancestry_name(), but that is because it does not read stat_parent_pid() [*4*] and realize that the only time ppid is left unset is when the function returns -1:

        static void push_ancestry_name(struct strvec *names, pid_t pid)
        {
                int ppid;

                if (stat_parent_pid(pid, &name, &ppid) < 0)
                        goto cleanup;
                ...
                if (ppid)
                        push_ancestry_name(names, ppid);
        cleanup:
                strbuf_release(&name);
		...

It is the same pattern as how "gcc-12 -Og" mishandles skip_prefix(). Another one "-Os" found is in builtin/fast-import.c:file_change_m() where the compiler does not notice that parse_mode() never returns non-NULL without setting "mode". It is exactly the same pattern.

[Footnotes]

*1* https://github.com/git/git/actions/runs/9432273715/job/25981831882#step:4:132

*2* We used to do the (IIRC GCC only) trick to initialize the
variable to itself, i.e.

@@ -29,7 +29,7 @@ static int load_pack_mtimes_file(char *mtimes_file,
int fd, ret = 0;
struct stat st;
uint32_t *data = NULL;
- size_t mtimes_size, expected_size;
+ size_t mtimes_size = mtimes_size, expected_size;
struct mtimes_header header;

fd = git_open(mtimes_file);

to work around stupid compilers that flag a variable whose use is safe as potentially-used-uninitialized but these days we got rid of the trick as some other compiler did not like it (for legitimate reasons). Using the trick is not any better than initializing the variable to a meaningless value, like 0, as both will prevent smarter compilers from noticing real bugs (after all, that is the point of the workaround--not to flag use of variable the compiler thinks uninitialized as a bug).

*3* I was planning to send "these are horrible workarounds to work
around the recent -Og build failures that has a huge negative consequences in the longer term" patch on top of your series.

But I do not think it is a good idea to add such a huge number of workarounds for various optimization levels. If we could add -Og with -Werror disabled, those who may be interested in sifting through false positives to find real errors may be able to do so without breaking build for others.

*4* As "-Os" is for smaller code, I would imagine that it didn't
try to auto inline the function. Come to think of it, "-Og" is to forbid optimizations that interfere with debugging, and code inlining may be considered as one, which would certainly explain why it has problem with skip_prefix().

These "workarounds" are to mark variables that are used after initialized, but some compilers with lower optimization levels cannot see and report "used uninitialized".

This set targets "gcc-12 -Og". For the reason why this is a wrong thing to do for longer-term code health, see

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

---

* Even though I said I won't do the actual patch, since I had to gauge the extent of damage, I ended up doing so anyways.

As I explained already, the size of this patch, i.e. number of places that need the workaround, does not really matter. What is horrible is how each of these workaround will hide real bugs we may introduce in the future from the compilers.

 builtin/branch.c          | 2 +-
 builtin/fast-import.c     | 4 ++--
 builtin/repack.c          | 2 +-
 fetch-pack.c              | 2 +-
 http-backend.c            | 2 +-
 http.c                    | 2 +-
 pack-mtimes.c             | 2 +-
 pack-revindex.c           | 2 +-
 refs/packed-backend.c     | 2 +-
 reftable/stack.c          | 2 +-
 remote-curl.c             | 4 ++--
 t/helper/test-ref-store.c | 2 +-
 trailer.c                 | 4 ++--
 13 files changed, 16 insertions(+), 16 deletions(-)

index 48cac74f97..1f4de92e05 100644
@@ -504,7 +504,7 @@ static void print_current_branch_name(void)
 	int flags;
 	const char *refname = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
 						      "HEAD", 0, NULL, &flags);
-	const char *shortname;
+	const char *shortname = shortname;
 	if (!refname)
 		die(_("could not resolve HEAD"));
 	else if (!(flags & REF_ISSYMREF))

index ea5ae5196d..6d53e42011 100644
@@ -1866,7 +1866,7 @@ static void parse_original_identifier(void)
 
 static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)
 {
-	const char *data;
+	const char *data = data;
 	strbuf_reset(sb);
 
 	if (!skip_prefix(command_buf.buf, "data ", &data))

@@ -2807,7 +2807,7 @@ static void parse_new_commit(const char *arg)
 static void parse_new_tag(const char *arg)
 {
 	static struct strbuf msg = STRBUF_INIT;
-	const char *from;
+	const char *from = from;
 	char *tagger;
 	struct branch *s;
 	struct tag *t;

index 7608430a37..e4752aae3f 100644
@@ -1106,7 +1106,7 @@ static int write_cruft_pack(const struct pack_objects_args *args,
 
 static const char *find_pack_prefix(const char *packdir, const char *packtmp)
 {
-	const char *pack_prefix;
+	const char *pack_prefix = pack_prefix;
 	if (!skip_prefix(packtmp, packdir, &pack_prefix))
 		die(_("pack prefix %s does not begin with objdir %s"),
 		    packtmp, packdir);

index eba9e420ea..f8f318742f 100644
@@ -1517,7 +1517,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,
 
 	process_section_header(reader, "shallow-info", 0);
 	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
-		const char *arg;
+		const char *arg = arg;
 		struct object_id oid;
 
 		if (skip_prefix(reader->line, "shallow ", &arg)) {

index 5b4dca65ed..eecba3b44f 100644
@@ -259,7 +259,7 @@ static void http_config(void)
 
 static struct rpc_service *select_service(struct strbuf *hdr, const char *name)
 {
-	const char *svc_name;
+	const char *svc_name = svc_name;
 	struct rpc_service *svc = NULL;
 	int i;
 

index 2dea2d03da..c45f91fa4e 100644
@@ -2156,7 +2156,7 @@ static int update_url_from_redirect(struct strbuf *base,
 				    const char *asked,
 				    const struct strbuf *got)
 {
-	const char *tail;
+	const char *tail = tail;
 	size_t new_len;
 
 	if (!strcmp(asked, got->buf))

index cdf30b8d2b..49598d4749 100644
@@ -29,7 +29,7 @@ static int load_pack_mtimes_file(char *mtimes_file,
 	int fd, ret = 0;
 	struct stat st;
 	uint32_t *data = NULL;
-	size_t mtimes_size, expected_size;
+	size_t mtimes_size = mtimes_size, expected_size;
 	struct mtimes_header header;
 
 	fd = git_open(mtimes_file);

index fc63aa76a2..7120bed5b1 100644
@@ -206,7 +206,7 @@ static int load_revindex_from_disk(char *revindex_name,
 	int fd, ret = 0;
 	struct stat st;
 	void *data = NULL;
-	size_t revindex_size;
+	size_t revindex_size = revindex_size;
 	struct revindex_header *hdr;
 
 	if (git_env_bool(GIT_TEST_REV_INDEX_DIE_ON_DISK, 0))

index c4c1e36aa2..25321a5758 100644
@@ -677,7 +677,7 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)
 
 	/* If the file has a header line, process it: */
 	if (snapshot->buf < snapshot->eof && *snapshot->buf == '#') {
-		char *tmp, *p, *eol;
+		char *tmp, *p = p, *eol;
 		struct string_list traits = STRING_LIST_INIT_NODUP;
 
 		eol = memchr(snapshot->buf, '\n',

index 737591125e..aa1851fd73 100644
@@ -1231,7 +1231,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n,
 					  uint8_t factor)
 {
 	struct segment seg = { 0 };
-	uint64_t bytes;
+	uint64_t bytes = bytes;
 	size_t i;
 
 	if (!factor)

index 827bd848ae..4f9808c63f 100644
@@ -1265,7 +1265,7 @@ static void parse_fetch(struct strbuf *buf)
 	int alloc_heads = 0, nr_heads = 0;
 
 	do {
-		const char *p;
+		const char *p = p;
 		if (skip_prefix(buf->buf, "fetch ", &p)) {
 			const char *name;
 			struct ref *ref;

@@ -1428,7 +1428,7 @@ static void parse_push(struct strbuf *buf)
 	int ret;
 
 	do {
-		const char *arg;
+		const char *arg = arg;
 		if (skip_prefix(buf->buf, "push ", &arg))
 			strvec_push(&specs, arg);
 		else

index ad24300170..7d3870fe76 100644
@@ -66,7 +66,7 @@ static unsigned int arg_flags(const char *arg, const char *name,
 
 static const char **get_store(const char **argv, struct ref_store **refs)
 {
-	const char *gitdir;
+	const char *gitdir = gitdir;
 
 	if (!argv[0]) {
 		die("ref store required");

index 72e5136c73..5056c16644 100644
@@ -508,11 +508,11 @@ static int git_trailer_config(const char *conf_key, const char *value,
 			      const struct config_context *ctx UNUSED,
 			      void *cb UNUSED)
 {
-	const char *trailer_item, *variable_name;
+	const char *trailer_item = trailer_item, *variable_name;
 	struct arg_item *item;
 	struct conf_info *conf;
 	char *name = NULL;
-	enum trailer_info_type type;
+	enum trailer_info_type type = type;
 	int i;
 
 	if (!skip_prefix(conf_key, "trailer.", &trailer_item))

2.45.2-445-g1b76f06508

These "workarounds" are to mark variables that are used after initialized, but some compilers with lower optimization levels cannot see and report "used uninitialized".

This set targets "gcc-13 -Os". For the reason why this is a wrong thing to do for longer-term code health, see

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

---
 builtin/fast-import.c   | 2 +-
 compat/linux/procinfo.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 6d53e42011..109b0facd5 100644
@@ -2320,7 +2320,7 @@ static void file_change_m(const char *p, struct branch *b)
 	static struct strbuf path = STRBUF_INIT;
 	struct object_entry *oe;
 	struct object_id oid;
-	uint16_t mode, inline_data = 0;
+	uint16_t mode = mode, inline_data = 0;
 
 	p = parse_mode(p, &mode);
 	if (!p)

index 4bb2d66227..17937906b8 100644
@@ -129,7 +129,7 @@ static int stat_parent_pid(pid_t pid, struct strbuf *name, int *statppid)
 static void push_ancestry_name(struct strvec *names, pid_t pid)
 {
 	struct strbuf name = STRBUF_INIT;
-	int ppid;
+	int ppid = ppid;
 
 	if (stat_parent_pid(pid, &name, &ppid) < 0)
 		goto cleanup;

2.45.2-445-g1b76f06508

And depending on the version of compilers, apparently even this is not enough. I do not offhand know what GitHub CI is running for linux-gcc-default (ubuntu-latest), but this gets flagged for using (try to guess which one without looking at the answer below) ...

        static int parse_count(const char *arg)
        {
                int count;

                if (strtol_i(arg, 10, &count) < 0)
                        die("'%s': not an integer", arg);
                return count;
        }

... count uninitilaized, since the compiler does not realize that strtol_i() always touches "count" unless the function returns negative, and die() never returns. Exactly the same pattern continues.

So, unless we disable -Werror, let's not continue this experiment with -Og or -Os as the damage seems to be far greater than the benefit (which I haven't seen any, but that is largely due to timezone differences---I asked "what's the real bug you found with this" a few hours ago that is past EOB in Europe).

The real bug that "-Og" would have been able to detect was reported by Peff via [1]. In this case it wasn't "-Og" that detected it, but Coverity did. But it would have been detected if we had a job that compiled with "-Og".

But now that I see the full picture of this with different compiler options I have to agree that this is not really worth it. Especially not given that Coverity is able to detect such cases, even though that only happens retroactively after a topic has landed.

Let's drop this experiment.

Patrick

[1]: [email protected]

Thanks. It is curious that you singled out "-Og" here, but with the usual "-O2" optimization level, it does not seem to get noticed.

OK, but let's keep the CFLAGS_APPEND one ;-)

Sure, let's do that. I assume you'll just cherry-pick that single commit?

Patrick

Ah, just spotted that you already did. Thanks!

Patrick

By the way, I do not know if any compiler gives us such a feature, but if the trick to squelch a false positive were finer grained, I would have been much more receptive to the idea of building with different optimization level, allowing a bit more false positives.

The workaround everybody jumps at is to initialize the variable to a meaningless value (like 0) and I have explained why it is suboptimal already. But if we can tell less intelligent compilers "we know our use of this variable AT THIS POINT is safe", e.g. by annotating the above snippet of the code, perhaps like this:

                if (ret) {
                        if (data)
				/* -Wno-uninitialized (mtimes_size) */
                                munmap(data, mtimes_size);
			printf("debug %d\n", (int)mtimes_size);

then it would be clear to the compiler that understand the annotation that inside that "if (data)" block, we know that the use of mtimes_size is not using an uninitialized variable.

For the use of the same variable on the next "debug" line, because it is outside of that "if (data)" block, the annotation should have no effect, and the compiler is free to do its own analysis and we will accept if it finds mtimes_size can be used uninitialized there. Any new use added for the same variable will not be masked by a meaningless initialization if we can use such a "workaround" to squelch false positives.

I agree that such an annotation is much more focused. It's still not foolproof, though (e.g., we might chance earlier code so that the data/mtimes_size correlation is no longer true).

I think you could do it with:

if (data)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wuninitialized"

				munmap(data, mtimes_size);

#pragma GCC diagnostic pop

which is...ugly. There's a _Pragma() operator, too, which I think would let you make a macro like:

			if (data)
				SUPPRESS("-Wuninitialized", munmap(data, mtimes_size));

which is maybe slightly less horrific? Still pretty magical though.

But if the alternative is to do none of that, and just continue to avoid looking for warnings with -Os, I prefer that.

-Peff

Oh, we are in agreement. Such effort to annotate code and tolerate inconvenience is better spent elsewhere but expertise and competence are not as fungible as I wish them to be ;-)

Thanks.