threads / patch / 58224

patchCI: add SANITIZE=[address|undefined] jobs

Subject: [PATCH] CI: add SANITIZE=[address|undefined] jobs

## tl;dr

12 messages between Jul 26, 2022 and Jul 28, 2022. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Ævar Arnfjörð Bjarmason· Jul 26, 2022, 11:09 UTC · lore

Add CI targets for SANITIZE=address and SANITIZE=undefined. The former would have caught a regression in 18bbc795fc5 (Merge branch 'gc/bare-repo-discovery', 2022-07-22) which made its way to "master"[1].

Per [2] the GitHub fork of git.git runs with these in CI, so it's already useful to some forks of this repository.

Also per [2] we could use SANITIZE=address with some ASAN_OPTIONS instead of our SANITIZE=leak job added in 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in CI, 2021-09-23), but unifying those two with these new jobs would be a lot harder, so let's leave that for now.

On my system a "make test" takes around 12m of user time, SANITIZE=address around 44m, and SANITIZE=undefined around 18m.

In practice this doesn't seem to slow down the wallclock time of the the GitHub CI by much This runs in about 50m, but getting through some of the "win build" and associated tests can take around 40m, and the "OSX" tests are on the order of 40m (all of this may vary with the available workers etc.).

The "address" job will fail due to the issue reported in [1]. the "undefined" one succeeds, but note that "t4058-diff-duplicates.sh" triggers its assertions, they're just hidden by "test_expect_failure" (see [3] for why).

1. https://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/YPClS0fj2HOJE5nH@coredump.intra.peff.net/
3. https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This CI wil catch the memory corruption that made it to "master", see: https://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com/

 .github/workflows/main.yml | 6 ++++++
 ci/lib.sh                  | 6 ++++++
 2 files changed, 12 insertions(+)
Show changes to 2 files +12 −0

.github/workflows/main.yml, ci/lib.sh

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index cd1f52692a5..4f59a7aa44c 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -251,6 +251,12 @@ jobs:
           - jobname: linux-leaks
             cc: gcc
             pool: ubuntu-latest
+          - jobname: SANITIZE=address
+            cc: gcc
+            pool: ubuntu-latest
+          - jobname: SANITIZE=undefined
+            cc: gcc
+            pool: ubuntu-latest
     env:
       CC: ${{matrix.vector.cc}}
       CC_PACKAGE: ${{matrix.vector.cc_package}}
diff --git a/ci/lib.sh b/ci/lib.sh
index f095519f8db..5ad60af2e7c 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -277,6 +277,12 @@ linux-leaks)
 	export SANITIZE=leak
 	export GIT_TEST_PASSING_SANITIZE_LEAK=true
 	;;
+SANITIZE=address)
+	export SANITIZE=address
+	;;
+SANITIZE=undefined)
+	export SANITIZE=undefined
+	;;
 esac
 
 MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"

-- 
2.37.1.1107.g76fe5d1ed7c
Derrick Stolee· Jul 26, 2022, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote:
Show 7 quoted lines
> Add CI targets for SANITIZE=address and SANITIZE=undefined. The former
> would have caught a regression in 18bbc795fc5 (Merge branch
> 'gc/bare-repo-discovery', 2022-07-22) which made its way to
> "master"[1].
> 
> Per [2] the GitHub fork of git.git runs with these in CI, so it's
> already useful to some forks of this repository.

I'm a fan of adding additional sanitizer checks in our CI. Let's let computers do the work for us here instead of relying on humans.

Show 14 quoted lines
> Also per [2] we could use SANITIZE=address with some ASAN_OPTIONS
> instead of our SANITIZE=leak job added in 956d2e4639b (tests: add a
> test mode for SANITIZE=leak, run it in CI, 2021-09-23), but unifying
> those two with these new jobs would be a lot harder, so let's leave
> that for now.
>            - jobname: linux-leaks
>              cc: gcc
>              pool: ubuntu-latest
> +          - jobname: SANITIZE=address
> +            cc: gcc
> +            pool: ubuntu-latest
> +          - jobname: SANITIZE=undefined
> +            cc: gcc
> +            pool: ubuntu-latest
Show 10 quoted lines
> @@ -277,6 +277,12 @@ linux-leaks)
>  	export SANITIZE=leak
>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
>  	;;
> +SANITIZE=address)
> +	export SANITIZE=address
> +	;;
> +SANITIZE=undefined)
> +	export SANITIZE=undefined
> +	;;

