Re: [PATCH v2 1/4] tests: add a test mode for SANITIZE=leak, run it in CI

> On 14/07/2021 19:23, Ævar Arnfjörð Bjarmason wrote:
>> While git can be compiled with SANITIZE=leak there has been no
>> corresponding GIT_TEST_* mode for it, i.e. memory leaks have been
>> fixed as one-offs without structured regression testing.
>> This change add such a mode, we now have new
>> linux-{clang,gcc}-sanitize-leak CI targets, these targets run the same
>> tests as linux-{clang,gcc}, except that almost all of them are
>> skipped.
>> There is a whitelist of some tests that are OK in test-lib.sh, and
>> individual tests can be opted-in by setting
>> GIT_TEST_SANITIZE_LEAK=true before sourcing test-lib.sh. Within those
>> individual test can be skipped with the "!SANITIZE_LEAK"
>> prerequisite. See the updated t/README for more details.
>> I'm using the GIT_TEST_SANITIZE_LEAK=true and !SANITIZE_LEAK pattern
>> in a couple of tests whose memory leaks I'll fix in subsequent
>> commits.
>> I'm not being aggressive about opting in tests, it's not all tests
>> that currently pass under SANITIZE=leak, just a small number of
>> known-good tests. We can add more later as we fix leaks and grow more
>> confident in this test mode.
>> See the recent discussion at [1] about the lack of this sort of test
>> mode, and 0e5bba53af (add UNLEAK annotation for reducing leak false
>> positives, 2017-09-08) for the initial addition of SANITIZE=leak.
>> See also 09595ab381 (Merge branch 'jk/leak-checkers', 2017-09-19),
>> 7782066f67 (Merge branch 'jk/apache-lsan', 2019-05-19) and the recent
>> 936e58851a (Merge branch 'ah/plugleaks', 2021-05-07) for some of the
>> past history of "one-off" SANITIZE=leak (and more) fixes.
>> When calling maybe_skip_all_sanitize_leak matching against
>> "$TEST_NAME" instead of "$this_test" as other "match_pattern_list()"
>> users do is intentional. I'd like to match things like "t13*config*"
>> in subsequent commits. This part of the API isn't public, so we can
>> freely change it in the future.
>> 1. https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   .github/workflows/main.yml |  6 ++++
>>   Makefile                   |  5 ++++
>>   ci/install-dependencies.sh |  4 +--
>>   ci/lib.sh                  | 18 ++++++++----
>>   ci/run-build-and-tests.sh  |  4 +--
>>   t/README                   | 16 ++++++++++
>>   t/t5701-git-serve.sh       |  2 +-
>>   t/test-lib.sh              | 60 ++++++++++++++++++++++++++++++++++++++
>>   8 files changed, 105 insertions(+), 10 deletions(-)
>> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
>> index 73856bafc9..752fe187f9 100644
>> --- a/.github/workflows/main.yml
>> +++ b/.github/workflows/main.yml
>> @@ -297,6 +297,12 @@ jobs:
>>             - jobname: linux-gcc-default
>>               cc: gcc
>>               pool: ubuntu-latest
>> +          - jobname: linux-clang-sanitize-leak
>> +            cc: clang
>> +            pool: ubuntu-latest
>> +          - jobname: linux-gcc-sanitize-leak
>> +            cc: gcc
>> +            pool: ubuntu-latest
>
> Is there any advantage to running leak checking with both gcc and
> clang? My understanding is that you end up using the same sanitiser 
> implementation under the hood - I can't remember if using a different
> compiler actually helps find different leaks though.

> My other question is: if we are adding a new job - should it really be
> just a leak checking job? Leak checking is just a subset of ASAN 
> (Address Sanitizer). And as discussed at [1] it's possible to run ASAN
> and UBSAN (Undefined Behaviour Sanitizer) in the same build. I feel
> like it's much more useful to first add a combined ASAN+UBSAN job,
> followed by enabling leak-checking as part of ASAN in those jobs for
> known leak-free tests - as opposed to only adding leak checking. We
> currently disable Leak checking for ASAN here [2], but that could be
> made conditional on the test ID (i.e. check an allowlist to enable
> leak checking for some tests)?

