Re: [PATCH 2/2] ci: let pedantic job compile with -Og

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4891 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]