[PATCH 0/2] ci: detect more warnings via `-Og`

[PATCH 0/2] ci: detect more warnings via `-Og`

[Patrick Steinhardt]

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

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]: <20240605100728.GA3440281@coredump.intra.peff.net>

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


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

[PATCH 1/2] ci: fix check for Ubuntu 20.04

[Patrick Steinhardt]

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

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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 1f4059b1b8..814578ffc6 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -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


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

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

[Patrick Steinhardt]

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/run-build-and-tests.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

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
 	run_tests=
 	;;
 esac
-- 
2.45.2.409.g7b0defb391.dirty


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

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

[Jeff King]

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.

-Peff

Re: [PATCH 1/2] ci: fix check for Ubuntu 20.04

[Jeff King]

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

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

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

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

[Patrick Steinhardt]

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

Re: [PATCH 1/2] ci: fix check for Ubuntu 20.04

[Patrick Steinhardt]

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

On Thu, Jun 06, 2024 at 02:53:28AM -0400, Jeff King wrote:
> On Thu, Jun 06, 2024 at 08:30:25AM +0200, Patrick Steinhardt wrote:
> 
> > 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.
> 
> 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).

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

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

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

[Jeff King]

On Thu, Jun 06, 2024 at 09:41:56AM +0200, Patrick Steinhardt wrote:

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

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

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

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.

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

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

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

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

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

[Patrick Steinhardt]

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

On Thu, Jun 06, 2024 at 04:05:52AM -0400, Jeff King wrote:
> On Thu, Jun 06, 2024 at 09:41:56AM +0200, Patrick Steinhardt wrote:
> > 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?
> 
> 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.
> 
> > 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.
> 
> 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.

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.

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

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.

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

True.

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

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

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

[PATCH v2 0/2] ci: detect more warnings via `-Og`

[Patrick Steinhardt]

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

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 <ps@pks.im>
     
      ## 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.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    + ## 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


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

[PATCH v2 1/2] ci: fix check for Ubuntu 20.04

[Patrick Steinhardt]

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

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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 1f4059b1b8..814578ffc6 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -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


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

[PATCH v2 2/2] ci: compile "linux-gcc-default" job with -Og

[Patrick Steinhardt]

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

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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

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

diff --git a/Makefile b/Makefile
index 59d98ba688..ff57c94fdf 100644
--- a/Makefile
+++ b/Makefile
@@ -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.
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 98dda42045..0f00dbd289 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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


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

Re: [PATCH v2 2/2] ci: compile "linux-gcc-default" job with -Og

[Justin Tobler]

On 24/06/06 11:31AM, 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

s/but/bit/

> 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

s/loose/lose/

> 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.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> 
> 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.

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

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

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

    make O="-O2 -g -Wall"

Re: [PATCH v2 2/2] ci: compile "linux-gcc-default" job with -Og

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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.

>  Makefile                  | 3 ++-
>  ci/run-build-and-tests.sh | 9 +++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 59d98ba688..ff57c94fdf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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.
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index 98dda42045..0f00dbd289 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -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
>  	;;

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

[Patrick Steinhardt]

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

On Thu, Jun 06, 2024 at 09:32:09AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > 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?).
> 
> 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.

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

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

Re: [PATCH v2 2/2] ci: compile "linux-gcc-default" job with -Og

[Patrick Steinhardt]

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

On Thu, Jun 06, 2024 at 10:02:04AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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.
> 
> 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.

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

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

[PATCH v3 0/4] ci: detect more warnings via `-Og`

[Patrick Steinhardt]

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

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
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    - ## 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


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

[PATCH v3 1/4] ci: fix check for Ubuntu 20.04

[Patrick Steinhardt]

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

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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 1f4059b1b8..814578ffc6 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -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


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

[PATCH v3 2/4] Makefile: add ability to append to CFLAGS and LDFLAGS

[Patrick Steinhardt]

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile b/Makefile
index 2f5f16847a..b5252bed3d 100644
--- a/Makefile
+++ b/Makefile
@@ -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


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

