patchci: add address and undefined sanitizer tasks

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(+)

Junio C Hamano <gitster@pobox.com> writes:

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.

On Mon, Oct 10, 2022 at 04:25:23PM -0700, Junio C Hamano wrote:

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

Jeff King <peff@peff.net> writes:

Ah, that may be true.  I'll try that in the next version.

Jeff King <peff@peff.net> writes:

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(+)

On Thu, Oct 20, 2022 at 10:59:28PM -0700, Junio C Hamano wrote:

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

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(+)

On Mon, Oct 10, 2022 at 05:21:38PM -0700, Junio C Hamano wrote:

Looks fine to me (and I obviously endorse the goal :) ).

-Peff

On Mon, Oct 10 2022, Junio C Hamano wrote:

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