In both of these cases, we are breaking from the nearby pattern. These jobs could be renamed to linux-address and linux-undefined to match the linux-leaks job.

Alternatively, we could rename linux-leaks to SANITIZE=leak, since the point is not to test the Linux platform but to use the additional runtime checks (and Linux is the fasted CI platform).

Thanks, -Stolee

Ævar Arnfjörð Bjarmason· Jul 26, 2022, 13:33 UTC · re: Derrick Stolee · lore

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

On Tue, Jul 26 2022, Derrick Stolee wrote:
Show 11 quoted lines
> On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote:
>> Add CI targets for SANITIZE=address and SANITIZE=undefined. The former
>> would have caught a regression in 18bbc795fc5 (Merge branch
>> 'gc/bare-repo-discovery', 2022-07-22) which made its way to
>> "master"[1].
>> 
>> Per [2] the GitHub fork of git.git runs with these in CI, so it's
>> already useful to some forks of this repository.
>
> I'm a fan of adding additional sanitizer checks in our CI. Let's let
> computers do the work for us here instead of relying on humans.
Thanks, good to have agreement on adding these CI runs.
Show 31 quoted lines
>> Also per [2] we could use SANITIZE=address with some ASAN_OPTIONS
>> instead of our SANITIZE=leak job added in 956d2e4639b (tests: add a
>> test mode for SANITIZE=leak, run it in CI, 2021-09-23), but unifying
>> those two with these new jobs would be a lot harder, so let's leave
>> that for now.
>>            - jobname: linux-leaks
>>              cc: gcc
>>              pool: ubuntu-latest
>> +          - jobname: SANITIZE=address
>> +            cc: gcc
>> +            pool: ubuntu-latest
>> +          - jobname: SANITIZE=undefined
>> +            cc: gcc
>> +            pool: ubuntu-latest
>
>> @@ -277,6 +277,12 @@ linux-leaks)
>>  	export SANITIZE=leak
>>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
>>  	;;
>> +SANITIZE=address)
>> +	export SANITIZE=address
>> +	;;
>> +SANITIZE=undefined)
>> +	export SANITIZE=undefined
>> +	;;
>
> In both of these cases, we are breaking from the nearby pattern. These
> jobs could be renamed to linux-address and linux-undefined to match the
> linux-leaks job.
>
> Alternatively, we could rename linux-leaks to SANITIZE=leak[...]

I deliberately deviated from the "linux-leaks" pattern since it's a lot more than just:

	make SANITIZE=leak test

I.e. we instrument what tests we run, skip some individual ones etc. These are different in that we can run the entire set. I'd think the reverse would make sense, i.e. one day if we run fully with SANITIZE=leak enabled to rename that job to "SANITIZE=leak".

> [...], since the
> point is not to test the Linux platform but to use the additional runtime
> checks (and Linux is the fasted CI platform).

Strictly speaking these tests are platform-specific in that they require us to take certain codepaths at runtime, so if we have any platform-specific code, or code that's affected by compilation options (say NO_REGEX=Y v.s. using the libc's) we might see failures on one platform, but not another. Compilation flags also matter (e.g. -O0 v.s. -O3).

But I think for all of [leak,address,undefined] it's a sensible trade-off for now to just pick one specific platform.

Since it's very unlikely that the resulting issues are OS-specific I thought it made sense to leave "linux" out of it, just like we have "pedantic", not "linux-pedantic", ditto "sparse" which is also platform-specific around the edges.

Having said all that I really don't care much what we call these as long as we get the test coverage, but I'll hold off on any possible re-roll to see if others chime in about the bikeshed :)

Derrick Stolee· Jul 27, 2022, 11:34 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

