Git development
 help / color / mirror / Atom feed
* [PATCH 0/2] bump static-analysis ci image version
@ 2026-07-26  8:32 Jeff King
  2026-07-26  8:37 ` [PATCH 1/2] bloom: silence CHECK_ASSERTION_SIDE_EFFECTS false positive Jeff King
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Jeff King @ 2026-07-26  8:32 UTC (permalink / raw)
  To: git; +Cc: tnyman, Taylor Blau, Junio C Hamano, Elijah Newren

This is another way to fix the slow coccinelle run discussed in:

  https://lore.kernel.org/git/20260724091152.27794-2-tnyman@openai.com/

by using a newer version of coccinelle.

We tweaked the code there to avoid the problem, so this isn't urgent.
But it is worth doing to avoid running into the same problem again (and
because in general I think it makes sense to run newer versions of our
dev tools than older ones).

The second patch is the interesting one. The first is a necessary
clean-up (+cc Elijah as the relevant author there).

  [1/2]: bloom: silence CHECK_ASSERTION_SIDE_EFFECTS false positive
  [2/2]: ci: bump ubuntu image version for static-analysis job

 .github/workflows/main.yml | 4 ++--
 .gitlab-ci.yml             | 2 +-
 bloom.c                    | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

-Peff

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] bloom: silence CHECK_ASSERTION_SIDE_EFFECTS false positive
  2026-07-26  8:32 [PATCH 0/2] bump static-analysis ci image version Jeff King
@ 2026-07-26  8:37 ` Jeff King
  2026-07-26  8:39 ` [PATCH 2/2] ci: bump ubuntu image version for static-analysis job Jeff King
  2026-07-26 16:34 ` [PATCH 0/2] bump static-analysis ci image version Junio C Hamano
  2 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2026-07-26  8:37 UTC (permalink / raw)
  To: git; +Cc: tnyman, Taylor Blau, Junio C Hamano, Elijah Newren

Using gcc 15, compiling with CHECK_ASSERTION_SIDE_EFFECTS=1 causes a
complaint about this line in bloom.c having a side effect:

	assert(version == 1 || version == 2);

I think this is pretty clearly a false positive, as those comparisons
should not have side effects. The side-effect checker uses a magic
definition of assert() that relies on the compiler's optimizer to drop a
reference to an otherwise unused variable. And for whatever reason, gcc
chooses not to do so here under -O2 (side note: if you have -O0 in your
CFLAGS, that naturally creates many more false positives!).

This code has been around for a while, but nobody seems to have noticed
because we use an older version of the compiler in our static-analysis
ci job, and it does not complain. Presumably very few people run this
check locally on their more modern compilers.

Let's silence the false positive to avoid confusion for anyone running
locally, and to make it possible to upgrade the image we use for our
static-analysis job.

We could just switch to our custom ASSERT() here, but I think we can
improve the code by integrating the assertion into the if/else cascade.
That avoids repeating the logic about which versions are acceptable.

Signed-off-by: Jeff King <peff@peff.net>
---
Building with clang or with "gcc -flto" seems to also silence the false
positive. We might consider using those for the static-analysis job.
But since in this instance we can both silence it and (IMHO) make the
code nicer to read, I think it's reasonable to do so. We can leave
tinkering with the assert() magic as a separate topic for anyone
interested.

 bloom.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bloom.c b/bloom.c
