{"thread":{"id":"61594","subject":"[PATCH 0/2] ci: detect more warnings via `-Og`","startedAt":"2024-06-06T06:30:26Z","lastAt":"2024-06-13T15:47:34Z","messageCount":48,"participants":["Patrick Steinhardt","Jeff King","Justin Tobler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"496433","messageId":"cover.1717655210.git.ps@pks.im","threadId":"61594","inReplyTo":null,"subject":"[PATCH 0/2] ci: detect more warnings via `-Og`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-06T06:30:19Z","receivedAt":"2024-06-06T06:30:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series is triggered by the thread at [1], where Peff noted a\npotentially-uninitialized variable. In theory, GCC is able to diagnose\nsuch issues via `-Wmaybe-uninitialized`. But in practice, this warning\nis dependent on the optimization level used by GCC, where the issue in\nthe thread is only noticed when compiling with `-Og`.\n\nThe first patch is just a small fix to our CI I found while at it. The\nsecond patch adapts our \"pedantic\" job to compile with `-Og` so that we\noverall surface more issues. This would've catched the issue.\n\nPatrick\n\n[1]: <20240605100728.GA3440281@coredump.intra.peff.net>\n\nPatrick Steinhardt (2):\n ci: fix check for Ubuntu 20.04\n ci: let pedantic job compile with -Og\n\n ci/lib.sh | 2 +-\n ci/run-build-and-tests.sh | 9 +++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\n-- \n2.45.2.409.g7b0defb391.dirty\n\n"},{"id":"496434","messageId":"f91004a4382c95b1b87bf3d1aafc018af2b7b92e.1717655210.git.ps@pks.im","threadId":"61594","inReplyTo":"cover.1717655210.git.ps@pks.im","subject":"[PATCH 1/2] ci: fix check for Ubuntu 20.04","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-06T06:30:25Z","receivedAt":"2024-06-06T06:30:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 5ca0c455f1 (ci: fix Python dependency on Ubuntu 24.04, 2024-05-06),\nwe made the use of Python 2 conditional on whether or not the CI job\nruns Ubuntu 20.04. There was a brown-paper-bag-style bug though, where\nthe condition forgot to invoke the `test` builtin. The result of it is\nthat the check always fails, and thus all of our jobs run with Python 3\nby accident.\n\nFix this.\n\nSigned-off-by: Patrick Steinhardt \n---\n ci/lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 1f4059b1b8..814578ffc6 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -328,7 +328,7 @@ ubuntu-*)\n \t# Python 2 is end of life, and Ubuntu 23.04 and newer don't actually\n \t# have it anymore. We thus only test with Python 2 on older LTS\n \t# releases.\n-\tif \"$distro\" = \"ubuntu-20.04\"\n+\tif test \"$distro\" = \"ubuntu-20.04\"\n \tthen\n \t\tPYTHON_PACKAGE=python2\n \telse\n-- \n2.45.2.409.g7b0defb391.dirty\n\n"},{"id":"496435","messageId":"351dec4a4d5a5619e7627e11a8e674e32125125e.1717655210.git.ps@pks.im","threadId":"61594","inReplyTo":"cover.1717655210.git.ps@pks.im","subject":"[PATCH 2/2] ci: let pedantic job compile with -Og","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-06T06:30:34Z","receivedAt":"2024-06-06T06:30:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have recently noticed that our CI does not always notice variables\nthat may be used uninitialized. While it is expected that compiler\nwarnings aren't perfect, this one was a but puzzling because it was\nrather obvious that the variable can be uninitialized.\n\nMany compiler warnings unfortunately depend on the optimization level\nused by the compiler. While `-O0` for example will disable a lot of\nwarnings altogether because optimization passes go away, `-O2`, which is\nour default optimization level used in CI, may optimize specific code\naway or even double down on undefined behaviour. Interestingly, this\nspecific instance that triggered the investigation does get noted by GCC\nwhen using `-Og`.\n\nWhile we could adapt all jobs to compile with `-Og` now, that would\npotentially mask other warnings that only get diagnosed with `-O2`.\nInstead, only adapt the \"pedantic\" job to compile with `-Og`.\n\nSigned-off-by: Patrick Steinhardt \n---\n ci/run-build-and-tests.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex 98dda42045..e78e19e4a6 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -44,6 +44,15 @@ pedantic)\n \t# Don't run the tests; we only care about whether Git can be\n \t# built.\n \texport DEVOPTS=pedantic\n+\t# Warnings generated by compilers are unfortunately specific to the\n+\t# optimization level. With `-O0`, many warnings won't be shown at all,\n+\t# whereas the optimizations performed by our default optimization level\n+\t# `-O2` will mask others. We thus use `-Og` here just so that we have\n+\t# at least one job with a different optimization level so that we can\n+\t# overall surface more warnings.\n+\tcat >config.mak <<-EOF\n+\texport CFLAGS=-Og\n+\tEOF\n \trun_tests=\n \t;;\n esac\n-- \n2.45.2.409.g7b0defb391.dirty\n\n"},{"id":"496437","messageId":"20240606065236.GA646308@coredump.intra.peff.net","threadId":"61594","inReplyTo":"351dec4a4d5a5619e7627e11a8e674e32125125e.1717655210.git.ps@pks.im","subject":"Re: [PATCH 2/2] ci: let pedantic job compile with -Og","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-06T06:52:36Z","receivedAt":"2024-06-06T06:52:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 06, 2024 at 08:30:34AM +0200, Patrick Steinhardt wrote:\n\n> We have recently noticed that our CI does not always notice variables\n> that may be used uninitialized. While it is expected that compiler\n> warnings aren't perfect, this one was a but puzzling because it was\n> rather obvious that the variable can be uninitialized.\n> \n> Many compiler warnings unfortunately depend on the optimization level\n> used by the compiler. While `-O0` for example will disable a lot of\n> warnings altogether because optimization passes go away, `-O2`, which is\n> our default optimization level used in CI, may optimize specific code\n> away or even double down on undefined behaviour. Interestingly, this\n> specific instance that triggered the investigation does get noted by GCC\n> when using `-Og`.\n> \n> While we could adapt all jobs to compile with `-Og` now, that would\n> potentially mask other warnings that only get diagnosed with `-O2`.\n> Instead, only adapt the \"pedantic\" job to compile with `-Og`.\n\nHmm. This is the first time I've ever seen lower optimization levels\nproduce more warnings. It is almost always the opposite case in my\nexperience. So it's not clear to me that moving to \"-Og\" will generally\nfind more warning spots, and that this isn't a bit of a fluke.\n\nAs you note, we'll still compile with -O2 in other jobs. But isn't the\npoint of the pedantic job to enable a bunch of extra warnings that\naren't available elsewhere? We wouldn't have any coverage of those.\n\nSo for the pedantic warnings, we're left with a guess as to whether -Og\nor -O2 will yield more results. And in my experience it is probably -O2.\n\nIf we want to get coverage of -Og, I'd suggest doing it in a job that is\notherwise overlapping with another (maybe linux-TEST-vars, which I think\nis otherwise a duplicate build?).\n\n> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n> index 98dda42045..e78e19e4a6 100755\n> --- a/ci/run-build-and-tests.sh\n> +++ b/ci/run-build-and-tests.sh\n> @@ -44,6 +44,15 @@ pedantic)\n> \t# Don't run the tests; we only care about whether Git can be\n> \t# built.\n> \texport DEVOPTS=pedantic\n> +\t# Warnings generated by compilers are unfortunately specific to the\n> +\t# optimization level. With `-O0`, many warnings won't be shown at all,\n> +\t# whereas the optimizations performed by our default optimization level\n> +\t# `-O2` will mask others. We thus use `-Og` here just so that we have\n> +\t# at least one job with a different optimization level so that we can\n> +\t# overall surface more warnings.\n> +\tcat >config.mak <<-EOF\n> +\texport CFLAGS=-Og\n> +\tEOF\n\nWriting config.mak is unusual, though I guess just setting CFLAGS in the\nenvironment isn't enough, because we set it unconditionally in the\nMakefile. Doing \"make CFLAGS=-Og\" would work, but we'd need help from\nthe code that actually runs \"make\".\n\nI do suspect the \"export\" is unnecessary; it should just be used by the\nMakefile recipes themselves.\n\nYour command above also loses the \"-g\" and \"-Wall\" from the default\nCFLAGS. Maybe OK, since DEVELOPER=1 implies -Wall anyway, and \"-g\" isn't\nimportant. But one thing I've done for a long time in my config.mak is:\n\n O ?= 2\n CFLAGS = -g -Wall -O$(O)\n\nThen you can \"make O=0\" or \"make O=g\" if you want. And I think that\nsetting O=g in the environment (exported) would work, as well.\n\n-Peff\n"},{"id":"496438","messageId":"20240606065328.GB646308@coredump.intra.peff.net","threadId":"61594","inReplyTo":"f91004a4382c95b1b87bf3d1aafc018af2b7b92e.1717655210.git.ps@pks.im","subject":"Re: [PATCH 1/2] ci: fix check for Ubuntu 20.04","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-06T06:53:28Z","receivedAt":"2024-06-06T06:53:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 06, 2024 at 08:30:25AM +0200, Patrick Steinhardt wrote:\n\n> In 5ca0c455f1 (ci: fix Python dependency on Ubuntu 24.04, 2024-05-06),\n> we made the use of Python 2 conditional on whether or not the CI job\n> runs Ubuntu 20.04. There was a brown-paper-bag-style bug though, where\n> the condition forgot to invoke the `test` builtin. The result of it is\n> that the check always fails, and thus all of our jobs run with Python 3\n> by accident.\n> \n> Fix this.\n\nYikes. This looks obviously correct. Though I guess nobody noticed or\ncared that we were not using python 2? It sounds like it is a\nnice-to-have to get more coverage, but the platform in question is happy\nto use python 3).\n\n-Peff\n"},{"id":"496442","messageId":"ZmFoRPd52iqxbOTJ@tanuki","threadId":"61594","inReplyTo":"20240606065236.GA646308@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] ci: let pedantic job compile with -Og","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-06T07:41:56Z","receivedAt":"2024-06-06T07:42:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jun 06, 2024 at 02:52:36AM -0400, Jeff King wrote:\n> On Thu, Jun 06, 2024 at 08:30:34AM +0200, Patrick Steinhardt wrote:\n> \n> > We have recently noticed that our CI does not always notice variables\n> > that may be used uninitialized. While it is expected that compiler\n> > warnings aren't perfect, this one was a but puzzling because it was\n> > rather obvious that the variable can be uninitialized.\n> > \n> > Many compiler warnings unfortunately depend on the optimization level\n> > used by the compiler. While `-O0` for example will disable a lot of\n> > warnings altogether because optimization passes go away, `-O2`, which is\n> > our default optimization level used in CI, may optimize specific code\n> > away or even double down on undefined behaviour. Interestingly, this\n> > specific instance that triggered the investigation does get noted by GCC\n> > when using `-Og`.\n> > \n> > While we could adapt all jobs to compile with `-Og` now, that would\n> > potentially mask other warnings that only get diagnosed with `-O2`.\n> > Instead, only adapt the \"pedantic\" job to compile with `-Og`.\n> \n> Hmm. This is the first time I've ever seen lower optimization levels\n> produce more warnings. It is almost always the opposite case in my\n> experience. So it's not clear to me that moving to \"-Og\" will generally\n> find more warning spots, and that this isn't a bit of a fluke.\n> \n> As you note, we'll still compile with -O2 in other jobs. But isn't the\n> point of the pedantic job to enable a bunch of extra warnings that\n> aren't available elsewhere? We wouldn't have any coverage of those.\n> \n> So for the pedantic warnings, we're left with a guess as to whether -Og\n> or -O2 will yield more results. And in my experience it is probably -O2.\n> \n> If we want to get coverage of -Og, I'd suggest doing it in a job that is\n> otherwise overlapping with another (maybe linux-TEST-vars, which I think\n> is otherwise a duplicate build?).\n\nI don't think linux-TEST-vars would be a good candidate for this because\nit uses Ubuntu 20.04. Ideally, we'd want to have a test run with an\nup-to-date version of Ubuntu so that we also get a recent version of the\ncompiler toolchain.\n\nI kind of wonder whether we should revamp this pedantic job in the first\nplace. The consequence of that job is that our codebase needs to be\ncompile cleanly with `-Wpedantic`. So if that is a requirement anyway,\nwhy don't we run all jobs with `DEVOPTS=pedantic` and just drop this job\naltogether? This may surface some additional warnings on platforms where\nwe currently don't set that, but is that a bad thing?\n\nThe only downside I can think of is that we stop compiling on Fedora,\nwhich may have a more up-to-date GCC version than Ubuntu. But if the\ngoal of this job was to _really_ get an up-to-date compiler toolchain,\nthen we should rather pick a rolling release distro like Arch. Otherwise\nI find this to be of dubious benefit.\n\nIf we merge it into the other jobs, then I'd just pick any random job\nthat uses \"ubuntu:latest\" like \"linux-gcc-default\" to compile with\n`-Og`.\n\n> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n> > index 98dda42045..e78e19e4a6 100755\n> > --- a/ci/run-build-and-tests.sh\n> > +++ b/ci/run-build-and-tests.sh\n> > @@ -44,6 +44,15 @@ pedantic)\n> > \t# Don't run the tests; we only care about whether Git can be\n> > \t# built.\n> > \texport DEVOPTS=pedantic\n> > +\t# Warnings generated by compilers are unfortunately specific to the\n> > +\t# optimization level. With `-O0`, many warnings won't be shown at all,\n> > +\t# whereas the optimizations performed by our default optimization level\n> > +\t# `-O2` will mask others. We thus use `-Og` here just so that we have\n> > +\t# at least one job with a different optimization level so that we can\n> > +\t# overall surface more warnings.\n> > +\tcat >config.mak <<-EOF\n> > +\texport CFLAGS=-Og\n> > +\tEOF\n> \n> Writing config.mak is unusual, though I guess just setting CFLAGS in the\n> environment isn't enough, because we set it unconditionally in the\n> Makefile. Doing \"make CFLAGS=-Og\" would work, but we'd need help from\n> the code that actually runs \"make\".\n\nYeah, I went the easy route because setting it via the environment is\nnot enough, as you correctly mention.\n\n> I do suspect the \"export\" is unnecessary; it should just be used by the\n> Makefile recipes themselves.\n> \n> Your command above also loses the \"-g\" and \"-Wall\" from the default\n> CFLAGS. Maybe OK, since DEVELOPER=1 implies -Wall anyway, and \"-g\" isn't\n> important. But one thing I've done for a long time in my config.mak is:\n> \n> O ?= 2\n> CFLAGS = -g -Wall -O$(O)\n> \n> Then you can \"make O=0\" or \"make O=g\" if you want. And I think that\n> setting O=g in the environment (exported) would work, as well.\n\nSure, I can do something along these lines.\n\nPatrick\n"},{"id":"496443","messageId":"ZmFo5bZ1hJGrgvGM@tanuki","threadId":"61594","inReplyTo":"20240606065328.GB646308@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ci: fix check for Ubuntu 20.04","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-06T07:44:37Z","receivedAt":"2024-06-06T07:44:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jun 06, 2024 at 02:53:28AM -0400, Jeff King wrote:\n> On Thu, Jun 06, 2024 at 08:30:25AM +0200, Patrick Steinhardt wrote:\n> \n> > In 5ca0c455f1 (ci: fix Python dependency on Ubuntu 24.04, 2024-05-06),\n> > we made the use of Python 2 conditional on whether or not the CI job\n> > runs Ubuntu 20.04. There was a brown-paper-bag-style bug though, where\n> > the condition forgot to invoke the `test` builtin. The result of it is\n> > that the check always fails, and thus all of our jobs run with Python 3\n> > by accident.\n> > \n> > Fix this.\n> \n> Yikes. This looks obviously correct. Though I guess nobody noticed or\n> cared that we were not using python 2? It sounds like it is a\n> nice-to-have to get more coverage, but the platform in question is happy\n> to use python 3).\n\nYeah, the reason for this check really only is to get more coverage\nwhile Python 2 is still available on some of the distros that users may\nreasonably use. It's kind of a best effort check to keep it compatible,\neven though we will likely eventually drop that guarantee once Python 2\nis getting phased out by distros.\n\nPatrick\n"},{"id":"496456","messageId":"20240606080552.GA658959@coredump.intra.peff.net","threadId":"61594","inReplyTo":"ZmFoRPd52iqxbOTJ@tanuki","subject":"Re: [PATCH 2/2] ci: let pedantic job compile with -Og","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-06T08:05:52Z","receivedAt":"2024-06-06T08:05:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 06, 2024 at 09:41:56AM +0200, Patrick Steinhardt wrote:\n\n> > If we want to get coverage of -Og, I'd suggest doing it in a job that is\n> > otherwise overlapping with another (maybe linux-TEST-vars, which I think\n> > is otherwise a duplicate build?).\n> \n> I don't think linux-TEST-vars would be a good candidate for this because\n> it uses Ubuntu 20.04. Ideally, we'd want to have a test run with an\n> up-to-date version of Ubuntu so that we also get a recent version of the\n> compiler toolchain.\n\nOof, yeah, I agree that if the point is to find obscure warnings, newer\nis going to be better.\n\n> I kind of wonder whether we should revamp this pedantic job in the first\n> place. The consequence of that job is that our codebase needs to be\n> compile cleanly with `-Wpedantic`. So if that is a requirement anyway,\n> why don't we run all jobs with `DEVOPTS=pedantic` and just drop this job\n> altogether? This may surface some additional warnings on platforms where\n> we currently don't set that, but is that a bad thing?\n\nYeah, if we always compile cleanly with pedantic, then I don't see why\nit wouldn't just be the default for DEVELOPER=1. The point of that flag\nis to be as picky as possible so that we catch things early. If some\nplatform can't handle it (let's imagine Windows or something), then I\nthink we should be explicitly _disabling_ pedantic there.\n\n> The only downside I can think of is that we stop compiling on Fedora,\n> which may have a more up-to-date GCC version than Ubuntu. But if the\n> goal of this job was to _really_ get an up-to-date compiler toolchain,\n> then we should rather pick a rolling release distro like Arch. Otherwise\n> I find this to be of dubious benefit.\n\nThere may be some value in general in compiling on multiple distros, as\na sort of \"unknown unknowns\" thing. We don't know what we might turn up,\nbut exposing ourselves to more variables may lead to catching failures\nbefore users see them.\n\nI don't know if Fedora was specifically chosen for recent gcc there, or\nif it was simply for variety.\n\nOnce again, these overlapping variables within various jobs make it hard\nto reason about (but I don't propose normalizing all of them; that would\nincrease the amount of CPU work by a lot; I am just grumbling).\n\nBut yeah, between Arch and Fedora, I don't have an opinion. Doing both\nmight even be valuable, if we are shoving random variations into random\njobs. ;)\n\n> If we merge it into the other jobs, then I'd just pick any random job\n> that uses \"ubuntu:latest\" like \"linux-gcc-default\" to compile with\n> `-Og`.\n\nThat would be OK with me. I also think it would be OK to do nothing for\nnow. We saw one case where \"-Og\" found a warning that \"-O2\" didn't. We\ncould wait to see if it happens twice before acting. I dunno.\n\n-Peff\n"},{"id":"496464","messageId":"ZmFyi1fQ14tVy7Xg@tanuki","threadId":"61594","inReplyTo":"20240606080552.GA658959@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] ci: let pedantic job compile with -Og","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-06T08:25:47Z","receivedAt":"2024-06-06T08:25:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jun 06, 2024 at 04:05:52AM -0400, Jeff King wrote:\n> On Thu, Jun 06, 2024 at 09:41:56AM +0200, Patrick Steinhardt wrote:\n> > I kind of wonder whether we should revamp this pedantic job in the first\n> > place. The consequence of that job is that our codebase needs to be\n> > compile cleanly with `-Wpedantic`. So if that is a requirement anyway,\n> > why don't we run all jobs with `DEVOPTS=pedantic` and just drop this job\n> > altogether? This may surface some additional warnings on platforms where\n> > we currently don't set that, but is that a bad thing?\n> \n> Yeah, if we always compile cleanly with pedantic, then I don't see why\n> it wouldn't just be the default for DEVELOPER=1. The point of that flag\n> is to be as picky as possible so that we catch things early. If some\n> platform can't handle it (let's imagine Windows or something), then I\n> think we should be explicitly _disabling_ pedantic there.\n> \n> > The only downside I can think of is that we stop compiling on Fedora,\n> > which may have a more up-to-date GCC version than Ubuntu. But if the\n> > goal of this job was to _really_ get an up-to-date compiler toolchain,\n> > then we should rather pick a rolling release distro like Arch. Otherwise\n> > I find this to be of dubious benefit.\n> \n> There may be some value in general in compiling on multiple distros, as\n> a sort of \"unknown unknowns\" thing. We don't know what we might turn up,\n> but exposing ourselves to more variables may lead to catching failures\n> before users see them.\n> \n> I don't know if Fedora was specifically chosen for recent gcc there, or\n> if it was simply for variety.\n\nTrue enough. But even so, I think the better solution here would be to\ndrop one of the Ubuntu-based jobs and then convert the Fedora one into a\nfully-fledged job that also runs tests.\n\nThat's something for a later iteration, though.\n\n> Once again, these overlapping variables within various jobs make it hard\n> to reason about (but I don't propose normalizing all of them; that would\n> increase the amount of CPU work by a lot; I am just grumbling).\n\nNo arguing there, it certainly is hard to discover overall. I don't\nreally think there's a way around this if we want to have different\ncombinations while not running a full matrix of jobs, because the\ncombinations are always going to be arbitrary.\n\n> But yeah, between Arch and Fedora, I don't have an opinion. Doing both\n> might even be valuable, if we are shoving random variations into random\n> jobs. ;)\n\nTrue.\n\n> > If we merge it into the other jobs, then I'd just pick any random job\n> > that uses \"ubuntu:latest\" like \"linux-gcc-default\" to compile with\n> > `-Og`.\n> \n> That would be OK with me. I also think it would be OK to do nothing for\n> now. We saw one case where \"-Og\" found a warning that \"-O2\" didn't. We\n> could wait to see if it happens twice before acting. I dunno.\n\nI don't think it hurts to have a job with `-Og`, and if it detects some\ncorner cases that we otherwise don't I think there's a real benefit. In\nthe best case, I'd want to give everyone the tools to avoid sending\npatch series to the mailing list which are broken and where that could\nhave been detected earlier. In the end, it saves everybody's time.\n\nPatrick\n"},{"id":"496470","messageId":"cover.1717662814.git.ps@pks.im","threadId":"61594","inReplyTo":"20240606080552.GA658959@coredump.intra.peff.net","subject":"[PATCH v2 0/2] ci: detect more warnings via `-Og`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-06T09:31:35Z","receivedAt":"2024-06-06T09:31:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that modifies one of our\nCI jobs to compile with `-Og`. We have noticed that it may surface more\nwarnings that we do not see with `-O2`, so this should help to find more\nbugs up front.\n\nChanges compared to v1:\n\n - Instead of adapting the \"pedantic\" job, we now adapt\n \"linux-gcc-default\" to compile with `-Og`. This is because that job\n uses ubuntu:latest and thus a recent compiler, and there are other\n jobs with ubuntu:latest that continue to compile with `-O2`. So this\n is a strict improvement of coverage for diagnostics.\n\n - Add a way to override the optimization level to our Makefile, as\n suggested by Peff.\n\nPatrick\n\nPatrick Steinhardt (2):\n ci: fix check for Ubuntu 20.04\n ci: compile \"linux-gcc-default\" job with -Og\n\n Makefile | 3 ++-\n ci/lib.sh | 2 +-\n ci/run-build-and-tests.sh | 9 +++++++++\n 3 files changed, 12 insertions(+), 2 deletions(-)\n\nRange-diff against v1:\n1: f91004a438 = 1: f91004a438 ci: fix check for Ubuntu 20.04\n2: 351dec4a4d ! 2: bdf0e40a77 ci: let pedantic job compile with -Og\n @@ Metadata\n Author: Patrick Steinhardt \n \n ## Commit message ##\n - ci: let pedantic job compile with -Og\n + ci: compile \"linux-gcc-default\" job with -Og\n \n We have recently noticed that our CI does not always notice variables\n that may be used uninitialized. While it is expected that compiler\n @@ Commit message\n \n While we could adapt all jobs to compile with `-Og` now, that would\n potentially mask other warnings that only get diagnosed with `-O2`.\n - Instead, only adapt the \"pedantic\" job to compile with `-Og`.\n + Instead, adapt the \"linux-gcc-default\" job to compile with `-Og`. This\n + job is chosen because it uses the \"ubuntu:latest\" image and should thus\n + have a comparatively recent compiler toolchain, and because we have\n + other jobs that use \"ubuntu:latest\" so that we do not loose coverage for\n + warnings diagnosed only on `-O2` level.\n +\n + To make it easier to set up the optimization level in our CI, add\n + support in our Makefile to specify the level via an environment\n + variable.\n \n Signed-off-by: Patrick Steinhardt \n \n + ## Makefile ##\n +@@ Makefile: endif\n + # tweaked by config.* below as well as the command-line, both of\n + # which'll override these defaults.\n + # Older versions of GCC may require adding \"-std=gnu99\" at the end.\n +-CFLAGS = -g -O2 -Wall\n ++O ?= 2\n ++CFLAGS = -g -O$(O) -Wall\n + LDFLAGS =\n + CC_LD_DYNPATH = -Wl,-rpath,\n + BASIC_CFLAGS = -I.\n +\n ## ci/run-build-and-tests.sh ##\n -@@ ci/run-build-and-tests.sh: pedantic)\n - \t# Don't run the tests; we only care about whether Git can be\n - \t# built.\n - \texport DEVOPTS=pedantic\n +@@ ci/run-build-and-tests.sh: esac\n + run_tests=t\n + \n + case \"$jobname\" in\n ++linux-gcc-default)\n +\t# Warnings generated by compilers are unfortunately specific to the\n +\t# optimization level. With `-O0`, many warnings won't be shown at all,\n +\t# whereas the optimizations performed by our default optimization level\n +\t# `-O2` will mask others. We thus use `-Og` here just so that we have\n +\t# at least one job with a different optimization level so that we can\n +\t# overall surface more warnings.\n -+\tcat >config.mak <<-EOF\n -+\texport CFLAGS=-Og\n -+\tEOF\n - \trun_tests=\n ++\texport O=g\n ++\t;;\n + linux-gcc)\n + \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n \t;;\n - esac\n-- \n2.45.2.409.g7b0defb391.dirty\n\n"},{"id":"496471","messageId":"f91004a4382c95b1b87bf3d1aafc018af2b7b92e.1717662814.git.ps@pks.im","threadId":"61594","inReplyTo":"cover.1717662814.git.ps@pks.im","subject":"[PATCH v2 1/2] ci: fix check for Ubuntu 20.04","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-06T09:31:40Z","receivedAt":"2024-06-06T09:31:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 5ca0c455f1 (ci: fix Python dependency on Ubuntu 24.04, 2024-05-06),\nwe made the use of Python 2 conditional on whether or not the CI job\nruns Ubuntu 20.04. There was a brown-paper-bag-style bug though, where\nthe condition forgot to invoke the `test` builtin. The result of it is\nthat the check always fails, and thus all of our jobs run with Python 3\nby accident.\n\nFix this.\n\nSigned-off-by: Patrick Steinhardt \n---\n ci/lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 1f4059b1b8..814578ffc6 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -328,7 +328,7 @@ ubuntu-*)\n \t# Python 2 is end of life, and Ubuntu 23.04 and newer don't actually\n \t# have it anymore. We thus only test with Python 2 on older LTS\n \t# releases.\n-\tif \"$distro\" = \"ubuntu-20.04\"\n+\tif test \"$distro\" = \"ubuntu-20.04\"\n \tthen\n \t\tPYTHON_PACKAGE=python2\n \telse\n-- \n2.45.2.409.g7b0defb391.dirty\n\n"},{"id":"496472","messageId":"bdf0e40a770c57b63e7519983d37b97a85ce07bf.1717662814.git.ps@pks.im","threadId":"61594","inReplyTo":"cover.1717662814.git.ps@pks.im","subject":"[PATCH v2 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-06T09:31:45Z","receivedAt":"2024-06-06T09:31:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have recently noticed that our CI does not always notice variables\nthat may be used uninitialized. While it is expected that compiler\nwarnings aren't perfect, this one was a but puzzling because it was\nrather obvious that the variable can be uninitialized.\n\nMany compiler warnings unfortunately depend on the optimization level\nused by the compiler. While `-O0` for example will disable a lot of\nwarnings altogether because optimization passes go away, `-O2`, which is\nour default optimization level used in CI, may optimize specific code\naway or even double down on undefined behaviour. Interestingly, this\nspecific instance that triggered the investigation does get noted by GCC\nwhen using `-Og`.\n\nWhile we could adapt all jobs to compile with `-Og` now, that would\npotentially mask other warnings that only get diagnosed with `-O2`.\nInstead, adapt the \"linux-gcc-default\" job to compile with `-Og`. This\njob is chosen because it uses the \"ubuntu:latest\" image and should thus\nhave a comparatively recent compiler toolchain, and because we have\nother jobs that use \"ubuntu:latest\" so that we do not loose coverage for\nwarnings diagnosed only on `-O2` level.\n\nTo make it easier to set up the optimization level in our CI, add\nsupport in our Makefile to specify the level via an environment\nvariable.\n\nSigned-off-by: Patrick Steinhardt \n---\n\nI was a little torn whether we really want to name the variable `O`\nhere because it feels so easy to set it by accident. We could rename\nthis to `OPTIMIZATION` or `OPTIMIZATION_LEVEL`, but that's quite a\nmouthful.\n\nAlternatively, if we don't want to have this variable in the first\nplace, then I'm also happy to adapt the script itself to pass the\noptimization level via an argument.\n\n Makefile | 3 ++-\n ci/run-build-and-tests.sh | 9 +++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 59d98ba688..ff57c94fdf 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1357,7 +1357,8 @@ endif\n # tweaked by config.* below as well as the command-line, both of\n # which'll override these defaults.\n # Older versions of GCC may require adding \"-std=gnu99\" at the end.\n-CFLAGS = -g -O2 -Wall\n+O ?= 2\n+CFLAGS = -g -O$(O) -Wall\n LDFLAGS =\n CC_LD_DYNPATH = -Wl,-rpath,\n BASIC_CFLAGS = -I.\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex 98dda42045..0f00dbd289 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -13,6 +13,15 @@ esac\n run_tests=t\n \n case \"$jobname\" in\n+linux-gcc-default)\n+\t# Warnings generated by compilers are unfortunately specific to the\n+\t# optimization level. With `-O0`, many warnings won't be shown at all,\n+\t# whereas the optimizations performed by our default optimization level\n+\t# `-O2` will mask others. We thus use `-Og` here just so that we have\n+\t# at least one job with a different optimization level so that we can\n+\t# overall surface more warnings.\n+\texport O=g\n+\t;;\n linux-gcc)\n \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n \t;;\n-- \n2.45.2.409.g7b0defb391.dirty\n\n"},{"id":"496526","messageId":"crfguam2loy7nrwenirbt3rub3vj52nvhxuvr5foyx6sd2weez@vjnnzrmc6scc","threadId":"61594","inReplyTo":"bdf0e40a770c57b63e7519983d37b97a85ce07bf.1717662814.git.ps@pks.im","subject":"Re: [PATCH v2 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-06-06T15:32:33Z","receivedAt":"2024-06-06T15:32:42Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/06/06 11:31AM, Patrick Steinhardt wrote:\n> We have recently noticed that our CI does not always notice variables\n> that may be used uninitialized. While it is expected that compiler\n> warnings aren't perfect, this one was a but puzzling because it was\n\ns/but/bit/\n\n> rather obvious that the variable can be uninitialized.\n> \n> Many compiler warnings unfortunately depend on the optimization level\n> used by the compiler. While `-O0` for example will disable a lot of\n> warnings altogether because optimization passes go away, `-O2`, which is\n> our default optimization level used in CI, may optimize specific code\n> away or even double down on undefined behaviour. Interestingly, this\n> specific instance that triggered the investigation does get noted by GCC\n> when using `-Og`.\n> \n> While we could adapt all jobs to compile with `-Og` now, that would\n> potentially mask other warnings that only get diagnosed with `-O2`.\n> Instead, adapt the \"linux-gcc-default\" job to compile with `-Og`. This\n> job is chosen because it uses the \"ubuntu:latest\" image and should thus\n> have a comparatively recent compiler toolchain, and because we have\n> other jobs that use \"ubuntu:latest\" so that we do not loose coverage for\n\ns/loose/lose/\n\n> warnings diagnosed only on `-O2` level.\n> \n> To make it easier to set up the optimization level in our CI, add\n> support in our Makefile to specify the level via an environment\n> variable.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> \n> I was a little torn whether we really want to name the variable `O`\n> here because it feels so easy to set it by accident. We could rename\n> this to `OPTIMIZATION` or `OPTIMIZATION_LEVEL`, but that's quite a\n> mouthful.\n> \n> Alternatively, if we don't want to have this variable in the first\n> place, then I'm also happy to adapt the script itself to pass the\n> optimization level via an argument.\n\nI think the variable itself is fine, but for a name I think that either \n`OPTIMIZATION` or `OPTIMIZATION_LEVEL` would be a better pick. We have \nsome other lengthy environment varible names so I don't think its too \nbad.\n\nOtherwise, this looks good to me :)\n\n-Justin\n"},{"id":"496543","messageId":"xmqq4ja6niba.fsf@gitster.g","threadId":"61594","inReplyTo":"20240606065236.GA646308@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] ci: let pedantic job compile with -Og","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T16:32:09Z","receivedAt":"2024-06-06T16:32:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So for the pedantic warnings, we're left with a guess as to whether -Og\n> or -O2 will yield more results. And in my experience it is probably -O2.\n>\n> If we want to get coverage of -Og, I'd suggest doing it in a job that is\n> otherwise overlapping with another (maybe linux-TEST-vars, which I think\n> is otherwise a duplicate build?).\n\nThe same knee-jerk reaction came to me.\n\nSpeaking of variants, is there any interest in migrating one or some\nof the existing x86-64 CI jobs to arm64 CI jobs GitHub introduced\nrecently? I suspect that we won't find any endianness bugs (I\nexpect they are configured to do little endian just like everybody\nelse) and there may no longer be lurking unaligned read bugs (but\n\"git log --grep=unaligned\" finds surprising number of them we have\nseen and fixed), so the returns may be very small.\n\n> Your command above also loses the \"-g\" and \"-Wall\" from the default\n> CFLAGS. Maybe OK, since DEVELOPER=1 implies -Wall anyway, and \"-g\" isn't\n> important. But one thing I've done for a long time in my config.mak is:\n>\n> O ?= 2\n> CFLAGS = -g -Wall -O$(O)\n>\n> Then you can \"make O=0\" or \"make O=g\" if you want. And I think that\n> setting O=g in the environment (exported) would work, as well.\n\nI do something similar, but my $(O) replaces the whole -O2 thing, so\nI can say something silly like\n\n make O=\"-O2 -g -Wall\"\n\n\n"},{"id":"496546","messageId":"xmqqplsum2cz.fsf@gitster.g","threadId":"61594","inReplyTo":"bdf0e40a770c57b63e7519983d37b97a85ce07bf.1717662814.git.ps@pks.im","subject":"Re: [PATCH v2 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T17:02:04Z","receivedAt":"2024-06-06T17:02:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> I was a little torn whether we really want to name the variable `O`\n> here because it feels so easy to set it by accident. We could rename\n> this to `OPTIMIZATION` or `OPTIMIZATION_LEVEL`, but that's quite a\n> mouthful.\n>\n> Alternatively, if we don't want to have this variable in the first\n> place, then I'm also happy to adapt the script itself to pass the\n> optimization level via an argument.\n\nThe latter is much more preferrable. It is too easy to stomp on\npeople's established workflow that already uses that variable for\nother purposes or expects slightly different syntax.\n\n> Makefile | 3 ++-\n> ci/run-build-and-tests.sh | 9 +++++++++\n> 2 files changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 59d98ba688..ff57c94fdf 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1357,7 +1357,8 @@ endif\n> # tweaked by config.* below as well as the command-line, both of\n> # which'll override these defaults.\n> # Older versions of GCC may require adding \"-std=gnu99\" at the end.\n> -CFLAGS = -g -O2 -Wall\n> +O ?= 2\n> +CFLAGS = -g -O$(O) -Wall\n> LDFLAGS =\n> CC_LD_DYNPATH = -Wl,-rpath,\n> BASIC_CFLAGS = -I.\n> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n> index 98dda42045..0f00dbd289 100755\n> --- a/ci/run-build-and-tests.sh\n> +++ b/ci/run-build-and-tests.sh\n> @@ -13,6 +13,15 @@ esac\n> run_tests=t\n> \n> case \"$jobname\" in\n> +linux-gcc-default)\n> +\t# Warnings generated by compilers are unfortunately specific to the\n> +\t# optimization level. With `-O0`, many warnings won't be shown at all,\n> +\t# whereas the optimizations performed by our default optimization level\n> +\t# `-O2` will mask others. We thus use `-Og` here just so that we have\n> +\t# at least one job with a different optimization level so that we can\n> +\t# overall surface more warnings.\n> +\texport O=g\n> +\t;;\n> linux-gcc)\n> \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> \t;;\n"},{"id":"496598","messageId":"ZmKWLV29TQ71DQ7u@tanuki","threadId":"61594","inReplyTo":"xmqq4ja6niba.fsf@gitster.g","subject":"Re: [PATCH 2/2] ci: let pedantic job compile with -Og","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-07T05:10:05Z","receivedAt":"2024-06-07T05:10:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jun 06, 2024 at 09:32:09AM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > So for the pedantic warnings, we're left with a guess as to whether -Og\n> > or -O2 will yield more results. And in my experience it is probably -O2.\n> >\n> > If we want to get coverage of -Og, I'd suggest doing it in a job that is\n> > otherwise overlapping with another (maybe linux-TEST-vars, which I think\n> > is otherwise a duplicate build?).\n> \n> The same knee-jerk reaction came to me.\n> \n> Speaking of variants, is there any interest in migrating one or some\n> of the existing x86-64 CI jobs to arm64 CI jobs GitHub introduced\n> recently? I suspect that we won't find any endianness bugs (I\n> expect they are configured to do little endian just like everybody\n> else) and there may no longer be lurking unaligned read bugs (but\n> \"git log --grep=unaligned\" finds surprising number of them we have\n> seen and fixed), so the returns may be very small.\n\nNote that we already run arm64 via GitLab's macOS runners. That's not\nLinux of course, but I guess that any architectural issues should still\nbe caught by that.\n\nNot to say that we shouldn't adapt GitHub.\n\nPatrick\n"},{"id":"496599","messageId":"ZmKalkqR3GqCXfqz@tanuki","threadId":"61594","inReplyTo":"xmqqplsum2cz.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-07T05:28:54Z","receivedAt":"2024-06-07T05:28:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jun 06, 2024 at 10:02:04AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > I was a little torn whether we really want to name the variable `O`\n> > here because it feels so easy to set it by accident. We could rename\n> > this to `OPTIMIZATION` or `OPTIMIZATION_LEVEL`, but that's quite a\n> > mouthful.\n> >\n> > Alternatively, if we don't want to have this variable in the first\n> > place, then I'm also happy to adapt the script itself to pass the\n> > optimization level via an argument.\n> \n> The latter is much more preferrable. It is too easy to stomp on\n> people's established workflow that already uses that variable for\n> other purposes or expects slightly different syntax.\n\nWe're unlikely to break existing workflows though if we name this\nvariable something like `OPTIMIZATION_LEVEL`. The workflows of both you\nand Peff would keep on working without any modification. Also, doesn't\nthe fact that both of you have similar workarounds hint that this is\nsomething that other devs might want to have, as well?\n\nWe could also generalize this a bit and introduce `CFLAGS_APPEND`\ninstead. Optimization levels are last-one-wins anyway, so people can use\nthat to append their own flags without overriding existing values. It\nwould also mean that we can avoid repeating the CFLAGS that we already\nhave in our Makefile in our CI scripts.\n\nI'll send a version along these lines. If people are still unhappy with\nthat direction, then I'll give up and just repeat the whole CFLAGS in\n\"ci/run-build-and-tests.sh\".\n\nPatrick\n"},{"id":"496628","messageId":"cover.1717742752.git.ps@pks.im","threadId":"61594","inReplyTo":"20240606080552.GA658959@coredump.intra.peff.net","subject":"[PATCH v3 0/4] ci: detect more warnings via `-Og`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-07T06:46:24Z","receivedAt":"2024-06-07T06:46:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the third version of this patch series that adapts one of our CI\njobs to compile with `-Og` for improved coverage of warnings.\n\nChanges compared to v2:\n\n - Typo fixes for commit messages.\n\n - Replaced the `O` variable with `CFLAGS_APPEND`. If that isn't\n palatable to folks then I'll drop this altogether and will inline it\n into the CI script, duplicating the default CFLAGS there.\n\n - Start compiling with V=1 so that the change can actually be seen. It\n also shouldn't clutter the job output too much given that the build\n is in a collapsible section on both GitHub and GitLab.\n\nPatrick\n\nPatrick Steinhardt (4):\n ci: fix check for Ubuntu 20.04\n Makefile: add ability to append to CFLAGS and LDFLAGS\n ci: compile code with V=1\n ci: compile \"linux-gcc-default\" job with -Og\n\n Makefile | 2 ++\n ci/lib.sh | 2 +-\n ci/run-build-and-tests.sh | 11 ++++++++++-\n 3 files changed, 13 insertions(+), 2 deletions(-)\n\nRange-diff against v2:\n1: f91004a438 = 1: 70fa755b4f ci: fix check for Ubuntu 20.04\n-: ---------- > 2: d68539834f Makefile: add ability to append to CFLAGS and LDFLAGS\n-: ---------- > 3: a3dfb7092a ci: compile code with V=1\n2: bdf0e40a77 ! 4: c7b5b62d9c ci: compile \"linux-gcc-default\" job with -Og\n @@ Commit message\n \n We have recently noticed that our CI does not always notice variables\n that may be used uninitialized. While it is expected that compiler\n - warnings aren't perfect, this one was a but puzzling because it was\n + warnings aren't perfect, this one was a bit puzzling because it was\n rather obvious that the variable can be uninitialized.\n \n Many compiler warnings unfortunately depend on the optimization level\n @@ Commit message\n Instead, adapt the \"linux-gcc-default\" job to compile with `-Og`. This\n job is chosen because it uses the \"ubuntu:latest\" image and should thus\n have a comparatively recent compiler toolchain, and because we have\n - other jobs that use \"ubuntu:latest\" so that we do not loose coverage for\n + other jobs that use \"ubuntu:latest\" so that we do not lose coverage for\n warnings diagnosed only on `-O2` level.\n \n To make it easier to set up the optimization level in our CI, add\n @@ Commit message\n \n Signed-off-by: Patrick Steinhardt \n \n - ## Makefile ##\n -@@ Makefile: endif\n - # tweaked by config.* below as well as the command-line, both of\n - # which'll override these defaults.\n - # Older versions of GCC may require adding \"-std=gnu99\" at the end.\n --CFLAGS = -g -O2 -Wall\n -+O ?= 2\n -+CFLAGS = -g -O$(O) -Wall\n - LDFLAGS =\n - CC_LD_DYNPATH = -Wl,-rpath,\n - BASIC_CFLAGS = -I.\n -\n ## ci/run-build-and-tests.sh ##\n @@ ci/run-build-and-tests.sh: esac\n run_tests=t\n @@ ci/run-build-and-tests.sh: esac\n +\t# `-O2` will mask others. We thus use `-Og` here just so that we have\n +\t# at least one job with a different optimization level so that we can\n +\t# overall surface more warnings.\n -+\texport O=g\n ++\texport CFLAGS_APPEND=-Og\n +\t;;\n linux-gcc)\n \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n-- \n2.45.2.436.gcd77e87115.dirty\n\n"},{"id":"496629","messageId":"70fa755b4ffd396904c9f20f8a783ab194699d77.1717742752.git.ps@pks.im","threadId":"61594","inReplyTo":"cover.1717742752.git.ps@pks.im","subject":"[PATCH v3 1/4] ci: fix check for Ubuntu 20.04","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-07T06:46:29Z","receivedAt":"2024-06-07T06:46:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 5ca0c455f1 (ci: fix Python dependency on Ubuntu 24.04, 2024-05-06),\nwe made the use of Python 2 conditional on whether or not the CI job\nruns Ubuntu 20.04. There was a brown-paper-bag-style bug though, where\nthe condition forgot to invoke the `test` builtin. The result of it is\nthat the check always fails, and thus all of our jobs run with Python 3\nby accident.\n\nFix this.\n\nSigned-off-by: Patrick Steinhardt \n---\n ci/lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 1f4059b1b8..814578ffc6 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -328,7 +328,7 @@ ubuntu-*)\n \t# Python 2 is end of life, and Ubuntu 23.04 and newer don't actually\n \t# have it anymore. We thus only test with Python 2 on older LTS\n \t# releases.\n-\tif \"$distro\" = \"ubuntu-20.04\"\n+\tif test \"$distro\" = \"ubuntu-20.04\"\n \tthen\n \t\tPYTHON_PACKAGE=python2\n \telse\n-- \n2.45.2.436.gcd77e87115.dirty\n\n"},{"id":"496630","messageId":"d68539834f3827fa3ffe91517e053c043243a378.1717742752.git.ps@pks.im","threadId":"61594","inReplyTo":"cover.1717742752.git.ps@pks.im","subject":"[PATCH v3 2/4] Makefile: add ability to append to CFLAGS and LDFLAGS","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-07T06:46:34Z","receivedAt":"2024-06-07T06:46:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are some usecases where we may want to append CFLAGS to the\ndefault CFLAGS set by Git. This could for example be to enable or\ndisable specific compiler warnings or to change the optimization level\nthat code is compiled with. This cannot be done without overriding the\ncomplete CFLAGS value though and thus requires the user to redeclare the\ncomplete defaults used by Git.\n\nIntroduce a new variable `CFLAGS_APPEND` that gets appended to the\ndefault value of `CFLAGS`. As compiler options are last-one-wins, this\nfulfills both of the usecases mentioned above. It's also common practice\nacross many other projects to have such a variable.\n\nWhile at it, also introduce a matching `LDFLAGS_APPEND` variable. While\nthere isn't really any need for this variable as there are no default\n`LDFLAGS`, users may expect this variable to exist, as well.\n\nNote that we have to use the `override` directive here such that the\nflags get appended when compiling with `make CFLAGS=x CFLAGS_APPEND=y`.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 2f5f16847a..b5252bed3d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1359,7 +1359,9 @@ endif\n # which'll override these defaults.\n # Older versions of GCC may require adding \"-std=gnu99\" at the end.\n CFLAGS = -g -O2 -Wall\n+override CFLAGS += $(CFLAGS_APPEND)\n LDFLAGS =\n+override LDFLAGS += $(LDFLAGS_APPEND)\n CC_LD_DYNPATH = -Wl,-rpath,\n BASIC_CFLAGS = -I.\n BASIC_LDFLAGS =\n-- \n2.45.2.436.gcd77e87115.dirty\n\n"},{"id":"496631","messageId":"a3dfb7092a4aeb09cd9863e3e9ced2b4685f8576.1717742752.git.ps@pks.im","threadId":"61594","inReplyTo":"cover.1717742752.git.ps@pks.im","subject":"[PATCH v3 3/4] ci: compile code with V=1","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-07T06:46:38Z","receivedAt":"2024-06-07T06:46:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It's hard to see which compiler flags our CI uses because we do not\ncompile with `V=1`, and thus we only see beautified output. This\ninformation can be important at times though and may help developers to\nsee what's going on in a CI job. Furthermore, both GitLab and GitHub\nhave sections in their output such that the build output can be neatly\nhidden away, so it doesn't clutter the output, either.\n\nCompile code with `V=1` to improve debuggability.\n\nSigned-off-by: Patrick Steinhardt \n---\n ci/run-build-and-tests.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex 98dda42045..2aaaf40f94 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -48,7 +48,7 @@ pedantic)\n \t;;\n esac\n \n-group Build make\n+group Build make V=1\n if test -n \"$run_tests\"\n then\n \tgroup \"Run tests\" make test ||\n-- \n2.45.2.436.gcd77e87115.dirty\n\n"},{"id":"496632","messageId":"c7b5b62d9c5b6c737d9afe3d32382ccf835e23ce.1717742752.git.ps@pks.im","threadId":"61594","inReplyTo":"cover.1717742752.git.ps@pks.im","subject":"[PATCH v3 4/4] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-07T06:46:45Z","receivedAt":"2024-06-07T06:46:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have recently noticed that our CI does not always notice variables\nthat may be used uninitialized. While it is expected that compiler\nwarnings aren't perfect, this one was a bit puzzling because it was\nrather obvious that the variable can be uninitialized.\n\nMany compiler warnings unfortunately depend on the optimization level\nused by the compiler. While `-O0` for example will disable a lot of\nwarnings altogether because optimization passes go away, `-O2`, which is\nour default optimization level used in CI, may optimize specific code\naway or even double down on undefined behaviour. Interestingly, this\nspecific instance that triggered the investigation does get noted by GCC\nwhen using `-Og`.\n\nWhile we could adapt all jobs to compile with `-Og` now, that would\npotentially mask other warnings that only get diagnosed with `-O2`.\nInstead, adapt the \"linux-gcc-default\" job to compile with `-Og`. This\njob is chosen because it uses the \"ubuntu:latest\" image and should thus\nhave a comparatively recent compiler toolchain, and because we have\nother jobs that use \"ubuntu:latest\" so that we do not lose coverage for\nwarnings diagnosed only on `-O2` level.\n\nTo make it easier to set up the optimization level in our CI, add\nsupport in our Makefile to specify the level via an environment\nvariable.\n\nSigned-off-by: Patrick Steinhardt \n---\n ci/run-build-and-tests.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex 2aaaf40f94..e5fbe7f531 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -13,6 +13,15 @@ esac\n run_tests=t\n \n case \"$jobname\" in\n+linux-gcc-default)\n+\t# Warnings generated by compilers are unfortunately specific to the\n+\t# optimization level. With `-O0`, many warnings won't be shown at all,\n+\t# whereas the optimizations performed by our default optimization level\n+\t# `-O2` will mask others. We thus use `-Og` here just so that we have\n+\t# at least one job with a different optimization level so that we can\n+\t# overall surface more warnings.\n+\texport CFLAGS_APPEND=-Og\n+\t;;\n linux-gcc)\n \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n \t;;\n-- \n2.45.2.436.gcd77e87115.dirty\n\n"},{"id":"496664","messageId":"xmqqmsnwd272.fsf@gitster.g","threadId":"61594","inReplyTo":"ZmKWLV29TQ71DQ7u@tanuki","subject":"Re: [PATCH 2/2] ci: let pedantic job compile with -Og","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T18:42:41Z","receivedAt":"2024-06-07T18:42:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> Speaking of variants, is there any interest in migrating one or some\n>> of the existing x86-64 CI jobs to arm64 CI jobs GitHub introduced\n>> recently? I suspect that we won't find any endianness bugs (I\n>> expect they are configured to do little endian just like everybody\n>> else) and there may no longer be lurking unaligned read bugs (but\n>> \"git log --grep=unaligned\" finds surprising number of them we have\n>> seen and fixed), so the returns may be very small.\n>\n> Note that we already run arm64 via GitLab's macOS runners. That's not\n> Linux of course, but I guess that any architectural issues should still\n> be caught by that.\n\nThe more the merrier and it is nice to know we have extra variety\nover there.\n\n> Not to say that we shouldn't adapt GitHub.\n\n;-)\n"},{"id":"496665","messageId":"xmqqikykd22p.fsf@gitster.g","threadId":"61594","inReplyTo":"ZmKalkqR3GqCXfqz@tanuki","subject":"Re: [PATCH v2 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T18:45:18Z","receivedAt":"2024-06-07T18:45:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> We're unlikely to break existing workflows though if we name this\n> variable something like `OPTIMIZATION_LEVEL`.\n\nYeah, being more explicit is always good.\n\n> We could also generalize this a bit and introduce `CFLAGS_APPEND`\n> instead. Optimization levels are last-one-wins anyway, so people can use\n> that to append their own flags without overriding existing values. It\n> would also mean that we can avoid repeating the CFLAGS that we already\n> have in our Makefile in our CI scripts.\n\nYup, Peff's $(O) cannot serve as such, but my $(O) is already being\nused as such. Naming the variable that gives additional CFLAGS as\nsuch is probably a good way to go.\n\nThanks.\n\n"},{"id":"496666","messageId":"xmqqed98d1wn.fsf@gitster.g","threadId":"61594","inReplyTo":"bdf0e40a770c57b63e7519983d37b97a85ce07bf.1717662814.git.ps@pks.im","subject":"Re: [PATCH v2 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T18:48:56Z","receivedAt":"2024-06-07T18:49:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> While we could adapt all jobs to compile with `-Og` now, that would\n> potentially mask other warnings that only get diagnosed with `-O2`.\n> Instead, adapt the \"linux-gcc-default\" job to compile with `-Og`.\n\nIt seems that we are already triggering for 'seen'. I haven't\nchecked if that is a false positive or a real problem, though.\n\nhttps://github.com/git/git/actions/runs/9407652417/job/25913907166#step:4:139\n\n pack-mtimes.c: In function ‘load_pack_mtimes_file’:\n Error: pack-mtimes.c:89:25: ‘mtimes_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]\n 89 | munmap(data, mtimes_size);\n | ^~~~~~~~~~~~~~~~~~~~~~~~~\n cc1: all warnings being treated as errors\n make: *** [Makefile:2757: pack-mtimes.o] Error 1\n make: *** Waiting for unfinished jobs....\n pack-revindex.c: In function ‘load_revindex_from_disk’:\n Error: pack-revindex.c:260:25: ‘revindex_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]\n 260 | munmap(data, revindex_size);\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~\n cc1: all warnings being treated as errors\n make: *** [Makefile:2757: pack-revindex.o] Error 1\n cat: exit.status: No such file or directory\n"},{"id":"496671","messageId":"xmqq34pofq3z.fsf@gitster.g","threadId":"61594","inReplyTo":"xmqqed98d1wn.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T20:35:28Z","receivedAt":"2024-06-07T20:35:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> pack-mtimes.c: In function ‘load_pack_mtimes_file’:\n> Error: pack-mtimes.c:89:25: ‘mtimes_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]\n> 89 | munmap(data, mtimes_size);\n> | ^~~~~~~~~~~~~~~~~~~~~~~~~\n> cc1: all warnings being treated as errors\n> make: *** [Makefile:2757: pack-mtimes.o] Error 1\n> make: *** Waiting for unfinished jobs....\n\nThe use on line 89 is guarded with \"if (data)\" and data can become\nnon-NULL only after mtimes_size is computed, so this is benign.\n\nThey have excuse for a false positive because the warning is about\n\"maybe\" uninitialized, but that does not help our annoyance factor\nX-<.\n\n> pack-revindex.c: In function ‘load_revindex_from_disk’:\n> Error: pack-revindex.c:260:25: ‘revindex_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]\n> 260 | munmap(data, revindex_size);\n> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~\n> cc1: all warnings being treated as errors\n> make: *** [Makefile:2757: pack-revindex.o] Error 1\n> cat: exit.status: No such file or directory\n\nThis follows exactly the same pattern established by the other one\n(or perhaps the other one copied from here). It is another false\npositive.\n\nI am not sure what the right fix would be. For example, if we were\ninterested in avoiding to incur too much resources for revindex, we\nmight do something like this\n\n--- i/pack-revindex.c\n+++ w/pack-revindex.c\n@@ -258,6 +258,8 @@ static int load_revindex_from_disk(char *revindex_name,\n \tif (ret) {\n \t\tif (data)\n \t\t\tmunmap(data, revindex_size);\n+\t\tfprintf(stderr, \"would have fit %d revindex in 10MB\\n\",\n+\t\t\t10 * 1024 * 1024 / revindex_size);\n \t} else {\n \t\t*len_p = revindex_size;\n \t\t*data_p = (const uint32_t *)data;\n\nwithout even guarding with \"if (data)\".\n\nIf we \"initialize\" revindex_size to a meaningless dummy value like 0\nlike the attached would _hide_ such a real bug from the compiler, so\nI dunno.\n\n@@ -206,7 +206,7 @@ static int load_revindex_from_disk(char *revindex_name,\n \tint fd, ret = 0;\n \tstruct stat st;\n \tvoid *data = NULL;\n-\tsize_t revindex_size;\n+\tsize_t revindex_size = 0;\n \tstruct revindex_header *hdr;\n \n \tif (git_env_bool(GIT_TEST_REV_INDEX_DIE_ON_DISK, 0))\n"},{"id":"496672","messageId":"xmqqwmn0eazm.fsf@gitster.g","threadId":"61594","inReplyTo":"cover.1717742752.git.ps@pks.im","subject":"Re: [PATCH v3 0/4] ci: detect more warnings via `-Og`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T20:47:25Z","receivedAt":"2024-06-07T20:47:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> this is the third version of this patch series that adapts one of our CI\n> jobs to compile with `-Og` for improved coverage of warnings.\n>\n> Changes compared to v2:\n>\n> - Typo fixes for commit messages.\n>\n> - Replaced the `O` variable with `CFLAGS_APPEND`. If that isn't\n> palatable to folks then I'll drop this altogether and will inline it\n> into the CI script, duplicating the default CFLAGS there.\n>\n> - Start compiling with V=1 so that the change can actually be seen. It\n> also shouldn't clutter the job output too much given that the build\n> is in a collapsible section on both GitHub and GitLab.\n\nI've taken the \"Python 2 - missing 'test'\" on a separate topic and\nwill merge it down to 'next' and 'master' to fast track.\n\nThe \"override CFLAGS += CFLAGS_APPEND\" thing looks good.\n\nI am not sure how annoying people will find the V=1 output. It is\nirrelevant that it is in a collapsible section. What matters is if\nit helps those who *need* to expand that collapsible section to take\na look, or if it clutteres what they have to wade through.\n\nWhen studying a build failure, I rarely found the exact command line\ngiven by V=1 helpful, but YMMV---while I am not 100% convinced, let's\ntake the series as-is, because not losing information may sometimes\nhelp even when we need to visually filter out extra clutter.\n\nThanks.\n\n"},{"id":"496693","messageId":"20240608084910.GC2390433@coredump.intra.peff.net","threadId":"61594","inReplyTo":"xmqqikykd22p.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-08T08:49:10Z","receivedAt":"2024-06-08T08:49:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 07, 2024 at 11:45:18AM -0700, Junio C Hamano wrote:\n\n> Patrick Steinhardt writes:\n> \n> > We're unlikely to break existing workflows though if we name this\n> > variable something like `OPTIMIZATION_LEVEL`.\n> \n> Yeah, being more explicit is always good.\n\nOne of the reasons I used the very short \"O\" for mine is that I often\nspecify it by hand. I actually leave it as O=0 by default, since the\nmajority of my builds are about developing and debugging (so speed of\ncompilation is much more important than speed of the resulting\nexecutable). And then when I am interested in performance, I run \"make\nO=2\".\n\nSo OPTIMIZATION_LEVEL defeats the purpose. ;)\n\n> > We could also generalize this a bit and introduce `CFLAGS_APPEND`\n> > instead. Optimization levels are last-one-wins anyway, so people can use\n> > that to append their own flags without overriding existing values. It\n> > would also mean that we can avoid repeating the CFLAGS that we already\n> > have in our Makefile in our CI scripts.\n> \n> Yup, Peff's $(O) cannot serve as such, but my $(O) is already being\n> used as such. Naming the variable that gives additional CFLAGS as\n> such is probably a good way to go.\n\nI have something like this in my config.mak, too. ;) But I call it\nEXTRA_CFLAGS. That seems less grammatically awkward to me than\nCFLAGS_APPEND, but that may be entering bikeshed territory.\n\nAnd you can tell from the length name that I do not use it all that\noften.\n\n-Peff\n"},{"id":"496694","messageId":"20240608085528.GD2390433@coredump.intra.peff.net","threadId":"61594","inReplyTo":"d68539834f3827fa3ffe91517e053c043243a378.1717742752.git.ps@pks.im","subject":"Re: [PATCH v3 2/4] Makefile: add ability to append to CFLAGS and LDFLAGS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-08T08:55:28Z","receivedAt":"2024-06-08T08:55:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 07, 2024 at 08:46:34AM +0200, Patrick Steinhardt wrote:\n\n> Note that we have to use the `override` directive here such that the\n> flags get appended when compiling with `make CFLAGS=x CFLAGS_APPEND=y`.\n\nAnother way to do this is just:\n\ndiff --git a/Makefile b/Makefile\nindex 2f5f16847a..9cd3b252ff 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1446,8 +1446,8 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)\n ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)\n endif\n \n-ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)\n-ALL_LDFLAGS = $(LDFLAGS)\n+ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_APPEND)\n+ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)\n \n ifdef SANITIZE\n SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))\n\nI can't think offhand of any way that your override would not do the\nright thing, but:\n\n - this is roughly the same problem faced by DEVELOPER_CFLAGS, etc, so\n handling it in the same way makes sense to me\n\n - I always get nervous around make features like \"override\", as there\n are sometimes corner cases lurking\n\n-Peff\n"},{"id":"496695","messageId":"20240608092855.GE2390433@coredump.intra.peff.net","threadId":"61594","inReplyTo":"xmqqwmn0eazm.fsf@gitster.g","subject":"Re: [PATCH v3 0/4] ci: detect more warnings via `-Og`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-08T09:28:55Z","receivedAt":"2024-06-08T09:28:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 07, 2024 at 01:47:25PM -0700, Junio C Hamano wrote:\n\n> I am not sure how annoying people will find the V=1 output. It is\n> irrelevant that it is in a collapsible section. What matters is if\n> it helps those who *need* to expand that collapsible section to take\n> a look, or if it clutteres what they have to wade through.\n> \n> When studying a build failure, I rarely found the exact command line\n> given by V=1 helpful, but YMMV---while I am not 100% convinced, let's\n> take the series as-is, because not losing information may sometimes\n> help even when we need to visually filter out extra clutter.\n\nI had the same thought. I have used V=1 for debugging, but usually\ndebugging Makefile changes locally (i.e., why is my option not being\npassed correctly). I don't think I've ever wanted it for a CI run.\n\nAnd I do think people see the output. It may be in a collapsible section\non the site, but:\n\n - you'd uncollapse that section if there is a build failure, and now\n your error messages are that much harder to find\n\n - if you look at the output outside of the site, you'll see the\n uncollapsed sections. And I usually view them in a local pager using\n curl[1].\n\nI guess I won't know until I see it in action, but I have a pretty\nstrong suspicion that it will be annoying.\n\n-Peff\n\n[1] For the curious, I use the script below to grab the logs from the\n last failed workflow run. It's pretty old, and I won't be surprised\n if there's a less ugly way to do it with the \"gh\" CLI tool or\n similar these days.\n\n-- >8 --\nrepo=peff/git\nGITHUB_TOKEN=...your.pat.here...\n\nactions() {\n curl -sL \\\n -H 'Accept: application/vnd.github.v3+json' \\\n -H \"Authorization: token $GITHUB_TOKEN\" \\\n \"https://api.github.com/repos/$repo/actions/$1\"\n}\n\nrun=$(\n actions runs?status=failure |\n jq -r '.workflow_runs | .[] | .id' |\n head -1\n)\n\nactions runs/$run/jobs?per_page=100 |\njq -r '.jobs | .[] | select(.conclusion == \"failure\") | \"\\(.id) \\(.name)\"' |\nwhile read id name; do\n {\n echo \"==> $name\"\n actions jobs/$id/logs\n } | less '+/not.ok|##\\[error\\]'\ndone\n"},{"id":"496723","messageId":"xmqq4ja3cl87.fsf@gitster.g","threadId":"61594","inReplyTo":"20240608085528.GD2390433@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/4] Makefile: add ability to append to CFLAGS and LDFLAGS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-08T19:01:28Z","receivedAt":"2024-06-08T19:01:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Another way to do this is just:\n>\n> diff --git a/Makefile b/Makefile\n> index 2f5f16847a..9cd3b252ff 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1446,8 +1446,8 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)\n> ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)\n> endif\n> \n> -ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)\n> -ALL_LDFLAGS = $(LDFLAGS)\n> +ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_APPEND)\n> +ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)\n\nMuch nicer.\n\n> I can't think offhand of any way that your override would not do the\n> right thing, but:\n>\n> - this is roughly the same problem faced by DEVELOPER_CFLAGS, etc, so\n> handling it in the same way makes sense to me\n>\n> - I always get nervous around make features like \"override\", as there\n> are sometimes corner cases lurking\n>\n> -Peff\n"},{"id":"496727","messageId":"xmqqsexnav1s.fsf@gitster.g","threadId":"61594","inReplyTo":"20240608092855.GE2390433@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/4] ci: detect more warnings via `-Og`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-08T23:12:15Z","receivedAt":"2024-06-08T23:12:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jun 07, 2024 at 01:47:25PM -0700, Junio C Hamano wrote:\n>\n>> I am not sure how annoying people will find the V=1 output. It is\n>> irrelevant that it is in a collapsible section. What matters is if\n>> it helps those who *need* to expand that collapsible section to take\n>> a look, or if it clutteres what they have to wade through.\n>> \n>> When studying a build failure, I rarely found the exact command line\n>> given by V=1 helpful, but YMMV---while I am not 100% convinced, let's\n>> take the series as-is, because not losing information may sometimes\n>> help even when we need to visually filter out extra clutter.\n>\n> I had the same thought. I have used V=1 for debugging, but usually\n> debugging Makefile changes locally (i.e., why is my option not being\n> passed correctly). I don't think I've ever wanted it for a CI run.\n>\n> And I do think people see the output. It may be in a collapsible section\n> on the site, but:\n>\n> - you'd uncollapse that section if there is a build failure, and now\n> your error messages are that much harder to find\n>\n> - if you look at the output outside of the site, you'll see the\n> uncollapsed sections. And I usually view them in a local pager using\n> curl[1].\n>\n> I guess I won't know until I see it in action, but I have a pretty\n> strong suspicion that it will be annoying.\n\nhttps://github.com/git/git/actions/runs/9424299208/job/25964282150#step:6:573\n\nI _knew_ that this run will fail compiling the updated timestamp\nparsing logic in date.c but it still took me a while to find the\nexact error.\n\nI typed \"date.o\" in the search box, which showed 5 hits (first two\nare false hits to fuzz-date.o and test-date.o), with\n\n 3rd hit on l.566 \"gcc -o date.o ... long long command line\"\n 4th hit on l.594 \"Makefile:2758: recipe for target 'date.o' failed\"\n 5th hit on l.595 \"make: *** [date.o] Error 1\"\n\nNitice that the error message with \"date.c\" is on 571 but with each\nline being very bloated to around 10 physical lines on screen, it is\nvery far from either 3rd or 4th hit.\n\nSo, this time it was annoying. But I suspect I'd be praising the\nwisdom of using V=1 if I were hunting for some breakage caused by\ntweaks in command line generation that broke the build or something,\nso I dunno.\n\n \n"},{"id":"496749","messageId":"ZmacSUI3d1nbx_F9@tanuki","threadId":"61594","inReplyTo":"xmqqsexnav1s.fsf@gitster.g","subject":"Re: [PATCH v3 0/4] ci: detect more warnings via `-Og`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-10T06:25:13Z","receivedAt":"2024-06-10T06:25:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Jun 08, 2024 at 04:12:15PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > On Fri, Jun 07, 2024 at 01:47:25PM -0700, Junio C Hamano wrote:\n> >\n> >> I am not sure how annoying people will find the V=1 output. It is\n> >> irrelevant that it is in a collapsible section. What matters is if\n> >> it helps those who *need* to expand that collapsible section to take\n> >> a look, or if it clutteres what they have to wade through.\n> >> \n> >> When studying a build failure, I rarely found the exact command line\n> >> given by V=1 helpful, but YMMV---while I am not 100% convinced, let's\n> >> take the series as-is, because not losing information may sometimes\n> >> help even when we need to visually filter out extra clutter.\n> >\n> > I had the same thought. I have used V=1 for debugging, but usually\n> > debugging Makefile changes locally (i.e., why is my option not being\n> > passed correctly). I don't think I've ever wanted it for a CI run.\n> >\n> > And I do think people see the output. It may be in a collapsible section\n> > on the site, but:\n> >\n> > - you'd uncollapse that section if there is a build failure, and now\n> > your error messages are that much harder to find\n> >\n> > - if you look at the output outside of the site, you'll see the\n> > uncollapsed sections. And I usually view them in a local pager using\n> > curl[1].\n> >\n> > I guess I won't know until I see it in action, but I have a pretty\n> > strong suspicion that it will be annoying.\n> \n> https://github.com/git/git/actions/runs/9424299208/job/25964282150#step:6:573\n> \n> I _knew_ that this run will fail compiling the updated timestamp\n> parsing logic in date.c but it still took me a while to find the\n> exact error.\n> \n> I typed \"date.o\" in the search box, which showed 5 hits (first two\n> are false hits to fuzz-date.o and test-date.o), with\n> \n> 3rd hit on l.566 \"gcc -o date.o ... long long command line\"\n> 4th hit on l.594 \"Makefile:2758: recipe for target 'date.o' failed\"\n> 5th hit on l.595 \"make: *** [date.o] Error 1\"\n> \n> Nitice that the error message with \"date.c\" is on 571 but with each\n> line being very bloated to around 10 physical lines on screen, it is\n> very far from either 3rd or 4th hit.\n> \n> So, this time it was annoying. But I suspect I'd be praising the\n> wisdom of using V=1 if I were hunting for some breakage caused by\n> tweaks in command line generation that broke the build or something,\n> so I dunno.\n\nI'll just drop this patch for now.\n\nPatrick\n"},{"id":"496750","messageId":"cover.1718001244.git.ps@pks.im","threadId":"61594","inReplyTo":"cover.1717655210.git.ps@pks.im","subject":"[PATCH v4 0/2] ci: detect more warnings via `-Og`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-10T06:38:15Z","receivedAt":"2024-06-10T06:38:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the fourth version of my patch series that starts to compile\nwith `-Og` in one of our CI jobs. This is done to surface more warnings\nfrom the compiler that might not show with other optimization levels.\n\nChanges compared to v3:\n\n - Remove the first patch as Junio has moved it into a separate\n topic so that it can be fast-tracked.\n\n - Drop the patch that changes CI jobs to compile with V=1.\n\n - Drop the `override` statement and instead implement CFLAGS_APPEND\n and LDFLAGS_APPEND via ALL_CFLAGS and ALL_LDFLAGS, respectively, as\n suggested by Peff.\n\nThe range diff is not all that useful for this version, and the patch\nseries is tiny, so I dropped it.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (2):\n Makefile: add ability to append to CFLAGS and LDFLAGS\n ci: compile \"linux-gcc-default\" job with -Og\n\n Makefile | 4 ++--\n ci/run-build-and-tests.sh | 9 +++++++++\n 2 files changed, 11 insertions(+), 2 deletions(-)\n-- \n2.45.2.436.gcd77e87115.dirty\n\n"},{"id":"496751","messageId":"8120ddaf0bdfd50e0fc4cf9a04f833102630b639.1718001244.git.ps@pks.im","threadId":"61594","inReplyTo":"cover.1718001244.git.ps@pks.im","subject":"[PATCH v4 1/2] Makefile: add ability to append to CFLAGS and LDFLAGS","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-10T06:38:20Z","receivedAt":"2024-06-10T06:38:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are some usecases where we may want to append CFLAGS to the\ndefault CFLAGS set by Git. This could for example be to enable or\ndisable specific compiler warnings or to change the optimization level\nthat code is compiled with. This cannot be done without overriding the\ncomplete CFLAGS value though and thus requires the user to redeclare the\ncomplete defaults used by Git.\n\nIntroduce a new variable `CFLAGS_APPEND` that gets appended to the\ndefault value of `CFLAGS`. As compiler options are last-one-wins, this\nfulfills both of the usecases mentioned above. It's also common practice\nacross many other projects to have such a variable.\n\nWhile at it, also introduce a matching `LDFLAGS_APPEND` variable. While\nthere isn't really any need for this variable as there are no default\n`LDFLAGS`, users may expect this variable to exist, as well.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 2f5f16847a..9cd3b252ff 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1446,8 +1446,8 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)\n ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)\n endif\n \n-ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)\n-ALL_LDFLAGS = $(LDFLAGS)\n+ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_APPEND)\n+ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)\n \n ifdef SANITIZE\n SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))\n-- \n2.45.2.436.gcd77e87115.dirty\n\n"},{"id":"496752","messageId":"03270d3414117ae7229d87127cff81e349557039.1718001244.git.ps@pks.im","threadId":"61594","inReplyTo":"cover.1718001244.git.ps@pks.im","subject":"[PATCH v4 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-10T06:38:25Z","receivedAt":"2024-06-10T06:38:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have recently noticed that our CI does not always notice variables\nthat may be used uninitialized. While it is expected that compiler\nwarnings aren't perfect, this one was a bit puzzling because it was\nrather obvious that the variable can be uninitialized.\n\nMany compiler warnings unfortunately depend on the optimization level\nused by the compiler. While `-O0` for example will disable a lot of\nwarnings altogether because optimization passes go away, `-O2`, which is\nour default optimization level used in CI, may optimize specific code\naway or even double down on undefined behaviour. Interestingly, this\nspecific instance that triggered the investigation does get noted by GCC\nwhen using `-Og`.\n\nWhile we could adapt all jobs to compile with `-Og` now, that would\npotentially mask other warnings that only get diagnosed with `-O2`.\nInstead, adapt the \"linux-gcc-default\" job to compile with `-Og`. This\njob is chosen because it uses the \"ubuntu:latest\" image and should thus\nhave a comparatively recent compiler toolchain, and because we have\nother jobs that use \"ubuntu:latest\" so that we do not lose coverage for\nwarnings diagnosed only on `-O2` level.\n\nTo make it easier to set up the optimization level in our CI, add\nsupport in our Makefile to specify the level via an environment\nvariable.\n\nSigned-off-by: Patrick Steinhardt \n---\n ci/run-build-and-tests.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex 98dda42045..40106c6c36 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -13,6 +13,15 @@ esac\n run_tests=t\n \n case \"$jobname\" in\n+linux-gcc-default)\n+\t# Warnings generated by compilers are unfortunately specific to the\n+\t# optimization level. With `-O0`, many warnings won't be shown at all,\n+\t# whereas the optimizations performed by our default optimization level\n+\t# `-O2` will mask others. We thus use `-Og` here just so that we have\n+\t# at least one job with a different optimization level so that we can\n+\t# overall surface more warnings.\n+\texport CFLAGS_APPEND=-Og\n+\t;;\n linux-gcc)\n \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n \t;;\n-- \n2.45.2.436.gcd77e87115.dirty\n\n"},{"id":"496755","messageId":"ZmaksqGEpLcMCjrn@tanuki","threadId":"61594","inReplyTo":"xmqq4ja3cl87.fsf@gitster.g","subject":"Re: [PATCH v3 2/4] Makefile: add ability to append to CFLAGS and LDFLAGS","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-10T07:01:06Z","receivedAt":"2024-06-10T07:01:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Jun 08, 2024 at 12:01:28PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > Another way to do this is just:\n> >\n> > diff --git a/Makefile b/Makefile\n> > index 2f5f16847a..9cd3b252ff 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -1446,8 +1446,8 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)\n> > ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)\n> > endif\n> > \n> > -ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)\n> > -ALL_LDFLAGS = $(LDFLAGS)\n> > +ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_APPEND)\n> > +ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)\n> \n> Much nicer.\n\nAgreed, this is much nicer. Will adapt, thanks!\n\nPatrick\n"},{"id":"496777","messageId":"xmqqed946auc.fsf@gitster.g","threadId":"61594","inReplyTo":"03270d3414117ae7229d87127cff81e349557039.1718001244.git.ps@pks.im","subject":"Re: [PATCH v4 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-10T16:06:51Z","receivedAt":"2024-06-10T16:06:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> While we could adapt all jobs to compile with `-Og` now, that would\n> potentially mask other warnings that only get diagnosed with `-O2`.\n\nI would say it is not just \"potentially\". It is likely that higher\noptimization levels will diagnose breakage more correctly, while\nlower optimization levels may trigger warnings more often. I\nsuspect a large part of those that lower optimization levels alone\nfind may be due to false positives.\n\nSo, ... I had a strong knee-jerk reaction against \"While we could\nadapt all\" with \"why would anybody sane would even want to do such a\nthing in the first place?\" If it were phrased like \"as we have\nplenty of jobs compiling with -O2, we can afford to switch a few of\nthem to different optimization levels, and we may find bugs that the\nexiting level did not notice, as the compilers' understanding of the\ndata and control flow is affected by these -O levels\", I would have\nunderstood it, and I think that is what happens in this patch.\n\nI just tried compiling pack-mtimes.c (untouced between maint and\nseen) with -Og using gcc-11, gcc-12, and gcc-13, using the exact\ncopy of the command line with V=1 output I see at the GitHub CI\n[*1*] (after all, this was a good use case, even though it was\ndropped from this round ;-)\n\nThe earlier versions of the compiler mark the use of mtimes_size\nthat is guarded in \"if (data)\" as maybe-uninitialized:\n\n pack-mtimes.c: In function ‘load_pack_mtimes_file’:\n pack-mtimes.c:89:25: error: ‘mtimes_size’ may be used uninitialized [-Werror=maybe-uninitialized]\n 89 | munmap(data, mtimes_size);\n | ^~~~~~~~~~~~~~~~~~~~~~~~~\n pack-mtimes.c:32:16: note: ‘mtimes_size’ was declared here\n 32 | size_t mtimes_size, expected_size;\n | ^~~~~~~~~~~\n cc1: all warnings being treated as errors\n make: *** [Makefile:2750: pack-mtimes.o] Error 1\n\nBut as I already reported elsewhere, this is a false positive. The\nplace \"data\" can possibly become non-NULL happens only after the\ncontrol passes the place mtimes_size get assigned a meaningful\nvalue. If the control reached here and data were non-NULL, the\ncontrol flow guarantees that mtimes_size has been assigned\nxsize_t(st.st_size) and cannot be uninitialized.\n\nWe can squelch the warning by initializing the variable to a\nmeaningless value, like 0, but I consider that such a change makes\nthe resulting code a lot worse than the original, and that is not\nbecause I do not want an extra and meaningless assignment emitted\n[*2*] in the resulting binary whose performance impact cannot be\nmeasured by anybody.\n\nIt is bad because it defeats efforts by compilers that understand\nthe data flow a bit better to diagnose a true \"used uninitialized\"\nbugs we may introduce in the future. Adding such a workaround goes\nagainst the longer-term health of the codebase.\n\nFor example, I could add another use of mtimes_size that is not\nguarded by \"if (data)\" right next to it, i.e.\n\n @@ -87,6 +87,7 @@ static int load_pack_mtimes_file(char *mtimes_file,\n if (ret) {\n if (data)\n munmap(data, mtimes_size);\n +\t\tprintf(\"debug %d\\n\", (int)mtimes_size);\n } else {\n *len_p = mtimes_size;\n *data_p = data;\n\ngcc-13 does notice that we are now truly using the variable\nuninitialized and flags it for us (gcc-12 also does notice but the\nimportant difference is that gcc-13 refrained from warning at the\nsafe use of the variable that is guarded by \"if (data)\").\n\nOnce we initialize the variable to meaninless 0 as a workaround,\nhowever, the compiler cannot help us with the \"used uninitialized\"\nwarning, and there is no \"the program uses the variable what was\ninitialized with meaningless value before it get set a meaningful\nvalue\" warning, unfortunately X-<. And that, not an extra\nassignment, is why such a workaround goes against the longer-term\nhealth of the codebase.\n\nBy the way, it is not just pack-mtimes.c [*3*]; there are quite a few\nobvious false positives, e.g.\n\nbuiltin/branch.c: In function ‘print_current_branch_name’:\nbuiltin/branch.c:509:17: error: ‘shortname’ may be used uninitialized [-Werror=maybe-uninitialized]\n 509 | puts(shortname);\n | ^~~~~~~~~~~~~~~\n\nwhere after skip_prefix() computed shortname like so:\n\n\telse if (skip_prefix(refname, \"refs/heads/\", &shortname))\n\t\tputs(shortname);\n\nand the compiler at that low optimization level does not even notice\nthat the static inline function skip_prefix() never returns true\nwithout updating its third parameter to a valid pointer X-<.\n\nAnd adding a workaround for each and every uses of variable that are\nset by skip_prefix() like so? I am not sure if that is a good move\n\nIf you saw some diagnosis that \"-Og\" made but not with higher\noptimization levels (with better understanding of the data and\ncontrol flow on the compiler's part), I am somewhat curious.\n\nWith gcc-13 everything seems to pass locally for me, so I personally\ndo not mind this patch, but from what I've seen \"gcc-12 -Wall -Og\"\nso far, especially given that GitHub CI (and GitLab CI as well?\notherwise you wouldn't be sending this change, I would imagine)\nseems to use a version of compiler whose \"-Og -Wall\" notices and\nflags these \"bugs\", I am not sure if we want to enable this option\nfor us. At least, -Werror and more stupid compilers is not a mix I\nwould want to live with and I am sure I would not want to add\nworkaround for such a mix to make our code worse (to end users and\nbuilders who want to use older comiplers with different optimization\nlevels, we can tell them not to enable -Werror in their build).\n\nAlso, with \"-Os\", we seem to find different set of \"bugs\" that \"-Og\"\ndid not see. For example, \"gcc-13 -Os -Wall\" flags that ppid may be\nuninitialized in compat/linux/procinfo.c:push_ancestry_name(), but\nthat is because it does not read stat_parent_pid() [*4*] and realize\nthat the only time ppid is left unset is when the function returns\n-1:\n\n static void push_ancestry_name(struct strvec *names, pid_t pid)\n {\n int ppid;\n\n if (stat_parent_pid(pid, &name, &ppid) < 0)\n goto cleanup;\n ...\n if (ppid)\n push_ancestry_name(names, ppid);\n cleanup:\n strbuf_release(&name);\n\t\t...\n\nIt is the same pattern as how \"gcc-12 -Og\" mishandles skip_prefix().\nAnother one \"-Os\" found is in builtin/fast-import.c:file_change_m()\nwhere the compiler does not notice that parse_mode() never returns\nnon-NULL without setting \"mode\". It is exactly the same pattern.\n\n\n[Footnotes]\n\n *1* https://github.com/git/git/actions/runs/9432273715/job/25981831882#step:4:132\n\n *2* We used to do the (IIRC GCC only) trick to initialize the\n variable to itself, i.e.\n\n @@ -29,7 +29,7 @@ static int load_pack_mtimes_file(char *mtimes_file,\n int fd, ret = 0;\n struct stat st;\n uint32_t *data = NULL;\n -\tsize_t mtimes_size, expected_size;\n +\tsize_t mtimes_size = mtimes_size, expected_size;\n struct mtimes_header header;\n\n fd = git_open(mtimes_file);\n\n to work around stupid compilers that flag a variable whose use\n is safe as potentially-used-uninitialized but these days we got\n rid of the trick as some other compiler did not like it (for\n legitimate reasons). Using the trick is not any better than\n initializing the variable to a meaningless value, like 0, as\n both will prevent smarter compilers from noticing real bugs\n (after all, that is the point of the workaround--not to flag\n use of variable the compiler thinks uninitialized as a bug).\n\n *3* I was planning to send \"these are horrible workarounds to work\n around the recent -Og build failures that has a huge negative\n consequences in the longer term\" patch on top of your series.\n\n But I do not think it is a good idea to add such a huge number\n of workarounds for various optimization levels. If we could\n add -Og with -Werror disabled, those who may be interested in\n sifting through false positives to find real errors may be able\n to do so without breaking build for others.\n\n *4* As \"-Os\" is for smaller code, I would imagine that it didn't\n try to auto inline the function. Come to think of it, \"-Og\" is\n to forbid optimizations that interfere with debugging, and code\n inlining may be considered as one, which would certainly\n explain why it has problem with skip_prefix().\n"},{"id":"496784","messageId":"xmqqjziw3arr.fsf@gitster.g","threadId":"61594","inReplyTo":"xmqqed946auc.fsf@gitster.g","subject":"[PATCH 1/2] DONTAPPLY: -Og fallout workaround","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-10T18:36:40Z","receivedAt":"2024-06-10T18:36:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These \"workarounds\" are to mark variables that are used after\ninitialized, but some compilers with lower optimization levels\ncannot see and report \"used uninitialized\".\n\nThis set targets \"gcc-12 -Og\". For the reason why this is a wrong\nthing to do for longer-term code health, see\n\n https://lore.kernel.org/git/xmqqed946auc.fsf@gitster.g/\n\nSigned-off-by: Junio C Hamano \n---\n\n * Even though I said I won't do the actual patch, since I had to\n gauge the extent of damage, I ended up doing so anyways.\n\n As I explained already, the size of this patch, i.e. number of\n places that need the workaround, does not really matter. What\n is horrible is how each of these workaround will hide real bugs\n we may introduce in the future from the compilers.\n\n builtin/branch.c | 2 +-\n builtin/fast-import.c | 4 ++--\n builtin/repack.c | 2 +-\n fetch-pack.c | 2 +-\n http-backend.c | 2 +-\n http.c | 2 +-\n pack-mtimes.c | 2 +-\n pack-revindex.c | 2 +-\n refs/packed-backend.c | 2 +-\n reftable/stack.c | 2 +-\n remote-curl.c | 4 ++--\n t/helper/test-ref-store.c | 2 +-\n trailer.c | 4 ++--\n 13 files changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 48cac74f97..1f4de92e05 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -504,7 +504,7 @@ static void print_current_branch_name(void)\n \tint flags;\n \tconst char *refname = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n \t\t\t\t\t\t \"HEAD\", 0, NULL, &flags);\n-\tconst char *shortname;\n+\tconst char *shortname = shortname;\n \tif (!refname)\n \t\tdie(_(\"could not resolve HEAD\"));\n \telse if (!(flags & REF_ISSYMREF))\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex ea5ae5196d..6d53e42011 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -1866,7 +1866,7 @@ static void parse_original_identifier(void)\n \n static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)\n {\n-\tconst char *data;\n+\tconst char *data = data;\n \tstrbuf_reset(sb);\n \n \tif (!skip_prefix(command_buf.buf, \"data \", &data))\n@@ -2807,7 +2807,7 @@ static void parse_new_commit(const char *arg)\n static void parse_new_tag(const char *arg)\n {\n \tstatic struct strbuf msg = STRBUF_INIT;\n-\tconst char *from;\n+\tconst char *from = from;\n \tchar *tagger;\n \tstruct branch *s;\n \tstruct tag *t;\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 7608430a37..e4752aae3f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1106,7 +1106,7 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \n static const char *find_pack_prefix(const char *packdir, const char *packtmp)\n {\n-\tconst char *pack_prefix;\n+\tconst char *pack_prefix = pack_prefix;\n \tif (!skip_prefix(packtmp, packdir, &pack_prefix))\n \t\tdie(_(\"pack prefix %s does not begin with objdir %s\"),\n \t\t packtmp, packdir);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex eba9e420ea..f8f318742f 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1517,7 +1517,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \n \tprocess_section_header(reader, \"shallow-info\", 0);\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n-\t\tconst char *arg;\n+\t\tconst char *arg = arg;\n \t\tstruct object_id oid;\n \n \t\tif (skip_prefix(reader->line, \"shallow \", &arg)) {\ndiff --git a/http-backend.c b/http-backend.c\nindex 5b4dca65ed..eecba3b44f 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -259,7 +259,7 @@ static void http_config(void)\n \n static struct rpc_service *select_service(struct strbuf *hdr, const char *name)\n {\n-\tconst char *svc_name;\n+\tconst char *svc_name = svc_name;\n \tstruct rpc_service *svc = NULL;\n \tint i;\n \ndiff --git a/http.c b/http.c\nindex 2dea2d03da..c45f91fa4e 100644\n--- a/http.c\n+++ b/http.c\n@@ -2156,7 +2156,7 @@ static int update_url_from_redirect(struct strbuf *base,\n \t\t\t\t const char *asked,\n \t\t\t\t const struct strbuf *got)\n {\n-\tconst char *tail;\n+\tconst char *tail = tail;\n \tsize_t new_len;\n \n \tif (!strcmp(asked, got->buf))\ndiff --git a/pack-mtimes.c b/pack-mtimes.c\nindex cdf30b8d2b..49598d4749 100644\n--- a/pack-mtimes.c\n+++ b/pack-mtimes.c\n@@ -29,7 +29,7 @@ static int load_pack_mtimes_file(char *mtimes_file,\n \tint fd, ret = 0;\n \tstruct stat st;\n \tuint32_t *data = NULL;\n-\tsize_t mtimes_size, expected_size;\n+\tsize_t mtimes_size = mtimes_size, expected_size;\n \tstruct mtimes_header header;\n \n \tfd = git_open(mtimes_file);\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex fc63aa76a2..7120bed5b1 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -206,7 +206,7 @@ static int load_revindex_from_disk(char *revindex_name,\n \tint fd, ret = 0;\n \tstruct stat st;\n \tvoid *data = NULL;\n-\tsize_t revindex_size;\n+\tsize_t revindex_size = revindex_size;\n \tstruct revindex_header *hdr;\n \n \tif (git_env_bool(GIT_TEST_REV_INDEX_DIE_ON_DISK, 0))\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex c4c1e36aa2..25321a5758 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -677,7 +677,7 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n \n \t/* If the file has a header line, process it: */\n \tif (snapshot->buf < snapshot->eof && *snapshot->buf == '#') {\n-\t\tchar *tmp, *p, *eol;\n+\t\tchar *tmp, *p = p, *eol;\n \t\tstruct string_list traits = STRING_LIST_INIT_NODUP;\n \n \t\teol = memchr(snapshot->buf, '\\n',\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 737591125e..aa1851fd73 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1231,7 +1231,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n,\n \t\t\t\t\t uint8_t factor)\n {\n \tstruct segment seg = { 0 };\n-\tuint64_t bytes;\n+\tuint64_t bytes = bytes;\n \tsize_t i;\n \n \tif (!factor)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 827bd848ae..4f9808c63f 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1265,7 +1265,7 @@ static void parse_fetch(struct strbuf *buf)\n \tint alloc_heads = 0, nr_heads = 0;\n \n \tdo {\n-\t\tconst char *p;\n+\t\tconst char *p = p;\n \t\tif (skip_prefix(buf->buf, \"fetch \", &p)) {\n \t\t\tconst char *name;\n \t\t\tstruct ref *ref;\n@@ -1428,7 +1428,7 @@ static void parse_push(struct strbuf *buf)\n \tint ret;\n \n \tdo {\n-\t\tconst char *arg;\n+\t\tconst char *arg = arg;\n \t\tif (skip_prefix(buf->buf, \"push \", &arg))\n \t\t\tstrvec_push(&specs, arg);\n \t\telse\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex ad24300170..7d3870fe76 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -66,7 +66,7 @@ static unsigned int arg_flags(const char *arg, const char *name,\n \n static const char **get_store(const char **argv, struct ref_store **refs)\n {\n-\tconst char *gitdir;\n+\tconst char *gitdir = gitdir;\n \n \tif (!argv[0]) {\n \t\tdie(\"ref store required\");\ndiff --git a/trailer.c b/trailer.c\nindex 72e5136c73..5056c16644 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -508,11 +508,11 @@ static int git_trailer_config(const char *conf_key, const char *value,\n \t\t\t const struct config_context *ctx UNUSED,\n \t\t\t void *cb UNUSED)\n {\n-\tconst char *trailer_item, *variable_name;\n+\tconst char *trailer_item = trailer_item, *variable_name;\n \tstruct arg_item *item;\n \tstruct conf_info *conf;\n \tchar *name = NULL;\n-\tenum trailer_info_type type;\n+\tenum trailer_info_type type = type;\n \tint i;\n \n \tif (!skip_prefix(conf_key, \"trailer.\", &trailer_item))\n-- \n2.45.2-445-g1b76f06508\n\n"},{"id":"496785","messageId":"xmqqcyoo3ark.fsf@gitster.g","threadId":"61594","inReplyTo":"xmqqed946auc.fsf@gitster.g","subject":"[PATCH 2/2] DONTAPPLY: -Os fallout workaround","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-10T18:36:47Z","receivedAt":"2024-06-10T18:36:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These \"workarounds\" are to mark variables that are used after\ninitialized, but some compilers with lower optimization levels\ncannot see and report \"used uninitialized\".\n\nThis set targets \"gcc-13 -Os\". For the reason why this is a wrong\nthing to do for longer-term code health, see\n\n https://lore.kernel.org/git/xmqqed946auc.fsf@gitster.g/\n\nSigned-off-by: Junio C Hamano \n---\n builtin/fast-import.c | 2 +-\n compat/linux/procinfo.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 6d53e42011..109b0facd5 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -2320,7 +2320,7 @@ static void file_change_m(const char *p, struct branch *b)\n \tstatic struct strbuf path = STRBUF_INIT;\n \tstruct object_entry *oe;\n \tstruct object_id oid;\n-\tuint16_t mode, inline_data = 0;\n+\tuint16_t mode = mode, inline_data = 0;\n \n \tp = parse_mode(p, &mode);\n \tif (!p)\ndiff --git a/compat/linux/procinfo.c b/compat/linux/procinfo.c\nindex 4bb2d66227..17937906b8 100644\n--- a/compat/linux/procinfo.c\n+++ b/compat/linux/procinfo.c\n@@ -129,7 +129,7 @@ static int stat_parent_pid(pid_t pid, struct strbuf *name, int *statppid)\n static void push_ancestry_name(struct strvec *names, pid_t pid)\n {\n \tstruct strbuf name = STRBUF_INIT;\n-\tint ppid;\n+\tint ppid = ppid;\n \n \tif (stat_parent_pid(pid, &name, &ppid) < 0)\n \t\tgoto cleanup;\n-- \n2.45.2-445-g1b76f06508\n\n"},{"id":"496793","messageId":"xmqqsexk1s43.fsf@gitster.g","threadId":"61594","inReplyTo":"xmqqjziw3arr.fsf@gitster.g","subject":"Re: [PATCH 1/2] DONTAPPLY: -Og fallout workaround","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-10T20:05:00Z","receivedAt":"2024-06-10T20:05:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> These \"workarounds\" are to mark variables that are used after\n> initialized, but some compilers with lower optimization levels\n> cannot see and report \"used uninitialized\".\n>\n> This set targets \"gcc-12 -Og\". For the reason why this is a wrong\n> thing to do for longer-term code health, see\n>\n> https://lore.kernel.org/git/xmqqed946auc.fsf@gitster.g/\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * Even though I said I won't do the actual patch, since I had to\n> gauge the extent of damage, I ended up doing so anyways.\n>\n> As I explained already, the size of this patch, i.e. number of\n> places that need the workaround, does not really matter. What\n> is horrible is how each of these workaround will hide real bugs\n> we may introduce in the future from the compilers.\n>\n> builtin/branch.c | 2 +-\n> builtin/fast-import.c | 4 ++--\n> builtin/repack.c | 2 +-\n> fetch-pack.c | 2 +-\n> http-backend.c | 2 +-\n> http.c | 2 +-\n> pack-mtimes.c | 2 +-\n> pack-revindex.c | 2 +-\n> refs/packed-backend.c | 2 +-\n> reftable/stack.c | 2 +-\n> remote-curl.c | 4 ++--\n> t/helper/test-ref-store.c | 2 +-\n> trailer.c | 4 ++--\n> 13 files changed, 16 insertions(+), 16 deletions(-)\n\nAnd depending on the version of compilers, apparently even this is\nnot enough. I do not offhand know what GitHub CI is running for\nlinux-gcc-default (ubuntu-latest), but this gets flagged for using\n(try to guess which one without looking at the answer below) ...\n\n static int parse_count(const char *arg)\n {\n int count;\n\n if (strtol_i(arg, 10, &count) < 0)\n die(\"'%s': not an integer\", arg);\n return count;\n }\n\n... count uninitilaized, since the compiler does not realize that\nstrtol_i() always touches \"count\" unless the function returns\nnegative, and die() never returns. Exactly the same pattern\ncontinues.\n\nSo, unless we disable -Werror, let's not continue this experiment\nwith -Og or -Os as the damage seems to be far greater than the\nbenefit (which I haven't seen any, but that is largely due to\ntimezone differences---I asked \"what's the real bug you found with\nthis\" a few hours ago that is past EOB in Europe).\n\n"},{"id":"496914","messageId":"Zmg-jl83UA0P2Dnk@tanuki","threadId":"61594","inReplyTo":"xmqqsexk1s43.fsf@gitster.g","subject":"Re: [PATCH 1/2] DONTAPPLY: -Og fallout workaround","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-11T12:09:50Z","receivedAt":"2024-06-11T12:09:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jun 10, 2024 at 01:05:00PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > These \"workarounds\" are to mark variables that are used after\n> > initialized, but some compilers with lower optimization levels\n> > cannot see and report \"used uninitialized\".\n> >\n> > This set targets \"gcc-12 -Og\". For the reason why this is a wrong\n> > thing to do for longer-term code health, see\n> >\n> > https://lore.kernel.org/git/xmqqed946auc.fsf@gitster.g/\n> >\n> > Signed-off-by: Junio C Hamano \n> > ---\n> >\n> > * Even though I said I won't do the actual patch, since I had to\n> > gauge the extent of damage, I ended up doing so anyways.\n> >\n> > As I explained already, the size of this patch, i.e. number of\n> > places that need the workaround, does not really matter. What\n> > is horrible is how each of these workaround will hide real bugs\n> > we may introduce in the future from the compilers.\n> >\n> > builtin/branch.c | 2 +-\n> > builtin/fast-import.c | 4 ++--\n> > builtin/repack.c | 2 +-\n> > fetch-pack.c | 2 +-\n> > http-backend.c | 2 +-\n> > http.c | 2 +-\n> > pack-mtimes.c | 2 +-\n> > pack-revindex.c | 2 +-\n> > refs/packed-backend.c | 2 +-\n> > reftable/stack.c | 2 +-\n> > remote-curl.c | 4 ++--\n> > t/helper/test-ref-store.c | 2 +-\n> > trailer.c | 4 ++--\n> > 13 files changed, 16 insertions(+), 16 deletions(-)\n> \n> And depending on the version of compilers, apparently even this is\n> not enough. I do not offhand know what GitHub CI is running for\n> linux-gcc-default (ubuntu-latest), but this gets flagged for using\n> (try to guess which one without looking at the answer below) ...\n> \n> static int parse_count(const char *arg)\n> {\n> int count;\n> \n> if (strtol_i(arg, 10, &count) < 0)\n> die(\"'%s': not an integer\", arg);\n> return count;\n> }\n> \n> ... count uninitilaized, since the compiler does not realize that\n> strtol_i() always touches \"count\" unless the function returns\n> negative, and die() never returns. Exactly the same pattern\n> continues.\n> \n> So, unless we disable -Werror, let's not continue this experiment\n> with -Og or -Os as the damage seems to be far greater than the\n> benefit (which I haven't seen any, but that is largely due to\n> timezone differences---I asked \"what's the real bug you found with\n> this\" a few hours ago that is past EOB in Europe).\n\nThe real bug that \"-Og\" would have been able to detect was reported by\nPeff via [1]. In this case it wasn't \"-Og\" that detected it, but\nCoverity did. But it would have been detected if we had a job that\ncompiled with \"-Og\".\n\nBut now that I see the full picture of this with different compiler\noptions I have to agree that this is not really worth it. Especially not\ngiven that Coverity is able to detect such cases, even though that only\nhappens retroactively after a topic has landed.\n\nLet's drop this experiment.\n\nPatrick\n\n[1]: 20240605100728.GA3440281@coredump.intra.peff.net\n"},{"id":"496934","messageId":"xmqqtthzwfnz.fsf@gitster.g","threadId":"61594","inReplyTo":"Zmg-jl83UA0P2Dnk@tanuki","subject":"Re: [PATCH 1/2] DONTAPPLY: -Og fallout workaround","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-11T17:30:24Z","receivedAt":"2024-06-11T17:30:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The real bug that \"-Og\" would have been able to detect was reported by\n> Peff via [1]. In this case it wasn't \"-Og\" that detected it, but\n> Coverity did. But it would have been detected if we had a job that\n> compiled with \"-Og\".\n\nThanks. It is curious that you singled out \"-Og\" here, but with the\nusual \"-O2\" optimization level, it does not seem to get noticed.\n\n> But now that I see the full picture of this with different compiler\n> options I have to agree that this is not really worth it. Especially not\n> given that Coverity is able to detect such cases, even though that only\n> happens retroactively after a topic has landed.\n>\n> Let's drop this experiment.\n\nOK, but let's keep the CFLAGS_APPEND one ;-)\n"},{"id":"496976","messageId":"ZmknNDRnYGCeqAoQ@tanuki","threadId":"61594","inReplyTo":"xmqqtthzwfnz.fsf@gitster.g","subject":"Re: [PATCH 1/2] DONTAPPLY: -Og fallout workaround","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-12T04:42:28Z","receivedAt":"2024-06-12T04:42:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jun 11, 2024 at 10:30:24AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> > But now that I see the full picture of this with different compiler\n> > options I have to agree that this is not really worth it. Especially not\n> > given that Coverity is able to detect such cases, even though that only\n> > happens retroactively after a topic has landed.\n> >\n> > Let's drop this experiment.\n> \n> OK, but let's keep the CFLAGS_APPEND one ;-)\n\nSure, let's do that. I assume you'll just cherry-pick that single\ncommit?\n\nPatrick\n"},{"id":"496977","messageId":"Zmkn0oWnWvVB6Nej@tanuki","threadId":"61594","inReplyTo":"ZmknNDRnYGCeqAoQ@tanuki","subject":"Re: [PATCH 1/2] DONTAPPLY: -Og fallout workaround","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-12T04:45:06Z","receivedAt":"2024-06-12T04:45:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jun 12, 2024 at 06:42:28AM +0200, Patrick Steinhardt wrote:\n> On Tue, Jun 11, 2024 at 10:30:24AM -0700, Junio C Hamano wrote:\n> > Patrick Steinhardt writes:\n> > > But now that I see the full picture of this with different compiler\n> > > options I have to agree that this is not really worth it. Especially not\n> > > given that Coverity is able to detect such cases, even though that only\n> > > happens retroactively after a topic has landed.\n> > >\n> > > Let's drop this experiment.\n> > \n> > OK, but let's keep the CFLAGS_APPEND one ;-)\n> \n> Sure, let's do that. I assume you'll just cherry-pick that single\n> commit?\n\nAh, just spotted that you already did. Thanks!\n\nPatrick\n"},{"id":"497053","messageId":"xmqqo785olpp.fsf@gitster.g","threadId":"61594","inReplyTo":"xmqqed946auc.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-12T22:11:30Z","receivedAt":"2024-06-12T22:11:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> We can squelch the warning by initializing the variable to a\n> meaningless value, like 0, but I consider that such a change makes\n> the resulting code a lot worse than the original, and that is not\n> because I do not want an extra and meaningless assignment emitted\n> [*2*] in the resulting binary whose performance impact cannot be\n> measured by anybody.\n>\n> It is bad because it defeats efforts by compilers that understand\n> the data flow a bit better to diagnose a true \"used uninitialized\"\n> bugs we may introduce in the future. Adding such a workaround goes\n> against the longer-term health of the codebase.\n>\n> For example, I could add another use of mtimes_size that is not\n> guarded by \"if (data)\" right next to it, i.e.\n>\n> @@ -87,6 +87,7 @@ static int load_pack_mtimes_file(char *mtimes_file,\n> if (ret) {\n> if (data)\n> munmap(data, mtimes_size);\n> +\t\tprintf(\"debug %d\\n\", (int)mtimes_size);\n> } else {\n> *len_p = mtimes_size;\n> *data_p = data;\n>\n> gcc-13 does notice that we are now truly using the variable\n> uninitialized and flags it for us (gcc-12 also does notice but the\n> important difference is that gcc-13 refrained from warning at the\n> safe use of the variable that is guarded by \"if (data)\").\n\nBy the way, I do not know if any compiler gives us such a feature,\nbut if the trick to squelch a false positive were finer grained, I\nwould have been much more receptive to the idea of building with\ndifferent optimization level, allowing a bit more false positives.\n\nThe workaround everybody jumps at is to initialize the variable to a\nmeaningless value (like 0) and I have explained why it is suboptimal\nalready. But if we can tell less intelligent compilers \"we know our\nuse of this variable AT THIS POINT is safe\", e.g. by annotating the\nabove snippet of the code, perhaps like this:\n\n if (ret) {\n if (data)\n\t\t\t\t/* -Wno-uninitialized (mtimes_size) */\n munmap(data, mtimes_size);\n\t\t\tprintf(\"debug %d\\n\", (int)mtimes_size);\n\nthen it would be clear to the compiler that understand the\nannotation that inside that \"if (data)\" block, we know that\nthe use of mtimes_size is not using an uninitialized variable.\n\nFor the use of the same variable on the next \"debug\" line, because\nit is outside of that \"if (data)\" block, the annotation should have\nno effect, and the compiler is free to do its own analysis and we\nwill accept if it finds mtimes_size can be used uninitialized there.\nAny new use added for the same variable will not be masked by a\nmeaningless initialization if we can use such a \"workaround\" to\nsquelch false positives.\n"},{"id":"497087","messageId":"20240613101522.GC817573@coredump.intra.peff.net","threadId":"61594","inReplyTo":"xmqqo785olpp.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-13T10:15:22Z","receivedAt":"2024-06-13T10:15:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 12, 2024 at 03:11:30PM -0700, Junio C Hamano wrote:\n\n> By the way, I do not know if any compiler gives us such a feature,\n> but if the trick to squelch a false positive were finer grained, I\n> would have been much more receptive to the idea of building with\n> different optimization level, allowing a bit more false positives.\n> \n> The workaround everybody jumps at is to initialize the variable to a\n> meaningless value (like 0) and I have explained why it is suboptimal\n> already. But if we can tell less intelligent compilers \"we know our\n> use of this variable AT THIS POINT is safe\", e.g. by annotating the\n> above snippet of the code, perhaps like this:\n> \n> if (ret) {\n> if (data)\n> \t\t\t\t/* -Wno-uninitialized (mtimes_size) */\n> munmap(data, mtimes_size);\n> \t\t\tprintf(\"debug %d\\n\", (int)mtimes_size);\n> \n> then it would be clear to the compiler that understand the\n> annotation that inside that \"if (data)\" block, we know that\n> the use of mtimes_size is not using an uninitialized variable.\n> \n> For the use of the same variable on the next \"debug\" line, because\n> it is outside of that \"if (data)\" block, the annotation should have\n> no effect, and the compiler is free to do its own analysis and we\n> will accept if it finds mtimes_size can be used uninitialized there.\n> Any new use added for the same variable will not be masked by a\n> meaningless initialization if we can use such a \"workaround\" to\n> squelch false positives.\n\nI agree that such an annotation is much more focused. It's still not\nfoolproof, though (e.g., we might chance earlier code so that the\ndata/mtimes_size correlation is no longer true).\n\nI think you could do it with:\n\n\t\t\tif (data)\n#pragma GCC diagnostic push\n#pragma GCC diagnostic ignored \"-Wuninitialized\"\n\t\t\t\tmunmap(data, mtimes_size);\n#pragma GCC diagnostic pop\n\nwhich is...ugly. There's a _Pragma() operator, too, which I think would\nlet you make a macro like:\n\n\t\t\tif (data)\n\t\t\t\tSUPPRESS(\"-Wuninitialized\", munmap(data, mtimes_size));\n\nwhich is maybe slightly less horrific? Still pretty magical though.\n\nBut if the alternative is to do none of that, and just continue to avoid\nlooking for warnings with -Os, I prefer that.\n\n-Peff\n"},{"id":"497094","messageId":"xmqqr0d0n8tn.fsf@gitster.g","threadId":"61594","inReplyTo":"20240613101522.GC817573@coredump.intra.peff.net","subject":"Re: [PATCH v4 2/2] ci: compile \"linux-gcc-default\" job with -Og","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-13T15:47:32Z","receivedAt":"2024-06-13T15:47:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think you could do it with:\n>\n> \t\t\tif (data)\n> #pragma GCC diagnostic push\n> #pragma GCC diagnostic ignored \"-Wuninitialized\"\n> \t\t\t\tmunmap(data, mtimes_size);\n> #pragma GCC diagnostic pop\n>\n> which is...ugly. There's a _Pragma() operator, too, which I think would\n> let you make a macro like:\n>\n> \t\t\tif (data)\n> \t\t\t\tSUPPRESS(\"-Wuninitialized\", munmap(data, mtimes_size));\n>\n> which is maybe slightly less horrific? Still pretty magical though.\n>\n> But if the alternative is to do none of that, and just continue to avoid\n> looking for warnings with -Os, I prefer that.\n\nOh, we are in agreement. Such effort to annotate code and tolerate\ninconvenience is better spent elsewhere but expertise and competence\nare not as fungible as I wish them to be ;-)\n\nThanks.\n"}]}