On Fri, Jan 03, 2025 at 01:05:41PM +0100, Patrick Steinhardt wrote:

quoted

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.

True, I didn't think of that. We do at least check the logs after each
case, so it would have to be multiple leaks in the same snippet. The
LSan output also mentions the process name, though not the arguments
(and some snippets may invoke the same program multiple times).

The other thing I guess we'd miss is that SIGABRT can optionally produce
a core dump. I don't think I've ever found that useful for LSan (since
it's not exiting until the end of process) but occasionally have used it
for ASan (though this patch does not change anything there).

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

Yeah, agreed. We can see how much of a problem it is in practice. You
can always switch the flag yourself for a specific run, like:

  LSAN_OPTIONS=exitcode=23 ./twhatever -i

-Peff