Re: [PATCH 0/7] Fix all leaks in t0001

From: Andrzej Hunt <andrzej@ahunt.org>
Date: Mar 14, 2021, 16:55 UTC
Message-ID: <a36441d6-4d9d-1cee-9227-272d5332f889@ahunt.org>
In-Reply-To: <xmqqk0qh5v7v.fsf@gitster.c.googlers.com>

On 08/03/2021 19:57, Junio C Hamano wrote:

Show 9 quoted lines

> "Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> hand: fixing or suppressing these leaks should make it easier to spot leaks
>> that have more significant user impact (it's entirely plausible that no real
>> impactful leaks exist).
> 
> Even if there is no leaks that matter exist now, to help maintain
> that state, suppressing false positives would be useful, provided if
> such checkers are run sufficiently often.

Expanding on my thoughts in response to Peff's comments regarding running leak checking under ASAN: I'm wondering whether it would be acceptable to piggyback leak-checking on top of the existing ASAN test runs:

- It looks like people are already running tests with ASAN from time to
   time - enabling leak checking there would add leak-checking coverage
   without having to run LSAN separately (this would have to be
    restricted to those tests that are leak-free - more on that below).
- I also saw some discussion around enabling ASAN in CI, although I
   don't think that went anywhere yet [1]. I'd be interested in trying to
   pick that up - ASAN seems quite valuable by itself, and running it
   in CI would be a simple way to also get leak-checking in CI.
   (Again, this would have to be limited to known leak-free tests.)

(I'm planning to run some benchmarks to see how much enabling leak-checking with ASAN actually costs - I'm assuming it's fairly cheap if you're already running ASAN, but I'd like to verify that.)

As to the mechanics of enabling leak-checking with ASAN: test-lib.sh currently completely disables ASAN's leak-checking. Instead we could add a simple allowlist, to enable leak-checking for those tests that don't - Fix LSAN crashleak - this needn't be much more complex than:

  $TEST_NUMBER <= highest_leak_free_test

I.e. something like this:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index d3f6af6a65..cf9f1ad827 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -41,7 +41,13 @@ GIT_BUILD_DIR="$TEST_DIRECTORY"/..
  # the noise level. This needs to happen at the start of the script,
  # before we even do our "did we build git yet" check (since we don't
  # want that one to complain to stderr).
-: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
+if test $TEST_NUMBER -le 10
+then
+    DETECT_LEAKS=1
+else
+    DETECT_LEAKS=0
+fi
+: ${ASAN_OPTIONS=detect_leaks=$DETECT_LEAKS:abort_on_error=1}
  export ASAN_OPTIONS

  # If LSAN is in effect we _do_ want leak checking, but we still

(This would also require moving TEST_NUMBER a bit earlier in
  test-lib.sh.)

[1] 
https://public-inbox.org/git/20170710155831.3zxijp7bvbquvlau@sigill.intra.peff.net/