> [1]
> https://lore.kernel.org/git/YMI%2Fg1sHxJgb8%2FYD@coredump.intra.peff.net/
>
> [2] https://git.kernel.org/pub/scm/git/git.git/tree/t/test-lib.sh#n44
>
>>       env:
>>         CC: ${{matrix.vector.cc}}
>>         jobname: ${{matrix.vector.jobname}}
>> diff --git a/Makefile b/Makefile
>> index 502e0c9a81..d4cad5136f 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1216,6 +1216,9 @@ PTHREAD_CFLAGS =
>>   SPARSE_FLAGS ?=
>>   SP_EXTRA_FLAGS = -Wno-universal-initializer
>>   +# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak target
>> +SANITIZE_LEAK =
>> +
>>   # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
>>   # usually result in less CPU usage at the cost of higher peak memory.
>>   # Setting it to 0 will feed all files in a single spatch invocation.
>> @@ -1260,6 +1263,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
>>   endif
>>   ifneq ($(filter leak,$(SANITIZERS)),)
>>   BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS
>> +SANITIZE_LEAK = YesCompiledWithIt >   endif
>>   ifneq ($(filter address,$(SANITIZERS)),)
>>   NO_REGEX = NeededForASAN
>> @@ -2793,6 +2797,7 @@ GIT-BUILD-OPTIONS: FORCE
>>   	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
>>   	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
>>   	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
>> +	@echo SANITIZE_LEAK=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_LEAK)))'\' >>$@+
>>   	@echo X=\'$(X)\' >>$@+
>>   ifdef TEST_OUTPUT_DIRECTORY
>>   	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
>> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
>> index 67852d0d37..8ac72d7246 100755
>> --- a/ci/install-dependencies.sh
>> +++ b/ci/install-dependencies.sh
>> @@ -12,13 +12,13 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
>>    libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
>>     case "$jobname" in
>> -linux-clang|linux-gcc)
>> +linux-clang|linux-gcc|linux-clang-sanitize-leak|linux-gcc-sanitize-leak)
>
> How about `linux-clang*|linux-gcc*)` here and below?

>>   	sudo apt-add-repository -y "ppa:ubuntu-toolchain-r/test"
>>   	sudo apt-get -q update
>>   	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
>>   		$UBUNTU_COMMON_PKGS
>>   	case "$jobname" in
>> -	linux-gcc)
>> +	linux-gcc|linux-gcc-sanitize-leak)
>>   		sudo apt-get -q -y install gcc-8
>>   		;;
>>   	esac
>> diff --git a/ci/lib.sh b/ci/lib.sh
>> index 476c3f369f..bb02b5abf4 100755
>> --- a/ci/lib.sh
>> +++ b/ci/lib.sh
>> @@ -183,14 +183,16 @@ export GIT_TEST_CLONE_2GB=true
>>   export SKIP_DASHED_BUILT_INS=YesPlease
>>     case "$jobname" in
>> -linux-clang|linux-gcc)
>> -	if [ "$jobname" = linux-gcc ]
>> -	then
>> +linux-clang|linux-gcc|linux-clang-sanitize-leak|linux-gcc-sanitize-leak)
>> +	case "$jobname" in
>> +	linux-gcc|linux-gcc-sanitize-leak)
>>   		export CC=gcc-8
>>   		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3"
>> -	else
>> +		;;
>> +	*)
>>   		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python2"
>> -	fi
>> +		;;
>> +	esac
>>     	export GIT_TEST_HTTPD=true
>>   @@ -233,4 +235,10 @@ linux-musl)
>>   	;;
>>   esac
>>   +case "$jobname" in
>> +linux-clang-sanitize-leak|linux-gcc-sanitize-leak)
>> +	export SANITIZE=leak
>> +	;;
>> +esac
>> +
>
> Have you considered doing this in the yaml job configuration instead?
> It's possible to set env-vars in yaml, although it will require some 
> careful tweaking - here's an example where I'm setting different
> values for SANITIZE depending on job (you'd probably just have to set
> it to empty for the non leak-checking jobs):
>
> https://github.com/ahunt/git/blob/master/.github/workflows/ahunt-sync-next2.yml#L51-L69
>
> That does make the yaml more complex, but I think it's worth it to
> reduce the amount of special-casing elsewhere (and is also worth it if 
> we ever add other sanitisers)?