On 7/26/22 9:33 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Jul 26 2022, Derrick Stolee wrote:
> 
>> On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote:
Show 36 quoted lines
>>>            - jobname: linux-leaks
>>>              cc: gcc
>>>              pool: ubuntu-latest
>>> +          - jobname: SANITIZE=address
>>> +            cc: gcc
>>> +            pool: ubuntu-latest
>>> +          - jobname: SANITIZE=undefined
>>> +            cc: gcc
>>> +            pool: ubuntu-latest
>>
>>> @@ -277,6 +277,12 @@ linux-leaks)
>>>  	export SANITIZE=leak
>>>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
>>>  	;;
>>> +SANITIZE=address)
>>> +	export SANITIZE=address
>>> +	;;
>>> +SANITIZE=undefined)
>>> +	export SANITIZE=undefined
>>> +	;;
>>
>> In both of these cases, we are breaking from the nearby pattern. These
>> jobs could be renamed to linux-address and linux-undefined to match the
>> linux-leaks job.
>>
>> Alternatively, we could rename linux-leaks to SANITIZE=leak[...]
> 
> I deliberately deviated from the "linux-leaks" pattern since it's a lot
> more than just:
> 
> 	make SANITIZE=leak test
> 
> I.e. we instrument what tests we run, skip some individual ones
> etc. These are different in that we can run the entire set. I'd think
> the reverse would make sense, i.e. one day if we run fully with
> SANITIZE=leak enabled to rename that job to "SANITIZE=leak".

Yes, SANITIZE=leak requires extra steps to make tests pass, but as far as setting up the CI job it is extremely similar to the two you are adding here. Their purpose is also very similar: add compiler flags that would cause our test suite to fail when certain memory issues occur. We just expect the SANITIZE=address and SANITIZE=undefined to pass for every test in the test suite.

That's why I think renaming linux-leaks to SANITIZE=leak is the best option.

Thanks, -Stolee

Junio C Hamano· Jul 27, 2022, 14:30 UTC · re: Derrick Stolee · lore

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

Derrick Stolee <derrickstolee@github.com> writes:
Show 28 quoted lines
>>            - jobname: linux-leaks
>>              cc: gcc
>>              pool: ubuntu-latest
>> +          - jobname: SANITIZE=address
>> +            cc: gcc
>> +            pool: ubuntu-latest
>> +          - jobname: SANITIZE=undefined
>> +            cc: gcc
>> +            pool: ubuntu-latest
>
>> @@ -277,6 +277,12 @@ linux-leaks)
>>  	export SANITIZE=leak
>>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
>>  	;;
>> +SANITIZE=address)
>> +	export SANITIZE=address
>> +	;;
>> +SANITIZE=undefined)
>> +	export SANITIZE=undefined
>> +	;;
>
> In both of these cases, we are breaking from the nearby pattern. These
> jobs could be renamed to linux-address and linux-undefined to match the
> linux-leaks job.
>
> Alternatively, we could rename linux-leaks to SANITIZE=leak, since the
> point is not to test the Linux platform but to use the additional runtime
> checks (and Linux is the fasted CI platform).

I tend to agree that in the existing linux-leaks job, the "linux"-ness is much less important than the "leaks"-ness, so the "alternative" might be slightly more preferable, but I do not mind the renaming goes the other way, either.

Thanks for your good eyes and good taste.
Ævar Arnfjörð Bjarmason· Jul 28, 2022, 16:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

On Wed, Jul 27 2022, Junio C Hamano wrote:
Show 35 quoted lines
> Derrick Stolee <derrickstolee@github.com> writes:
>
>>>            - jobname: linux-leaks
>>>              cc: gcc
>>>              pool: ubuntu-latest
>>> +          - jobname: SANITIZE=address
>>> +            cc: gcc
>>> +            pool: ubuntu-latest
>>> +          - jobname: SANITIZE=undefined
>>> +            cc: gcc
>>> +            pool: ubuntu-latest
>>
>>> @@ -277,6 +277,12 @@ linux-leaks)
>>>  	export SANITIZE=leak
>>>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
>>>  	;;
>>> +SANITIZE=address)
>>> +	export SANITIZE=address
>>> +	;;
>>> +SANITIZE=undefined)
>>> +	export SANITIZE=undefined
>>> +	;;
>>
>> In both of these cases, we are breaking from the nearby pattern. These
>> jobs could be renamed to linux-address and linux-undefined to match the
>> linux-leaks job.
>>
>> Alternatively, we could rename linux-leaks to SANITIZE=leak, since the
>> point is not to test the Linux platform but to use the additional runtime
>> checks (and Linux is the fasted CI platform).
>
> I tend to agree that in the existing linux-leaks job, the
> "linux"-ness is much less important than the "leaks"-ness, so the
> "alternative" might be slightly more preferable, but I do not mind
> the renaming goes the other way, either.