[PATCH v3 3/4] ci: compile code with V=1

[Patrick Steinhardt]

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

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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/run-build-and-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 98dda42045..2aaaf40f94 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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


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

[PATCH v3 4/4] ci: compile "linux-gcc-default" job with -Og

[Patrick Steinhardt]

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

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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/run-build-and-tests.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 2aaaf40f94..e5fbe7f531 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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


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

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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

> Not to say that we shouldn't adapt GitHub.

;-)

Re: [PATCH v2 2/2] ci: compile "linux-gcc-default" job with -Og

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> We're unlikely to break existing workflows though if we name this
> variable something like `OPTIMIZATION_LEVEL`.

Yeah, being more explicit is always good.

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

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.

Re: [PATCH v2 2/2] ci: compile "linux-gcc-default" job with -Og

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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

Re: [PATCH v2 2/2] ci: compile "linux-gcc-default" job with -Og

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

>   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

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

--- i/pack-revindex.c
+++ w/pack-revindex.c
@@ -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))

Re: [PATCH v3 0/4] ci: detect more warnings via `-Og`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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.

Re: [PATCH v2 2/2] ci: compile "linux-gcc-default" job with -Og

[Jeff King]

On Fri, Jun 07, 2024 at 11:45:18AM -0700, Junio C Hamano wrote:

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > We're unlikely to break existing workflows though if we name this
> > variable something like `OPTIMIZATION_LEVEL`.
> 
> Yeah, being more explicit is always good.

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

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

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

Re: [PATCH v3 2/4] Makefile: add ability to append to CFLAGS and LDFLAGS

[Jeff King]

On Fri, Jun 07, 2024 at 08:46:34AM +0200, Patrick Steinhardt wrote:

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

Another way to do this is just:

diff --git a/Makefile b/Makefile
index 2f5f16847a..9cd3b252ff 100644
--- a/Makefile
+++ b/Makefile
@@ -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

Re: [PATCH v3 0/4] ci: detect more warnings via `-Og`

[Jeff King]

On Fri, Jun 07, 2024 at 01:47:25PM -0700, Junio C Hamano wrote:

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

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

Re: [PATCH v3 2/4] Makefile: add ability to append to CFLAGS and LDFLAGS

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Another way to do this is just:
>
> diff --git a/Makefile b/Makefile
> index 2f5f16847a..9cd3b252ff 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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)

Much nicer.

> 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

Re: [PATCH v3 0/4] ci: detect more warnings via `-Og`

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jun 07, 2024 at 01:47:25PM -0700, Junio C Hamano wrote:
>
>> 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.
>
> 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.

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.

    

Re: [PATCH v3 0/4] ci: detect more warnings via `-Og`

[Patrick Steinhardt]

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

On Sat, Jun 08, 2024 at 04:12:15PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Jun 07, 2024 at 01:47:25PM -0700, Junio C Hamano wrote:
> >
> >> 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.
> >
> > 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.
> 
> 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

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

[PATCH v4 0/2] ci: detect more warnings via `-Og`

[Patrick Steinhardt]

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

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


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

[PATCH v4 1/2] Makefile: add ability to append to CFLAGS and LDFLAGS

[Patrick Steinhardt]

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

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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 2f5f16847a..9cd3b252ff 100644
--- a/Makefile
+++ b/Makefile
@@ -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


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

[PATCH v4 2/2] ci: compile "linux-gcc-default" job with -Og

[Patrick Steinhardt]

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

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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/run-build-and-tests.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 98dda42045..40106c6c36 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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


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

Re: [PATCH v3 2/4] Makefile: add ability to append to CFLAGS and LDFLAGS

[Patrick Steinhardt]

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

On Sat, Jun 08, 2024 at 12:01:28PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > Another way to do this is just:
> >
> > diff --git a/Makefile b/Makefile
> > index 2f5f16847a..9cd3b252ff 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -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)
> 
> Much nicer.

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

Patrick

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

Re: [PATCH v4 2/2] ci: compile "linux-gcc-default" job with -Og

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> While we could adapt all jobs to compile with `-Og` now, that would
> potentially mask other warnings that only get diagnosed with `-O2`.

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

