On Thu, Jun 06, 2024 at 02:52:36AM -0400, Jeff King wrote:

On Thu, Jun 06, 2024 at 08:30:34AM +0200, Patrick Steinhardt wrote:

quoted

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

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

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

quoted

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 98dda42045..e78e19e4a6 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh

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

Writing config.mak is unusual, though I guess just setting CFLAGS in the
environment isn't enough, because we set it unconditionally in the
Makefile. Doing "make CFLAGS=-Og" would work, but we'd need help from
the code that actually runs "make".

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

I do suspect the "export" is unnecessary; it should just be used by the
Makefile recipes themselves.

Your command above also loses the "-g" and "-Wall" from the default
CFLAGS. Maybe OK, since DEVELOPER=1 implies -Wall anyway, and "-g" isn't
important. But one thing I've done for a long time in my config.mak is:

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

Then you can "make O=0" or "make O=g" if you want. And I think that
setting O=g in the environment (exported) would work, as well.

Sure, I can do something along these lines.

Patrick