I'm fine with it either way, but to add a prep patch to this series to do s/linux-leaks/SANITIZE=leak/g would cause a semantic conflict with my in-flight https://lore.kernel.org/git/cover-v3-00.15-00000000000-20220727T230800Z-avarab@gmail.com/; It refers to "linux-leaks" in newly added documentation.

So what would you prefer here, to take a lightly altered re-roll of this (I think Jeff's suggestion of bundling "undefined" into one job makes sense), or have it build on top of that otherwise unrelated series, or just do the s/linux-leaks/.../g as an eventual follow-up?

Junio C Hamano· Jul 28, 2022, 17:41 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 5 quoted lines
> I'm fine with it either way, but to add a prep patch to this series to
> do s/linux-leaks/SANITIZE=leak/g would cause a semantic conflict with my
> in-flight
> https://lore.kernel.org/git/cover-v3-00.15-00000000000-20220727T230800Z-avarab@gmail.com/;
> It refers to "linux-leaks" in newly added documentation.

If you mean "in-flight" something that is not in 'next', it may have to be rerolled on top of this new topic.

If something is already in 'next' and we are committed to see it through down to 'master', then this new topic may need to depend on it.

Jeff King· Jul 27, 2022, 19:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

On Tue, Jul 26, 2022 at 01:09:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Per [2] the GitHub fork of git.git runs with these in CI, so it's
> already useful to some forks of this repository.

Yeah, it has been helpful there and I think this is worth doing as part of our CI. It's a lot of CPU versus running the test suite once on Linux, but probably not compared to the overall cost of our current CI.

For the GitHub fork, the code-coverage issues you noticed were easy: we only built one variant, so we could just test with those knobs. ;) But I tend to agree with your approach here to just test on one platform, which covers _most_ of the code. It's certainly better than the status quo, and it strikes a nice balance of CPU versus coverage.

One thing I'd say...
Show 14 quoted lines
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index cd1f52692a5..4f59a7aa44c 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -251,6 +251,12 @@ jobs:
>            - jobname: linux-leaks
>              cc: gcc
>              pool: ubuntu-latest
> +          - jobname: SANITIZE=address
> +            cc: gcc
> +            pool: ubuntu-latest
> +          - jobname: SANITIZE=undefined
> +            cc: gcc
> +            pool: ubuntu-latest

There's really no reason to split the "address" and "undefined" builds into two jobs. We expect them to pass, and if one fails, having the results split is not likely to give any extra information. So I think one job with SANITIZE=address,undefined is fine, and reclaims some of the extra CPU we're spending.

-Peff
Ævar Arnfjörð Bjarmason· Jul 28, 2022, 16:54 UTC · re: Jeff King · lore

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

On Wed, Jul 27 2022, Jeff King wrote:
Show 14 quoted lines
> On Tue, Jul 26, 2022 at 01:09:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Per [2] the GitHub fork of git.git runs with these in CI, so it's
>> already useful to some forks of this repository.
>
> Yeah, it has been helpful there and I think this is worth doing as part
> of our CI. It's a lot of CPU versus running the test suite once on
> Linux, but probably not compared to the overall cost of our current CI.
>
> For the GitHub fork, the code-coverage issues you noticed were easy: we
> only built one variant, so we could just test with those knobs. ;) But I
> tend to agree with your approach here to just test on one platform,
> which covers _most_ of the code. It's certainly better than the status
> quo, and it strikes a nice balance of CPU versus coverage.
*nod*
Show 22 quoted lines
> One thing I'd say...
>
>> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
>> index cd1f52692a5..4f59a7aa44c 100644
>> --- a/.github/workflows/main.yml
>> +++ b/.github/workflows/main.yml
>> @@ -251,6 +251,12 @@ jobs:
>>            - jobname: linux-leaks
>>              cc: gcc
>>              pool: ubuntu-latest
>> +          - jobname: SANITIZE=address
>> +            cc: gcc
>> +            pool: ubuntu-latest
>> +          - jobname: SANITIZE=undefined
>> +            cc: gcc
>> +            pool: ubuntu-latest
>
> There's really no reason to split the "address" and "undefined" builds
> into two jobs. We expect them to pass, and if one fails, having the
> results split is not likely to give any extra information. So I think
> one job with SANITIZE=address,undefined is fine, and reclaims some of
> the extra CPU we're spending.

