Re: [PATCH v2 1/4] tests: add a test mode for SANITIZE=leak, run it in CI

From: Jeff King <peff@peff.net>
Date: Jul 15, 2021, 21:14 UTC
Message-ID: <YPClS0fj2HOJE5nH@coredump.intra.peff.net>
In-Reply-To: <eebb4f74-b5e3-6c11-3b84-fcee1b876992@ahunt.org>

On Wed, Jul 14, 2021 at 08:42:27PM +0200, Andrzej Hunt wrote:

Show 9 quoted lines

> My other question is: if we are adding a new job - should it really be just
> a leak checking job? Leak checking is just a subset of ASAN (Address
> Sanitizer). And as discussed at [1] it's possible to run ASAN and UBSAN
> (Undefined Behaviour Sanitizer) in the same build. I feel like it's much
> more useful to first add a combined ASAN+UBSAN job, followed by enabling
> leak-checking as part of ASAN in those jobs for known leak-free tests - as
> opposed to only adding leak checking. We currently disable Leak checking for
> ASAN here [2], but that could be made conditional on the test ID (i.e. check
> an allowlist to enable leak checking for some tests)?

I do think it's worth having an ASan+UBSan job. In the CI we use for our custom fork of Git at GitHub, we run it for every pull request (and I do bring upstream any applicable fixes). It's kind of expensive compared to a regular "make test", but probably not nearly as bad as just running the regular test suite on Windows.

And it's true that ASan can do leak-checking, too. In the long run, when we are leak-free, I think it may make sense to combine the jobs. But in the interim state where we can run the whole suite with ASan/UBSan, but not with LSan, I think it's simpler to just keep them separate. That lets us just entirely skip tests or scripts in the leak-checking run. I haven't measured, but I also expect that LSan is not much more expensive than a regular run, so combining the two isn't that big a win).

So I do like your suggestion, but I think it just be orthogonal further to leak-checking.

-Peff