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:
signature.asc
Description: PGP signature