[PATCH 1/2] DONTAPPLY: -Og fallout workaround

[Junio C Hamano]

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/xmqqed946auc.fsf@gitster.g/

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * 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(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 48cac74f97..1f4de92e05 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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))
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index ea5ae5196d..6d53e42011 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -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;
diff --git a/builtin/repack.c b/builtin/repack.c
index 7608430a37..e4752aae3f 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -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);
diff --git a/fetch-pack.c b/fetch-pack.c
index eba9e420ea..f8f318742f 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -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)) {
diff --git a/http-backend.c b/http-backend.c
index 5b4dca65ed..eecba3b44f 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -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;
 
diff --git a/http.c b/http.c
index 2dea2d03da..c45f91fa4e 100644
--- a/http.c
+++ b/http.c
@@ -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))
diff --git a/pack-mtimes.c b/pack-mtimes.c
index cdf30b8d2b..49598d4749 100644
--- a/pack-mtimes.c
+++ b/pack-mtimes.c
@@ -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);
diff --git a/pack-revindex.c b/pack-revindex.c
index fc63aa76a2..7120bed5b1 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -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))
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index c4c1e36aa2..25321a5758 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -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',
diff --git a/reftable/stack.c b/reftable/stack.c
index 737591125e..aa1851fd73 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -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)
diff --git a/remote-curl.c b/remote-curl.c
index 827bd848ae..4f9808c63f 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -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
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index ad24300170..7d3870fe76 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -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");
diff --git a/trailer.c b/trailer.c
index 72e5136c73..5056c16644 100644
--- a/trailer.c
+++ b/trailer.c
@@ -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

[PATCH 2/2] DONTAPPLY: -Os fallout workaround

[Junio C Hamano]

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/xmqqed946auc.fsf@gitster.g/

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/fast-import.c   | 2 +-
 compat/linux/procinfo.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 6d53e42011..109b0facd5 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -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)
diff --git a/compat/linux/procinfo.c b/compat/linux/procinfo.c
index 4bb2d66227..17937906b8 100644
--- a/compat/linux/procinfo.c
+++ b/compat/linux/procinfo.c
@@ -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

Re: [PATCH 1/2] DONTAPPLY: -Og fallout workaround

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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/xmqqed946auc.fsf@gitster.g/
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * 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(-)

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

Re: [PATCH 1/2] DONTAPPLY: -Og fallout workaround

[Patrick Steinhardt]

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

On Mon, Jun 10, 2024 at 01:05:00PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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/xmqqed946auc.fsf@gitster.g/
> >
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > ---
> >
> >  * 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(-)
> 
> 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]: 20240605100728.GA3440281@coredump.intra.peff.net

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

Re: [PATCH 1/2] DONTAPPLY: -Og fallout workaround

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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

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

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

Re: [PATCH 1/2] DONTAPPLY: -Og fallout workaround

[Patrick Steinhardt]

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

On Tue, Jun 11, 2024 at 10:30:24AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > 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.
> 
> 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

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

Re: [PATCH 1/2] DONTAPPLY: -Og fallout workaround

[Patrick Steinhardt]

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

On Wed, Jun 12, 2024 at 06:42:28AM +0200, Patrick Steinhardt wrote:
> On Tue, Jun 11, 2024 at 10:30:24AM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > > 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.
> > 
> > OK, but let's keep the CFLAGS_APPEND one ;-)
> 
> Sure, let's do that. I assume you'll just cherry-pick that single
> commit?

Ah, just spotted that you already did. Thanks!

Patrick

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

Re: [PATCH v4 2/2] ci: compile "linux-gcc-default" job with -Og

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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.

Re: [PATCH v4 2/2] ci: compile "linux-gcc-default" job with -Og

[Jeff King]

On Wed, Jun 12, 2024 at 03:11:30PM -0700, Junio C Hamano wrote:

> 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

Re: [PATCH v4 2/2] ci: compile "linux-gcc-default" job with -Og

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.