I'll do that in a re-roll, pending a resolution of the naming discussion at: https://lore.kernel.org/git/220728.86sfmljhyx.gmgdl@evledraar.gmail.com/

But note that it *does* give you extra information to split them up currently, i.e. the "test_expect_failure" that you get with "undefined" isn't conflated with the non-changes that SANITIZE=address flags (sans outstanding recent breakage) in the test report.

But just having that "TODO" test sitting there will suck less than potentially having CI run much longer, or taking up resources from concurrent CI runs, so I'll do this.

We also leave a lot of CI performance on the table by e.g. doing "chain lint" in every single test run (except Windows), there *are* platform edge-cases there like with SANITIZE=address, but I wonder if we should just declare it good enough to do it in 1-2 jobs.

Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about all of those some other time....

Thanks for reviewing this.
Jeff King· Jul 28, 2022, 21:09 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

On Thu, Jul 28, 2022 at 06:54:37PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 9 quoted lines
> > There's really no reason to split the "address" and "undefined" builds
> > into two jobs. We expect them to pass, and if one fails, having the
> > results split is not likely to give any extra information. So I think
> > one job with SANITIZE=address,undefined is fine, and reclaims some of
> > the extra CPU we're spending.
> 
> I'll do that in a re-roll, pending a resolution of the naming discussion
> at:
> https://lore.kernel.org/git/220728.86sfmljhyx.gmgdl@evledraar.gmail.com/
Thanks. FWIW, I have no opinion on the job naming. ;)
Show 8 quoted lines
> But note that it *does* give you extra information to split them up
> currently, i.e. the "test_expect_failure" that you get with "undefined"
> isn't conflated with the non-changes that SANITIZE=address flags (sans
> outstanding recent breakage) in the test report.
> 
> But just having that "TODO" test sitting there will suck less than
> potentially having CI run much longer, or taking up resources from
> concurrent CI runs, so I'll do this.

My hope would be that we'd identify and fix cases where this flagged, even in expect_failure, and so any state like you describe would be temporary. In the long term, we should be able to assume that these don't trigger warnings in the existing code base, and optimize in that direction.

> We also leave a lot of CI performance on the table by e.g. doing "chain
> lint" in every single test run (except Windows), there *are* platform
> edge-cases there like with SANITIZE=address, but I wonder if we should
> just declare it good enough to do it in 1-2 jobs.

I'd be fine with that, but I think chain lint isn't actually that expensive. The original in-shell bits are super cheap. The extra sed process is measurable, but I think I blunted the worst of it in the 2d86a96220 (t: avoid sed-based chain-linting in some expensive cases, 2021-05-13).

Still, that patch should make it easy to time things just by setting GIT_TEST_CHAIN_LINT_HARDER=0 in various jobs. It does seem to buy ~100s of CPU time per test run on my Linux box. That's not a lot in the grand scheme, but perhaps adds up. And I could believe it's much worse on Windows. Maybe worth seeing how it performs in the actual CI environments.

> Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about
> all of those some other time....

I'd be surprised if the malloc checking itself is all that expensive, though it does look like we call getconf and expr once per test there for setup. We could almost certainly hoist that out and call it once per script.

-Peff
Jeff King· Jul 28, 2022, 21:31 UTC · re: Jeff King · lore

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

On Thu, Jul 28, 2022 at 05:09:38PM -0400, Jeff King wrote:
Show 7 quoted lines
> > Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about
> > all of those some other time....
> 
> I'd be surprised if the malloc checking itself is all that expensive,
> though it does look like we call getconf and expr once per test there
> for setup. We could almost certainly hoist that out and call it once per
> script.

Nope, I was totally wrong here. Running with TEST_NO_MALLOC_CHECK=1 does indeed make a big difference:

  Benchmark 1: make test
    Time (mean ± σ):     67.486 s ±  3.339 s    [User: 622.643 s, System: 409.951 s]
    Range (min … max):   63.634 s … 69.550 s    3 runs
   
  Benchmark 2: TEST_NO_MALLOC_CHECK=1 make test
    Time (mean ± σ):     57.596 s ±  0.899 s    [User: 577.273 s, System: 291.627 s]
    Range (min … max):   57.072 s … 58.634 s    3 runs

Eliminating the extra per-test processes with the patch below helps a little, but most of the effort really is (presumably) going to the actual malloc checking code:

  Benchmark 1: make test [nb: after applying patch...]
  Time (mean ± σ):     67.091 s ±  0.385 s    [User: 599.382 s, System: 410.781 s]
  Range (min … max):   66.648 s … 67.343 s    3 runs

Curious that most of the CPU time seems to go into system time. I'd have thought it was extra internal malloc debugging bits, but maybe it is allocating mmap/brk calls in a less efficient way. I dunno how any of it actually works.

-Peff
---
Show changes to t/test-lib.sh +9 −3
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 55857af601..303fbe8ecc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -557,14 +557,20 @@ then
 		: nothing
 	}
 else
+	if _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&
+	   _GLIBC_VERSION=${_GLIBC_VERSION#"glibc "} &&
+	    expr 2.34 \<= "$_GLIBC_VERSION" >/dev/null
+	then
+		USE_LIBC_MALLOC_DEBUG=t
+	else
+		USE_LIBC_MALLOC_DEBUG=
+	fi
 	setup_malloc_check () {
 		local g
 		local t
 		MALLOC_CHECK_=3	MALLOC_PERTURB_=165
 		export MALLOC_CHECK_ MALLOC_PERTURB_
-		if _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&
-		   _GLIBC_VERSION=${_GLIBC_VERSION#"glibc "} &&
-		   expr 2.34 \<= "$_GLIBC_VERSION" >/dev/null
+		if test -n "$USE_LIBC_MALLOC_DEBUG"
 		then
 			g=
 			LD_PRELOAD="libc_malloc_debug.so.0"
Ævar Arnfjörð Bjarmason· Jul 28, 2022, 23:10 UTC · re: Jeff King · lore

Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

On Thu, Jul 28 2022, Jeff King wrote:
Show 27 quoted lines
> On Thu, Jul 28, 2022 at 06:54:37PM +0200, Ævar Arnfjörð Bjarmason wrote:
> [...]
>> We also leave a lot of CI performance on the table by e.g. doing "chain
>> lint" in every single test run (except Windows), there *are* platform
>> edge-cases there like with SANITIZE=address, but I wonder if we should
>> just declare it good enough to do it in 1-2 jobs.
>
> I'd be fine with that, but I think chain lint isn't actually that
> expensive. The original in-shell bits are super cheap. The extra
> sed process is measurable, but I think I blunted the worst of it in the
> 2d86a96220 (t: avoid sed-based chain-linting in some expensive cases,
> 2021-05-13).
>
> Still, that patch should make it easy to time things just by setting
> GIT_TEST_CHAIN_LINT_HARDER=0 in various jobs. It does seem to buy ~100s
> of CPU time per test run on my Linux box. That's not a lot in the grand
> scheme, but perhaps adds up. And I could believe it's much worse on
> Windows. Maybe worth seeing how it performs in the actual CI
> environments.
>
>> Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about
>> all of those some other time....
>
> I'd be surprised if the malloc checking itself is all that expensive,
> though it does look like we call getconf and expr once per test there
> for setup. We could almost certainly hoist that out and call it once per
> script.

I posted some benchmarks for these a while ago: https://lore.kernel.org/git/220405.86k0c3lt2l.gmgdl@evledraar.gmail.com/

YMMV, but these make a big difference for some tests, although less for a whole run, as you point out disabling chain lint least impact.

For me a full test suite run is:
* TEST_NO_MALLOC_CHECK=1 --no-bin-wrappers --no-chain-lint:
	real    1m55.600s
	user    8m48.039s
	sys     3m17.776s
* [no opts, TEST_NO_MALLOC_CHECK=0 & --bin-wrappers && --chain-lint are on]
	real    2m18.126s
	user    10m40.230s
	sys     4m7.152s

So far from the >2x speedup you can get with some tests, but a big difference nonetheless.

← back to recent threads