[PATCH] ci: add address and undefined sanitizer tasks

[PATCH] ci: add address and undefined sanitizer tasks

[Junio C Hamano]

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

Re: [PATCH] ci: add address and undefined sanitizer tasks

[Junio C Hamano]

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

Re: [PATCH] ci: add address and undefined sanitizer tasks

[Jeff King]

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

Re: [PATCH] ci: add address and undefined sanitizer tasks

[Junio C Hamano]

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.

[PATCH v2] ci: add address and undefined sanitizer tasks

[Junio C Hamano]

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

Re: [PATCH v2] ci: add address and undefined sanitizer tasks

[Jeff King]

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

Re: [PATCH v2] ci: add address and undefined sanitizer tasks

[Ævar Arnfjörð Bjarmason]

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

Re: [PATCH] ci: add address and undefined sanitizer tasks

[Junio C Hamano]

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

Re: [PATCH] ci: add address and undefined sanitizer tasks

[Jeff King]

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