Re: [PATCH 3/6] test-lib: rely on logs to detect leaks

[Patrick Steinhardt <ps@pks.im>]

On Wed, Jan 01, 2025 at 03:14:44PM -0500, Jeff King wrote:
> When we run with sanitizers, we set abort_on_error=1 so that the tests
> themselves can detect problems directly (when the buggy program exits
> with SIGABRT). This has one blind spot, though: we don't always check
> the exit codes for all programs (e.g., helpers like upload-pack invoked
> behind the scenes).
> 
> For ASan and UBSan this is mostly fine; they exit as soon as they see an
> error, so the unexpected abort of the program causes the test to fail
> anyway.
> 
> But for LSan, the program runs to completion, since we can only check
> for leaks at the end. And in that case we could miss leak reports. And
> thus we started checking LSan logs in faececa53f (test-lib: have the
> "check" mode for SANITIZE=leak consider leak logs, 2022-07-28).
> Originally the logs were optional, but logs are generated (and checked)
> always as of 8c1d6691bc (test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by
> default, 2024-07-11). And we even check them for each test snippet, as
> of cf1464331b (test-lib: check for leak logs after every test,
> 2024-09-24).
> 
> So now aborting on error is superfluous for LSan! We can get everything
> we need by checking the logs. And checking the logs is actually
> preferable, since it gives us more control over silencing false
> positives (something we do not yet do, but will soon).
> 
> So let's tell LSan to just exit normally, even if it finds leaks. We can
> do so with exitcode=0, which also suppresses the abort_on_error flag.

The only downside I can think of is that we now run the whole testcase
to completion before checking for leaks, whereas beforehand we most
likely aborted the testcase on hitting the first leak. It follows that
we may now have multiple leak reports, and it is not immediately clear
which of the commands has actually been failing.

I think we're now in a clean-enough state regarding memory leaks that
this isn't a huge issue anymore though.

Patrick