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

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] ci: add address and undefined sanitizer tasks
Date: Fri, 21 Oct 2022 02:25:13 -0400	[thread overview]
Message-ID: <Y1I7SeRGikEtgx9t@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqtu3xogxr.fsf@gitster.g>

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