index c98d1672ad..caf22f9831 100644
--- a/bloom.c
+++ b/bloom.c
@@ -610,10 +610,10 @@ int bloom_filter_contains_vec(const struct bloom_filter *filter,
 uint32_t test_bloom_murmur3_seeded(uint32_t seed, const char *data, size_t len,
 				   int version)
 {
-	assert(version == 1 || version == 2);
-
 	if (version == 2)
 		return murmur3_seeded_v2(seed, data, len);
-	else
+	else if (version == 1)
 		return murmur3_seeded_v1(seed, data, len);
+	else
+		BUG("unexpected bloom version: %d", version);
 }
-- 
2.55.0.742.gf2bff09aa6


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] ci: bump ubuntu image version for static-analysis job
  2026-07-26  8:32 [PATCH 0/2] bump static-analysis ci image version Jeff King
  2026-07-26  8:37 ` [PATCH 1/2] bloom: silence CHECK_ASSERTION_SIDE_EFFECTS false positive Jeff King
@ 2026-07-26  8:39 ` Jeff King
  2026-08-07 10:24   ` Patrick Steinhardt
                     ` (2 more replies)
  2026-07-26 16:34 ` [PATCH 0/2] bump static-analysis ci image version Junio C Hamano
  2 siblings, 3 replies; 8+ messages in thread
From: Jeff King @ 2026-07-26  8:39 UTC (permalink / raw)
  To: git; +Cc: tnyman, Taylor Blau, Junio C Hamano, Elijah Newren

We recently ran into a case[1] where old versions of coccinelle ran very
slowly, but newer ones are fine. The version we use in GitHub's CI was
the old slow version, leading to timeouts of the static-analysis job.

We get the old version because we ask for the ubuntu-22.04 image. That
has coccinelle 1.1.1, but the "fast" improvement is in coccinelle 1.3.0,
specifically their 58619b8fe (break up envs for e1 & e2, 2024-08-18).

Bumping to ubuntu-25.10 would be enough to get that new version. But I
don't see any need to ask for a specific version at all. We originally
used a specific version because coccinelle wasn't available in ubuntu
20.04, so we pinned to 18.04 in d051ed77ee (.github/workflows/main.yml:
run static-analysis on bionic, 2021-02-08). Later that got bumped in
ef46584831 (ci: update 'static-analysis' to Ubuntu 22.04, 2022-08-23)
when 18.04 support was dropped.

It seems like the absence of coccinelle was a blip in 20.04, and we can
just stick with "latest" going forward.

I tested the result on GitHub's CI. I bumped the matching line in the
GitLab definition, but didn't have a simple means of testing (but it's
such a trivial change nothing could go wrong, right?).

[1] https://lore.kernel.org/git/20260724091152.27794-2-tnyman@openai.com/

Signed-off-by: Jeff King <peff@peff.net>
---
 .github/workflows/main.yml | 4 ++--
 .gitlab-ci.yml             | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 85cfedf5b0..205325eb33 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -460,8 +460,8 @@ jobs:
     if: needs.ci-config.outputs.enabled == 'yes'
     env:
       jobname: StaticAnalysis
-      CI_JOB_IMAGE: ubuntu-22.04
-    runs-on: ubuntu-22.04
+      CI_JOB_IMAGE: ubuntu-latest
+    runs-on: ubuntu-latest
     concurrency:
       group: static-analysis-${{ github.ref }}
       cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 1c4d04da9d..0242283c3c 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -227,7 +227,7 @@ test:fuzz-smoke-tests:
     - ./ci/run-build-and-minimal-fuzzers.sh
 
 static-analysis:
-  image: ubuntu:22.04
+  image: ubuntu:latest
   stage: analyze
   needs: [ ]
   variables:
-- 
2.55.0.742.gf2bff09aa6

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] bump static-analysis ci image version
  2026-07-26  8:32 [PATCH 0/2] bump static-analysis ci image version Jeff King
  2026-07-26  8:37 ` [PATCH 1/2] bloom: silence CHECK_ASSERTION_SIDE_EFFECTS false positive Jeff King
  2026-07-26  8:39 ` [PATCH 2/2] ci: bump ubuntu image version for static-analysis job Jeff King
@ 2026-07-26 16:34 ` Junio C Hamano
  2 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2026-07-26 16:34 UTC (permalink / raw)
  To: Jeff King; +Cc: git, tnyman, Taylor Blau, Elijah Newren

Jeff King <peff@peff.net> writes:

> This is another way to fix the slow coccinelle run discussed in:
>
>   https://lore.kernel.org/git/20260724091152.27794-2-tnyman@openai.com/
>
> by using a newer version of coccinelle.
>
> We tweaked the code there to avoid the problem, so this isn't urgent.
> But it is worth doing to avoid running into the same problem again (and
> because in general I think it makes sense to run newer versions of our
> dev tools than older ones).
>
> The second patch is the interesting one. The first is a necessary
> clean-up (+cc Elijah as the relevant author there).

