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

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

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

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