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

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

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