Both are interesting.  Thanks for doing this (with relevant
archaeology as usual).

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] ci: bump ubuntu image version for static-analysis job
  2026-07-26  8:39 ` [PATCH 2/2] ci: bump ubuntu image version for static-analysis job Jeff King
@ 2026-08-07 10:24   ` Patrick Steinhardt
  2026-08-07 16:16     ` Junio C Hamano
  2026-08-07 16:47   ` Elijah Newren
  2026-08-08 17:31   ` SZEDER Gábor
  2 siblings, 1 reply; 8+ messages in thread
From: Patrick Steinhardt @ 2026-08-07 10:24 UTC (permalink / raw)
  To: Jeff King; +Cc: git, tnyman, Taylor Blau, Junio C Hamano, Elijah Newren

On Sun, Jul 26, 2026 at 04:39:05AM -0400, Jeff King wrote:
> We recently ran into a case[1] where old versions of coccinelle ran very
> slowly, but newer ones are fine. The version we use in GitHub's CI was
> the old slow version, leading to timeouts of the static-analysis job.
> 
> We get the old version because we ask for the ubuntu-22.04 image. That
> has coccinelle 1.1.1, but the "fast" improvement is in coccinelle 1.3.0,
> specifically their 58619b8fe (break up envs for e1 & e2, 2024-08-18).

I have been wondering about slow Coccinelle for a while now. Making
things faster via a simple version upgrade is great, as it comes almost
for free.

But that being said, we also have a bunch of Coccinelle rules nowadays,
and my gut feeling tells me that there's a bunch of them that aren't
useful anymore. "refs", "object_id", "the_repository",
"git_config_number", "index-compatibility" and "context_fn_ctx" all look
like files that we could probably just get rid of because we have long
done the migrations, and it's unlikely anybody still has patches that
use the pre-migration variants.

They'd of course require a bit of a deeper look, but that could be
another way to speed up Coccinelle for us. Even though I cannot say for
sure by how much, I didn't give it a test.

Thanks!

Patrick

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] ci: bump ubuntu image version for static-analysis job
  2026-08-07 10:24   ` Patrick Steinhardt
@ 2026-08-07 16:16     ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2026-08-07 16:16 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: Jeff King, git, tnyman, Taylor Blau, Elijah Newren

Patrick Steinhardt <ps@pks.im> writes:

> They'd of course require a bit of a deeper look, but that could be
> another way to speed up Coccinelle for us. Even though I cannot say for
> sure by how much, I didn't give it a test.

Another benefit is that it would reduce the programmer's burden, as
it is not immediately apparent which rules are still relevant.

I wonder if we can easily define the exit criteria when we introduce
a new rule and document them, immediately next to the rules.

You said "refs, object_id, the_repository, ... all look like we have
long done with the migrations"; in retrospect, would it have been
easily doable for those who introduced these rules to describe how
we would declare "now migration is done"?  If so, perhaps a good
step forward may be to update tools/coccinelle/README to add such a
rule.

    ... goes and looks ...

The readme file clearly states that transformations needed for
migrations are *not* regularly run.  Is it possible that we have
these rules you mentioned misclassified?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] ci: bump ubuntu image version for static-analysis job
  2026-07-26  8:39 ` [PATCH 2/2] ci: bump ubuntu image version for static-analysis job Jeff King
  2026-08-07 10:24   ` Patrick Steinhardt
@ 2026-08-07 16:47   ` Elijah Newren
  2026-08-08 17:31   ` SZEDER Gábor
  2 siblings, 0 replies; 8+ messages in thread
From: Elijah Newren @ 2026-08-07 16:47 UTC (permalink / raw)
  To: Jeff King; +Cc: git, tnyman, Taylor Blau, Junio C Hamano

On Sun, Jul 26, 2026 at 1:39 AM Jeff King <peff@peff.net> wrote:
>
> We recently ran into a case[1] where old versions of coccinelle ran very
> slowly, but newer ones are fine. The version we use in GitHub's CI was
> the old slow version, leading to timeouts of the static-analysis job.
>
> We get the old version because we ask for the ubuntu-22.04 image. That
> has coccinelle 1.1.1, but the "fast" improvement is in coccinelle 1.3.0,
> specifically their 58619b8fe (break up envs for e1 & e2, 2024-08-18).
>
> Bumping to ubuntu-25.10 would be enough to get that new version. But I
> don't see any need to ask for a specific version at all. We originally
> used a specific version because coccinelle wasn't available in ubuntu
> 20.04, so we pinned to 18.04 in d051ed77ee (.github/workflows/main.yml:
> run static-analysis on bionic, 2021-02-08). Later that got bumped in
> ef46584831 (ci: update 'static-analysis' to Ubuntu 22.04, 2022-08-23)
> when 18.04 support was dropped.
>
> It seems like the absence of coccinelle was a blip in 20.04, and we can
> just stick with "latest" going forward.
>
> I tested the result on GitHub's CI. I bumped the matching line in the
> GitLab definition, but didn't have a simple means of testing (but it's
> such a trivial change nothing could go wrong, right?).
>
> [1] https://lore.kernel.org/git/20260724091152.27794-2-tnyman@openai.com/
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  .github/workflows/main.yml | 4 ++--
>  .gitlab-ci.yml             | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 85cfedf5b0..205325eb33 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -460,8 +460,8 @@ jobs:
>      if: needs.ci-config.outputs.enabled == 'yes'
>      env:
>        jobname: StaticAnalysis
> -      CI_JOB_IMAGE: ubuntu-22.04
> -    runs-on: ubuntu-22.04
> +      CI_JOB_IMAGE: ubuntu-latest
> +    runs-on: ubuntu-latest
>      concurrency:
>        group: static-analysis-${{ github.ref }}
>        cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 1c4d04da9d..0242283c3c 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -227,7 +227,7 @@ test:fuzz-smoke-tests:
>      - ./ci/run-build-and-minimal-fuzzers.sh
>
>  static-analysis:
> -  image: ubuntu:22.04
> +  image: ubuntu:latest
>    stage: analyze
>    needs: [ ]
>    variables:
> --
> 2.55.0.742.gf2bff09aa6

