Re: [PATCH v3 0/8] add a test mode for SANITIZE=leak, run it in CI

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Sep 01 2021, Jeff King wrote:

> On Tue, Aug 31, 2021 at 03:35:34PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>>  * In v2 compiling with SANITIZE=leak would change things so only
>>    known-good passing tests were run by default, everything else would
>>    pass as a dummy. Now the default running of tests is unchanged, but
>>    if we run with GIT_TEST_PASSING_SANITIZE_LEAK=true only those tests
>>    are run which set and export TEST_PASSES_SANITIZE_LEAK=true.
>> 
>>  * The facility for declaring known-good tests in test-lib.sh based on
>>    wildcards is gone, instead individual tests need to declare if
>>    they're OK under SANITIZE=leak.[...]
>
> Hmm. This still seems more complicated than we need. If we just want a
> flag in each script, then test-lib.sh can use that flag to tweak
> LSAN_OPTIONS. See the patch below.

On the "pragma" include v.s. env var + export: I figured this would be
easier to read as I thought the export was required (I don't think it is
in most cases, but e.g. for t0000*.sh I think it is, but that's from
memory...).

> That has two drawbacks:
>
>   - it doesn't have any way to switch the flag per-test. But IMHO it is
>     a mistake to go in that direction. This is all temporary scaffolding
>     while we have leaks, and the script-level of granularity is fine.

We have a lot of tests that do simple checking of the tool itself, and
later in the script might be stressing trace2, or common sources of
leaks like "git log" in combination with the tool (e.g. the commit-graph
tests).

So being able to tweak this inside the script is useful, but that can of
course also be done with this proposed TEST_LSAN_OK + prereq.

>   - it runs the tests not marked as LSAN-OK, just without leak checking,
>     which is redundant in CI where we're already running them. But we
>     could still be collecting leak stats (and just not failing the
>     tests). See the patch below.

Sure, I'd prefer 

>     If we do care about not running them, then I think it makes more
>     sense to extend the run/skip mechanisms and build on that.

The patch I have here is already nicely integrated with the skip
mechanism. I.e. we use skip_all which shows a summary in any TAP
consumer, and we can skip individual tests with prerequisites.

>     (I also think I prefer the central list of "mark these scripts as OK
>     for leak-checking", rather than annotating individuals. Because
>     again, this is temporary, and it's nice to keep it in a sandbox that
>     only people working on leak-checking would look at or touch).
>
> I realize this is kind-of bikeshedding, and I'm not vehemently opposed
> to what you have here. It just seems like fewer moving parts would be
> less likely to confuse folks who want to poke at it.

I can see that for the proposed v2 mechanism, but in this v3 nothing
changes unless you opt-in to things via new GIT_TEST_* setting. So the
chance for confusion seems minimal to nonexisting.

I was interested in doing some summaries of existing leaks
eventually. It seems even with LSAN_OPTIONS=detect_leaks=0 compiling
with SANITIZE=leak make things a bit slower, but not by much (but actual
leak checking is much slower).

But I'd prefer to leave any "write out leak logs and summarize" step for
some later change.

>>    This is done via "export
>>    TEST_PASSES_SANITIZE_LEAK=true", there's a handy import of
>>    "./test-pragma-SANITIZE=leak-ok.sh" before sourcing "./test-lib.sh"
>>    itself to set this.
>
> I found the extra level of indirection added by this pragma confusing.
> We just need to set a variable, which is also a one-liner, and one that
> is more obvious about what it's doing. In your code you also export it,
> but that's not necessary for something that test-lib.sh is going to look
> at. Or if it's really necessary at some point, then test-lib.sh can do
> the export itself.

*nod*, will remove it per discussion above.

>> Ævar Arnfjörð Bjarmason (8):
>>   Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS
>>   CI: refactor "if" to "case" statement
>>   tests: add a test mode for SANITIZE=leak, run it in CI
>>   tests: annotate t000*.sh with TEST_PASSES_SANITIZE_LEAK=true
>>   tests: annotate t001*.sh with TEST_PASSES_SANITIZE_LEAK=true
>>   tests: annotate t002*.sh with TEST_PASSES_SANITIZE_LEAK=true
>>   tests: annotate select t0*.sh with TEST_PASSES_SANITIZE_LEAK=true
>>   tests: annotate select t*.sh with TEST_PASSES_SANITIZE_LEAK=true
>
> Sort of a meta-question, but what's the plan for folks who add a new
> test to say t0000, and it reveals a leak in code they didn't touch?

Then CI will fail on this job. We'd have those same failures now
(e.g. the mentioned current delta between master..seen), we just don't
see them. Having visibility on them seems like an improvement.

> They'll get a CI failure (as will Junio if he picks up the patch), so
> somebody is going to have to deal with it. Do they fix it? Do they unset
> the "this script is OK" flag? Do they mark the individual test as
> non-lsan-ok?

I'd think they'd fix it, or make marking the regression as OK part of
their re-roll, just like failures on master..seen now.

If you're getting at that we should start out this job as an FYI job
that doesn't impact the CI run's overall status if it fails I think that
would be OK as a start.

> I do like the idea of finding real regressions. But while the state of
> leak-checking is still so immature, I'm worried about this adding extra
> friction for developers. Especially if they get some spooky action at a
> distance caused by a leak in far-away code.

Yeah, ultimately this series is an implicit endorsement of us caring
more than we do now.

I think this friction point is going to be mitigated a lot by the
ability I've added to not just skip entire test scripts, but allow
prereq skipping of some tests, early bailing out etc.

It allows you to say add a "git log" test at the end of some test that
otherwise just uses some core API or a test tool and not have to throw
the baby out with the bathwater in terms of disabling all existing leak
checks there to make forward progress (or split up the entire test
script).

> Anyway, here's LSAN_OPTIONS thing I was thinking of.

Thanks, that & your follow-up is very interesting. Can I assume this has
your SOB? I'd like to add that redirect to fd 4 change to this series.

> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index df544bb321..b1da18955d 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -2,6 +2,7 @@
>  
>  test_description='git init'
>  
> +TEST_LSAN_OK=1
>  . ./test-lib.sh
>  
>  check_config () {
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index abcfbed6d6..62627afeaf 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -44,9 +44,30 @@ GIT_BUILD_DIR="$TEST_DIRECTORY"/..
>  : ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
>  export ASAN_OPTIONS
>  
> -# If LSAN is in effect we _do_ want leak checking, but we still
> -# want to abort so that we notice the problems.
> -: ${LSAN_OPTIONS=abort_on_error=1}
> +if test -n "$LSAN_OPTIONS"
> +then
> +	# Leave user-provided options alone.
> +	:
> +elif test -n "$TEST_LSAN_OK"
> +then
> +	# The test script has declared itself as LSAN-clean; turn on full leak
> +	# checking.
> +	LSAN_OPTIONS=abort_on_error=1
> +else
> +	# The test script has possible LSAN failures. Just disable
> +	# leak-checking entirely. Another option would be to log the failures
> +	# with:
> +	#
> +	#   LSAN_OPTIONS=exitcode=0:log_path=$TEST_DIRECTORY/lsan/out
> +	#
> +	# The results are rather confusing, though, as the logs are
> +	# per-process; you have no idea which one came from which test script.
> +	# Ideally we'd send them to descriptor 4 along with the rest of the
> +	# script log, but there's no LSAN_OPTION for that (recent versions of
> +	# libsanitizer do have a public function to do so, so we could hook it
> +	# ourselves via common-main).
> +	LSAN_OPTIONS=detect_leaks=0
> +fi
>  export LSAN_OPTIONS
>  
>  if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS