* [PATCH] ci: add address and undefined sanitizer tasks
@ 2022-10-09 22:44 Junio C Hamano
2022-10-10 23:25 ` Junio C Hamano
2022-10-11 0:21 ` [PATCH v2] " Junio C Hamano
0 siblings, 2 replies; 9+ messages in thread
From: Junio C Hamano @ 2022-10-09 22:44 UTC (permalink / raw)
To: git
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* I've been running my local post-integration-pre-pushout tests of
'seen' with these two sanitizer tests, which has saved me from a
few potential embarrassments early. As it takes a lot extra time
to run these locally, I am aiming to burden contributors who run
their due diligence "before sending the patch" checks using the
GitHub Actions CI ;-).
The way the patch adds jobs to CI just imitates how -leaks one is
defined.
.github/workflows/main.yml | 6 ++++++
ci/lib.sh | 3 +++
2 files changed, 9 insertions(+)
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 831f4df56c..2f80da7cfb 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: linux-address
+ cc: gcc
+ pool: ubuntu-latest
+ - jobname: linux-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 1b0cc2b57d..678edd5abb 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -278,6 +278,9 @@ linux-leaks)
export GIT_TEST_PASSING_SANITIZE_LEAK=true
export GIT_TEST_SANITIZE_LEAK_LOG=true
;;
+linux-address | linux-undefined)
+ export SANITIZE=${jobname#linux-}
+ ;;
esac
MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
--
2.38.0-167-gf9a88ca9e9
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ci: add address and undefined sanitizer tasks
2022-10-09 22:44 [PATCH] ci: add address and undefined sanitizer tasks Junio C Hamano
@ 2022-10-10 23:25 ` Junio C Hamano
2022-10-11 0:00 ` Jeff King
2022-10-11 0:21 ` [PATCH v2] " Junio C Hamano
1 sibling, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2022-10-10 23:25 UTC (permalink / raw)
To: git
Junio C Hamano <gitster@pobox.com> writes:
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> * I've been running my local post-integration-pre-pushout tests of
> 'seen' with these two sanitizer tests, which has saved me from a
> few potential embarrassments early. As it takes a lot extra time
> to run these locally, I am aiming to burden contributors who run
> their due diligence "before sending the patch" checks using the
> GitHub Actions CI ;-).
>
> The way the patch adds jobs to CI just imitates how -leaks one is
> defined.
>
> .github/workflows/main.yml | 6 ++++++
> ci/lib.sh | 3 +++
> 2 files changed, 9 insertions(+)
One downside is that the usual CI cycle for a branch that takes a
bit shorter than 40 minutes seems to take between 50 to 60 minutes
(the primary culprit seems to be the address sanitizer).
Arguably, that 10 or 20 minutes are time saved from human
developers' time, so it might not be too bad, but I'll keep
it out of 'next' for now.
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 831f4df56c..2f80da7cfb 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: linux-address
> + cc: gcc
> + pool: ubuntu-latest
> + - jobname: linux-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 1b0cc2b57d..678edd5abb 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -278,6 +278,9 @@ linux-leaks)
> export GIT_TEST_PASSING_SANITIZE_LEAK=true
> export GIT_TEST_SANITIZE_LEAK_LOG=true
> ;;
> +linux-address | linux-undefined)
> + export SANITIZE=${jobname#linux-}
> + ;;
> esac
>
> MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ci: add address and undefined sanitizer tasks
2022-10-10 23:25 ` Junio C Hamano
@ 2022-10-11 0:00 ` Jeff King
2022-10-11 0:09 ` Junio C Hamano
2022-10-21 5:59 ` Junio C Hamano
0 siblings, 2 replies; 9+ messages in thread
From: Jeff King @ 2022-10-11 0:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Mon, Oct 10, 2022 at 04:25:23PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > ---
> > * I've been running my local post-integration-pre-pushout tests of
> > 'seen' with these two sanitizer tests, which has saved me from a
> > few potential embarrassments early. As it takes a lot extra time
> > to run these locally, I am aiming to burden contributors who run
> > their due diligence "before sending the patch" checks using the
> > GitHub Actions CI ;-).
> >
> > The way the patch adds jobs to CI just imitates how -leaks one is
> > defined.
>
> One downside is that the usual CI cycle for a branch that takes a
> bit shorter than 40 minutes seems to take between 50 to 60 minutes
> (the primary culprit seems to be the address sanitizer).
>
> Arguably, that 10 or 20 minutes are time saved from human
> developers' time, so it might not be too bad, but I'll keep
> it out of 'next' for now.
You can make it a bit faster by running both at once as
SANITIZE=address,undefined.
Since we expect both to pass cleanly, there's really no other
complication; the user will either see errors (and correct them) or they
won't. The signal of "passed with asan, but not ubsan" (or vice versa)
is not that useful in practice.
In the long run, I hope that "leaks" can run in the same way, but we're
not there yet since there's a lot of selective test-running. In theory
the regular linux test run is not necessary with this job, but I think
the signal for "broke with/without sanitizers" is a little higher (and
anyway, we have to run the regular suite on a zillion other platforms,
too).
-Peff
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ci: add address and undefined sanitizer tasks
2022-10-11 0:00 ` Jeff King
@ 2022-10-11 0:09 ` Junio C Hamano
2022-10-21 5:59 ` Junio C Hamano
1 sibling, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2022-10-11 0:09 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> The signal of "passed with asan, but not ubsan" (or vice versa)
> is not that useful in practice.
Ah, that may be true. I'll try that in the next version.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] ci: add address and undefined sanitizer tasks
2022-10-09 22:44 [PATCH] ci: add address and undefined sanitizer tasks Junio C Hamano
2022-10-10 23:25 ` Junio C Hamano
@ 2022-10-11 0:21 ` Junio C Hamano
2022-10-11 0:28 ` Jeff King
2022-10-11 8:23 ` Ævar Arnfjörð Bjarmason
1 sibling, 2 replies; 9+ messages in thread
From: Junio C Hamano @ 2022-10-11 0:21 UTC (permalink / raw)
To: git; +Cc: Jeff King
The signal of "passed with asan, but not ubsan" (or vice versa) is
not that useful in practice, run both santizers in a single task.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Range-diff against v1:
1: 04a9dc5439 ! 1: cbf0d80ab1 ci: add address and undefined sanitizer tasks
@@ Metadata
## Commit message ##
ci: add address and undefined sanitizer tasks
+ The signal of "passed with asan, but not ubsan" (or vice versa) is
+ not that useful in practice, run both santizers in a single task.
+
+ Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
## .github/workflows/main.yml ##
@@ .github/workflows/main.yml: jobs:
- jobname: linux-leaks
cc: gcc
pool: ubuntu-latest
-+ - jobname: linux-address
-+ cc: gcc
-+ pool: ubuntu-latest
-+ - jobname: linux-undefined
++ - jobname: linux-sanitize
+ cc: gcc
+ pool: ubuntu-latest
env:
@@ ci/lib.sh: linux-leaks)
export GIT_TEST_PASSING_SANITIZE_LEAK=true
export GIT_TEST_SANITIZE_LEAK_LOG=true
;;
-+linux-address | linux-undefined)
-+ export SANITIZE=${jobname#linux-}
++linux-sanitize)
++ export SANITIZE=address,undefined
+ ;;
esac
.github/workflows/main.yml | 3 +++
ci/lib.sh | 3 +++
2 files changed, 6 insertions(+)
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 831f4df56c..92d27db0b9 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -251,6 +251,9 @@ jobs:
- jobname: linux-leaks
cc: gcc
pool: ubuntu-latest
+ - jobname: linux-sanitize
+ 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 1b0cc2b57d..c9c4982e21 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -278,6 +278,9 @@ linux-leaks)
export GIT_TEST_PASSING_SANITIZE_LEAK=true
export GIT_TEST_SANITIZE_LEAK_LOG=true
;;
+linux-sanitize)
+ export SANITIZE=address,undefined
+ ;;
esac
MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
--
2.38.0-146-gaff07b31d7
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] ci: add address and undefined sanitizer tasks
2022-10-11 0:21 ` [PATCH v2] " Junio C Hamano
@ 2022-10-11 0:28 ` Jeff King
2022-10-11 8:23 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 9+ messages in thread
From: Jeff King @ 2022-10-11 0:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Mon, Oct 10, 2022 at 05:21:38PM -0700, Junio C Hamano wrote:
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 831f4df56c..92d27db0b9 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -251,6 +251,9 @@ jobs:
> - jobname: linux-leaks
> cc: gcc
> pool: ubuntu-latest
> + - jobname: linux-sanitize
> + 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 1b0cc2b57d..c9c4982e21 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -278,6 +278,9 @@ linux-leaks)
> export GIT_TEST_PASSING_SANITIZE_LEAK=true
> export GIT_TEST_SANITIZE_LEAK_LOG=true
> ;;
> +linux-sanitize)
> + export SANITIZE=address,undefined
> + ;;
Looks fine to me (and I obviously endorse the goal :) ).
-Peff
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] ci: add address and undefined sanitizer tasks
2022-10-11 0:21 ` [PATCH v2] " Junio C Hamano
2022-10-11 0:28 ` Jeff King
@ 2022-10-11 8:23 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 9+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-10-11 8:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
On Mon, Oct 10 2022, Junio C Hamano wrote:
> The signal of "passed with asan, but not ubsan" (or vice versa) is
> not that useful in practice, run both santizers in a single task.
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Range-diff against v1:
> 1: 04a9dc5439 ! 1: cbf0d80ab1 ci: add address and undefined sanitizer tasks
> @@ Metadata
> ## Commit message ##
> ci: add address and undefined sanitizer tasks
>
> + The signal of "passed with asan, but not ubsan" (or vice versa) is
> + not that useful in practice, run both santizers in a single task.
> +
> + Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> ## .github/workflows/main.yml ##
> @@ .github/workflows/main.yml: jobs:
> - jobname: linux-leaks
> cc: gcc
> pool: ubuntu-latest
> -+ - jobname: linux-address
> -+ cc: gcc
> -+ pool: ubuntu-latest
> -+ - jobname: linux-undefined
> ++ - jobname: linux-sanitize
> + cc: gcc
> + pool: ubuntu-latest
> env:
> @@ ci/lib.sh: linux-leaks)
> export GIT_TEST_PASSING_SANITIZE_LEAK=true
> export GIT_TEST_SANITIZE_LEAK_LOG=true
> ;;
> -+linux-address | linux-undefined)
> -+ export SANITIZE=${jobname#linux-}
> ++linux-sanitize)
> ++ export SANITIZE=address,undefined
> + ;;
> esac
>
>
> .github/workflows/main.yml | 3 +++
> ci/lib.sh | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 831f4df56c..92d27db0b9 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -251,6 +251,9 @@ jobs:
> - jobname: linux-leaks
> cc: gcc
> pool: ubuntu-latest
> + - jobname: linux-sanitize
> + 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 1b0cc2b57d..c9c4982e21 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -278,6 +278,9 @@ linux-leaks)
> export GIT_TEST_PASSING_SANITIZE_LEAK=true
> export GIT_TEST_SANITIZE_LEAK_LOG=true
> ;;
> +linux-sanitize)
> + export SANITIZE=address,undefined
> + ;;
> esac
>
> MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
This LGTM, and all looks a bit familiar, even down to Jeff having the
same feedback on the v1:
https://lore.kernel.org/git/patch-1.1-e48b6853dd5-20220726T110716Z-avarab@gmail.com/
:)
FWIW I had a re-roll ready for that, but was waiting for the dust to
settle on failures. As soon as we were SANITIZE=address clean (which 2x
commits broke in quick succession) we had the scalar SANITIZE=undefined
failure, and then the release.
But this works too. It's the same as what I've had queued up locally,
except the job name is different, which was in response to other
feedback in the original thread:
https://github.com/git/git/compare/master...avar:git:avar/ci-add-SANITIZE%3Daddress-and-SANITIZE%3Dundefined-2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ci: add address and undefined sanitizer tasks
2022-10-11 0:00 ` Jeff King
2022-10-11 0:09 ` Junio C Hamano
@ 2022-10-21 5:59 ` Junio C Hamano
2022-10-21 6:25 ` Jeff King
1 sibling, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2022-10-21 5:59 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> You can make it a bit faster by running both at once as
> SANITIZE=address,undefined.
With a single combined sanitier job, we saw failures at CI of p4
tests, where the server "goes away" in the middle of tests, for
apparently no reason, and I just speculated perhaps the tests taking
too long may be causing an impatient server side to go away before
the client side finishes its work or something. After splitting the
ASan and UBSan jobs into two and ejecting a topic out of 'seen', CI
started passing for the first time in several days.
So, here is what I ended up using, at least for now.
----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] ci: add address and undefined sanitizer tasks
The current code is clean with these two sanitizers, and we would
like to keep it that way by running the checks for any new code.
The signal of "passed with asan, but not ubsan" (or vice versa) is
not that useful in practice, so it is tempting to run both santizers
in a single task, but it seems to take forever, so tentatively let's
try having two separate ones.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
.github/workflows/main.yml | 6 ++++++
ci/lib.sh | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 831f4df56c..bd6f75b8e0 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: linux-asan
+ cc: gcc
+ pool: ubuntu-latest
+ - jobname: linux-ubsan
+ 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 1b0cc2b57d..e3d49d3296 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -278,6 +278,12 @@ linux-leaks)
export GIT_TEST_PASSING_SANITIZE_LEAK=true
export GIT_TEST_SANITIZE_LEAK_LOG=true
;;
+linux-asan)
+ export SANITIZE=address
+ ;;
+linux-ubsan)
+ export SANITIZE=undefined
+ ;;
esac
MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
--
2.38.1-312-ge3e2987aab
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ci: add address and undefined sanitizer tasks
2022-10-21 5:59 ` Junio C Hamano
@ 2022-10-21 6:25 ` Jeff King
0 siblings, 0 replies; 9+ messages in thread
From: Jeff King @ 2022-10-21 6:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, Oct 20, 2022 at 10:59:28PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > You can make it a bit faster by running both at once as
> > SANITIZE=address,undefined.
>
> With a single combined sanitier job, we saw failures at CI of p4
> tests, where the server "goes away" in the middle of tests, for
> apparently no reason, and I just speculated perhaps the tests taking
> too long may be causing an impatient server side to go away before
> the client side finishes its work or something. After splitting the
> ASan and UBSan jobs into two and ejecting a topic out of 'seen', CI
> started passing for the first time in several days.
I am not too surprised to hear that. We flushed out many test races in
the past from using valgrind and SANITIZE. But I don't run the p4 tests
locally, so this is likely some of their first exposure to slower runs.
But if that is the case, then you are probably just papering over
failures here which will likely come back, at least racily.
I admit I was surprised how much slower the combined one is. Here are a
few timings on my laptop:
$ for i in '' address undefined address,undefined; do
echo -n "SANITIZE=$i"
make SANITIZE=$i >/dev/null 2>&1 &&
(cd t && time ./t3700-add.sh >/dev/null 2>&1)
echo
done
SANITIZE=
real 0m1.109s
user 0m0.533s
sys 0m0.589s
SANITIZE=address
real 0m1.816s
user 0m0.999s
sys 0m0.806s
SANITIZE=undefined
real 0m1.336s
user 0m0.618s
sys 0m0.710s
SANITIZE=address,undefined
real 0m2.928s
user 0m1.304s
sys 0m1.635s
Curiously, with CC=clang the results are closer to what I'd expect:
SANITIZE=
real 0m1.186s
user 0m0.608s
sys 0m0.579s
SANITIZE=address
real 0m1.910s
user 0m1.014s
sys 0m0.890s
SANITIZE=undefined
real 0m1.477s
user 0m0.611s
sys 0m0.865s
SANITIZE=address,undefined
real 0m2.309s
user 0m1.126s
sys 0m1.191s
I'm not sure what the takeaway is. If using two jobs produces
appreciably fewer races in practice, it may be worth doing. But I wonder
if just using clang would work better.
-Peff
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-10-21 6:25 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-09 22:44 [PATCH] ci: add address and undefined sanitizer tasks Junio C Hamano
2022-10-10 23:25 ` Junio C Hamano
2022-10-11 0:00 ` Jeff King
2022-10-11 0:09 ` Junio C Hamano
2022-10-21 5:59 ` Junio C Hamano
2022-10-21 6:25 ` Jeff King
2022-10-11 0:21 ` [PATCH v2] " Junio C Hamano
2022-10-11 0:28 ` Jeff King
2022-10-11 8:23 ` Ævar Arnfjörð Bjarmason
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).