Makes sense to me; looks good.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] ci: bump ubuntu image version for static-analysis job
  2026-07-26  8:39 ` [PATCH 2/2] ci: bump ubuntu image version for static-analysis job Jeff King
  2026-08-07 10:24   ` Patrick Steinhardt
  2026-08-07 16:47   ` Elijah Newren
@ 2026-08-08 17:31   ` SZEDER Gábor
  2 siblings, 0 replies; 8+ messages in thread
From: SZEDER Gábor @ 2026-08-08 17:31 UTC (permalink / raw)
  To: Jeff King; +Cc: git, tnyman, Taylor Blau, Junio C Hamano, Elijah Newren

On Sun, Jul 26, 2026 at 04:39:05AM -0400, Jeff King wrote:
> We recently ran into a case[1] where old versions of coccinelle ran very
> slowly, but newer ones are fine. The version we use in GitHub's CI was
> the old slow version, leading to timeouts of the static-analysis job.
> 
> We get the old version because we ask for the ubuntu-22.04 image. That
> has coccinelle 1.1.1, but the "fast" improvement is in coccinelle 1.3.0,
> specifically their 58619b8fe (break up envs for e1 & e2, 2024-08-18).

I've built Docker images of various Coccinelle versions [1] years ago,
and seeing this issue I've updated those images with more recent base
image and Coccinelle versions.

Using these to run 'make coccicheck' on 630cf86933, i.e. 'seen' on or
around 2026-07-14, which contained a024a5818c (branch: add
--delete-merged <branch>, 2026-07-14) with those problematic loop
counter variables I got the following results:

  - 1.1.1: 1437.78user 56.66system 2:10.29elapsed 1146%CPU (0avgtext+0avgdata 223896maxresident)k

  - 1.2.0: ctrl-c after 2.5h.  The bulk of the work was done in about
           10 minutes, but processing 'builtin/branch.c' seemed to
           hang forever.

  - 1.3.1: 6532.81user 106.75system 9:35.04elapsed 1154%CPU (0avgtext+0avgdata 635592maxresident)k

So my Coccinelle 1.1.1 didn't hang, moreover, it was about 4.5 times
faster than 1.3.1.  I got similar runtime differences between 1.1.1
and 1.3.1 when checking e.g. v2.55.0 or current master; in these cases
1.2.0 didn't hang, but took about the same time as 1.3.1.

Am I doing something wrong?   Or is everyone else is doing something
wrong? :)

[1] https://hub.docker.com/r/szeder/coccinelle/tags


On a somewhat related note, for a while now we've been unnecessarily
installing all the dependencies of the "build and test" jobs
(compiler, build systems, apache, p4, jgit, etc.) for the various
static analysis and the 'documentation' CI jobs as well.

I think this is because 707d2f2fe8 (CI: use "$runs_on_pool", not
"$jobname" to select packages & config, 2021-11-23) started installing
all those dependencies for jobs using 'ubuntu-latest', including the
'documentation' job as well, though this side-effect was not mentioned
in the commit message.  The 'StaticAnalysis' and 'sparse' jobs were
not affected at the time, because they were using a specific Ubuntu
version, but then 0178420b9c (github-actions: run gcc-8 on
ubuntu-20.04 image, 2022-11-25) came along and changed the pattern
matching $runs_on_pool from 'ubuntu-latest' to 'ubuntu-*'.



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-08-08 17:32 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-26  8:32 [PATCH 0/2] bump static-analysis ci image version Jeff King
2026-07-26  8:37 ` [PATCH 1/2] bloom: silence CHECK_ASSERTION_SIDE_EFFECTS false positive Jeff King
2026-07-26  8:39 ` [PATCH 2/2] ci: bump ubuntu image version for static-analysis job Jeff King
2026-08-07 10:24   ` Patrick Steinhardt
2026-08-07 16:16     ` Junio C Hamano
2026-08-07 16:47   ` Elijah Newren
2026-08-08 17:31   ` SZEDER Gábor
2026-07-26 16:34 ` [PATCH 0/2] bump static-analysis ci image version Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox