Re: UNLEAK(), leak checking in the default tests etc.

From: Andrzej Hunt <andrzej@ahunt.org>
Date: Jun 10, 2021, 15:32 UTC
Message-ID: <bd212451-d8f0-e041-3460-bbbff57542d8@ahunt.org>
In-Reply-To: <YMIVzYgNddsR4FSd@coredump.intra.peff.net>

On 10/06/2021 15:38, Jeff King wrote:

Show 22 quoted lines

>> I ran it, noted the failing tests, produced a giant GIT_SKIP_TESTS list
>> and hacked ci/ to run that as a new linux-clang-SANITIZE job. That messy
>> WIP code is currently running at:
>> https://github.com/avar/git/runs/2793150092
>>
>> Wouldn't it be a good idea to have such a job and slowly work on the
>> exclusion list?
>>
>> E.g. I saw that t0004 failed, which was trivially fixed with a single
>> strbuf_release(), and we could guard against regressions.
> 
> I don't mind that. My intent was to get the whole suite clean
> eventually, and then start worrying about regressions. But that may take
> a while.
> 
> I do think it would be worth splitting out ASan from leak-checking. The
> whole suite should run clean with regular ASan already, and we'd want to
> find regressions there even in the tests that aren't leak-clean. I do
> periodic ASan runs already; the main argument against doing it for every
> CI run is just that's a lot more CPU. But maybe not enough to be
> prohibitive? It's probably still way cheaper than running the test suite
> on Windows.

I've been running tests with ASAN in the Github Actions environment, and a single run takes just over 30 minutes [1] - which I believe is similar to the normal test jobs (they do run the test suite twice in that time I think).

I've been doing the same with UBSAN, and that's even faster at 15-20 minutes [2]. However I get the impression that ASAN issues are both more common (at least on seen), and more impactful - so I would argue that ASAN should be prioritised if there's spare capacity. (I have no idea if ASAN+UBSAN can be combined, but I suspect that doing so would make the tests slower?)

I'm also running LSAN tests in CI to try and catch regressions, but I've only enabled a handful of tests so far. My much simpler approach was to specify the range of tests to run as 0-X, and as we make progress on fixing leaks, X will slowly approach 9999 (currently we're at something like X~=5, although I'm not too far off sending out some patches to boost that to 99). The skip-tests approach seems much more useful!

ATB,

   Andrzej

[1] https://github.com/ahunt/git/runs/2789921851?check_suite_focus=true [2] https://github.com/ahunt/git/runs/2760632000?check_suite_focus=true