| Message ID | patch-1.4-0795436a24-20210714T172251Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add a test mode for SANITIZE=leak, run it in CI | expand |
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)? I think it's worth focusing on ASAN+UBSAN first because they tend to find more impactful issues (e.g. buffer overflows, and other real bugs) - whereas leaks... are ugly, but leaks in git don't actually have much user impact? [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)? > MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}" > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh > index 3ce81ffee9..5fe047b5c6 100755 > --- a/ci/run-build-and-tests.sh > +++ b/ci/run-build-and-tests.sh > @@ -12,7 +12,7 @@ esac > > make > case "$jobname" in > -linux-gcc) > +linux-gcc|linux-gcc-sanitize-leak) > export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > make test > export GIT_TEST_SPLIT_INDEX=yes > @@ -29,7 +29,7 @@ linux-gcc) > export GIT_TEST_CHECKOUT_WORKERS=2 > make test > ;; > -linux-clang) > +linux-clang|linux-clang-sanitize-leak) > export GIT_TEST_DEFAULT_HASH=sha1 > make test > export GIT_TEST_DEFAULT_HASH=sha256 > diff --git a/t/README b/t/README > index 1a2072b2c8..303d0be817 100644 > --- a/t/README > +++ b/t/README > @@ -448,6 +448,22 @@ GIT_TEST_CHECKOUT_WORKERS=<n> overrides the 'checkout.workers' setting > to <n> and 'checkout.thresholdForParallelism' to 0, forcing the > execution of the parallel-checkout code. > > +GIT_TEST_SANITIZE_LEAK=<boolean> will force the tests to run when git > +is compiled with SANITIZE=leak (we pick it up via > +../GIT-BUILD-OPTIONS). > + > +By default all tests are skipped when compiled with SANITIZE=leak, and > +individual test scripts opt themselves in to leak testing by setting > +GIT_TEST_SANITIZE_LEAK=true before sourcing test-lib.sh. Within those > +tests use the SANITIZE_LEAK prerequisite to skip individiual tests > +(i.e. test_expect_success !SANITIZE_LEAK [...]). > + > +So the GIT_TEST_SANITIZE_LEAK setting is different in behavior from > +both other GIT_TEST_*=[true|false] settings, but more useful given how > +SANITIZE=leak works & the state of the test suite. Manually setting > +GIT_TEST_SANITIZE_LEAK=true is only useful during development when > +finding and fixing memory leaks. > + > Naming Tests > ------------ > > diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh > index 930721f053..d58efb0aa9 100755 > --- a/t/t5701-git-serve.sh > +++ b/t/t5701-git-serve.sh > @@ -243,7 +243,7 @@ test_expect_success 'unexpected lines are not allowed in fetch request' ' > > # Test the basics of object-info > # > -test_expect_success 'basics of object-info' ' > +test_expect_success !SANITIZE_LEAK 'basics of object-info' ' > test-tool pkt-line pack >in <<-EOF && > command=object-info > object-format=$(test_oid algo) > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 7036f83b33..9201510e16 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -1353,6 +1353,40 @@ then > exit 1 > fi > > +# SANITIZE=leak test mode > +sanitize_leak_true= > +add_sanitize_leak_true () { > + sanitize_leak_true="$sanitize_leak_true$1 " > +} > + > +sanitize_leak_false= > +add_sanitize_leak_false () { > + sanitize_leak_false="$sanitize_leak_false$1 " > +} > + > +sanitize_leak_opt_in_msg="opt-in with GIT_TEST_SANITIZE_LEAK=true" > +maybe_skip_all_sanitize_leak () { > + # Whitelist patterns > + add_sanitize_leak_true 't000*' > + add_sanitize_leak_true 't001*' > + add_sanitize_leak_true 't006*' > + > + # Blacklist patterns (overrides whitelist) > + add_sanitize_leak_false 't000[469]*' > + add_sanitize_leak_false 't001[2459]*' > + add_sanitize_leak_false 't006[0248]*' > + > + if match_pattern_list "$1" "$sanitize_leak_false" > + then > + skip_all="test $this_test on SANITIZE=leak blacklist, $sanitize_leak_opt_in_msg" > + test_done > + elif match_pattern_list "$1" "$sanitize_leak_true" > + then > + return 0 > + fi > + return 1 > +} > + > # Are we running this test at all? > remove_trash= > this_test=${0##*/} > @@ -1364,6 +1398,31 @@ then > test_done > fi > > +# Aggressively skip non-whitelisted tests when compiled with > +# SANITIZE=leak > +if test -n "$SANITIZE_LEAK" > +then > + if test -z "$GIT_TEST_SANITIZE_LEAK" && > + maybe_skip_all_sanitize_leak "$TEST_NAME" > + then > + say_color info >&3 "test $this_test on SANITIZE=leak whitelist" > + GIT_TEST_SANITIZE_LEAK=true > + fi > + > + # We need to see it in "git env--helper" (via > + # test_bool_env) > + export GIT_TEST_SANITIZE_LEAK > + > + if ! test_bool_env GIT_TEST_SANITIZE_LEAK false > + then > + skip_all="skip all tests in $this_test under SANITIZE=leak, $sanitize_leak_opt_in_msg" > + test_done > + fi > +elif test_bool_env GIT_TEST_SANITIZE_LEAK false > +then > + error "GIT_TEST_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak" > +fi > + > # Last-minute variable setup > HOME="$TRASH_DIRECTORY" > GNUPGHOME="$HOME/gnupg-home-not-used" > @@ -1516,6 +1575,7 @@ test -z "$NO_PYTHON" && test_set_prereq PYTHON > test -n "$USE_LIBPCRE2" && test_set_prereq PCRE > test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2 > test -z "$NO_GETTEXT" && test_set_prereq GETTEXT > +test -n "$SANITIZE_LEAK" && test_set_prereq SANITIZE_LEAK > > if test -z "$GIT_TEST_CHECK_CACHE_TREE" > then >
On Wed, Jul 14 2021, Andrzej Hunt wrote: > 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. I didn't know that, makes sense. I'll make it one job and have it use whatever CC is. > 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)? It sounds good to support that, but at least right now I've got the itch of finding leaks during development, and I think in any case being able to do a full run with just sanitizing, leak checking (or combined) makes sense, i.e. to make GIT_TEST_SANITIZE_LEAK=* the top-level interface. I haven't checked how noisy ASAN is, is it like the leak checking where we fail almost all tests now? Anyway, once we have some test mode like this it'll be trivial to extend it. I mainly want us to get this into CI so we can have an expanding line in the sand with regressions. > I think it's worth focusing on ASAN+UBSAN first because they tend to > find more impactful issues (e.g. buffer overflows, and other real > bugs) - whereas leaks... are ugly, but leaks in git don't actually > have much user impact? We have one-off commands, but also long-lived things like "git cat-file --batch", it's useful if we don't leak in those. The entry point to those tends to be one-off commands in tests, so checking leaks for all commands in a test (if you can get there) is a useful indicator for how the underlying API performs. I think in the git.git codebase we don't have much of an issue with buffer overflows etc, because we tend to consistently use APIs like strbuf that avoid those issues, but then again I haven't run the tests with that, maybe I'll be unpleasantly surprised. I also find leak checking to be useful during development to spot faulty assumptions, i.e. the leak itself may not be a big deal, but it's usually an early sign that I'm structuring something incorrectly. > [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? I did that in v1, as Đoàn Trần Công Danh pointed out we have other jobs that would match that. >> 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)? I'm not too familiar with git.git's ci/* dir, but I think it's like that in general because we don't just run in GitHub CI, but want to support Azure, Travis etc. So almost all of the logic is in those shellscripts, right now this target just runs in the GitHub CI, but it would be useful to make it drop-in enabled elsewhere. I think given that that doing anything overly clever in the YAML would probably be counter-productive.
On Wed, Jul 14, 2021 at 07:23:51PM +0200, Æ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 opening puzzled me. I'm not sure I understand why we need a special GIT_TEST_* mode for it. If you do "make SANITIZE=leak test", then your binaries will leak-check while running the tests. I.e., there is nothing that test-lib.sh itself needs to do differently to enable it. What we _do_ need is some mechanism of annotating to tests to say "this is known to leak", so that we can skip them for normal integration runs. And that is part of what's going on in this patch, but I'm not sure it is the simplest way to do it. The first question is: how do we want to annotate the tests. By marking individual scripts or tests in the test-files themselves? Or by using a separate list of "these scripts or tests are known to pass"? IMHO the latter is preferable. It keeps the annotations out of the way of normal work (they are a temporary thing until we eventually pass the whole suite leak free, but I expect they'll be with us for a while). The downside is that the annotations may get out of sync with test numbers. But if we are primarily annotating whole scripts (and not individual tests), then that is generally pretty stable. And with that in mind, can we just use an existing mechanism for picking which tests to run, and drive it externally from the CI job? We already have GIT_SKIP_TESTS and --run. Those are perhaps a bit awkward for feeding huge lists to, and there is no environment equivalent for --run (so you can't trigger it easily from "make test"). But what if we could do something like: GIT_TEST_RUN_FROM=t/leak-free make SANITIZE=leak test and then t/leak-free contained the usual patterns like: t000* t1234.5 and so on. That requires two new features in test-lib.sh: - making a GIT_TEST_RUN variable that is the opposite of GIT_TEST_SKIP (instead of just the command-line --run). - adding GIT_TEST_{RUN,SKIP}_FROM variables to read the values from a file rather than the environment (I suppose the caller could just stuff the contents into the variable, but I expect that test-lib.sh may want to pare down the entries that do not even apply to the current script for the sake of efficiency in checking each test). That infrastructure would then be applicable to other cases, too. Or even just useful for using another list (or no list at all) when you are looking at whether other tests are leak-free or not. > 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. I'm not clear on what we expect to get out of running it with both clang and gcc. They should be producing identical results. -Peff
On Wed, Jul 14, 2021 at 08:42:27PM +0200, Andrzej Hunt wrote: > 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)? I do think it's worth having an ASan+UBSan job. In the CI we use for our custom fork of Git at GitHub, we run it for every pull request (and I do bring upstream any applicable fixes). It's kind of expensive compared to a regular "make test", but probably not nearly as bad as just running the regular test suite on Windows. And it's true that ASan can do leak-checking, too. In the long run, when we are leak-free, I think it may make sense to combine the jobs. But in the interim state where we can run the whole suite with ASan/UBSan, but not with LSan, I think it's simpler to just keep them separate. That lets us just entirely skip tests or scripts in the leak-checking run. I haven't measured, but I also expect that LSan is not much more expensive than a regular run, so combining the two isn't that big a win). So I do like your suggestion, but I think it just be orthogonal further to leak-checking. -Peff
On Thu, Jul 15 2021, Jeff King wrote: > On Wed, Jul 14, 2021 at 07:23:51PM +0200, Æ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 opening puzzled me. I'm not sure I understand why we need a special > GIT_TEST_* mode for it. If you do "make SANITIZE=leak test", then your > binaries will leak-check while running the tests. > > I.e., there is nothing that test-lib.sh itself needs to do differently > to enable it. > > What we _do_ need is some mechanism of annotating to tests to say "this > is known to leak", so that we can skip them for normal integration runs. > > And that is part of what's going on in this patch, but I'm not sure it > is the simplest way to do it. The first question is: how do we want to > annotate the tests. By marking individual scripts or tests in the > test-files themselves? Or by using a separate list of "these scripts or > tests are known to pass"? > > IMHO the latter is preferable. It keeps the annotations out of the way > of normal work (they are a temporary thing until we eventually pass the > whole suite leak free, but I expect they'll be with us for a while). The > downside is that the annotations may get out of sync with test numbers. > But if we are primarily annotating whole scripts (and not individual > tests), then that is generally pretty stable. > > And with that in mind, can we just use an existing mechanism for picking > which tests to run, and drive it externally from the CI job? > > We already have GIT_SKIP_TESTS and --run. Those are perhaps a bit > awkward for feeding huge lists to, and there is no environment > equivalent for --run (so you can't trigger it easily from "make test"). > But what if we could do something like: > > GIT_TEST_RUN_FROM=t/leak-free make SANITIZE=leak test > > and then t/leak-free contained the usual patterns like: > > t000* > t1234.5 > > and so on. That requires two new features in test-lib.sh: > > - making a GIT_TEST_RUN variable that is the opposite of GIT_TEST_SKIP > (instead of just the command-line --run). > > - adding GIT_TEST_{RUN,SKIP}_FROM variables to read the values from a > file rather than the environment (I suppose the caller could just > stuff the contents into the variable, but I expect that test-lib.sh > may want to pare down the entries that do not even apply to the > current script for the sake of efficiency in checking each test). > > That infrastructure would then be applicable to other cases, too. Or > even just useful for using another list (or no list at all) when you > are looking at whether other tests are leak-free or not. I've included a mechanism for whitelisting specific globs, the idea was not to have that be too detailed, but we'd e.g. get to the point of t00* or whatever passing. Anything that's a lot more granular than that is doing to suck, e.g. exposing teh GIT_TEST_SKIP and --run features. of specific test numbers, now you need to count your tests if you add one in the middle of one of those, and more likely you won't test under the mode and just see it in CI. The marking at a distance I've done also has that problem in theory, but I think in practice we'll use it carefully for globs of tests unlikely to break. This whole thing is much more with the GIT_TEST_SANITIZE_LEAK mode, it's a really common case that we e.g. leak in some revision.c API user, we should fix that, but holding up marking the rest of at test whose entire tests otherwise pass is bad, it means you can't do any testing of a given API or subsystem without getting the entire file to pass. Whereas while we're fixing very common leaks in the codabase it's likely that any given test file will have a few such tests. It also means everything works by default, you get an appropriate notice from prove(1), and even if you run one test manually it'll skip, but emit a message saying you can set the env var to force its run. >> 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. > > I'm not clear on what we expect to get out of running it with both clang > and gcc. They should be producing identical results. Indeed, addressed elsewhere, i.e. it's just a thinko of mine.
On Fri, Jul 16, 2021 at 04:46:12PM +0200, Ævar Arnfjörð Bjarmason wrote: > > and so on. That requires two new features in test-lib.sh: > > > > - making a GIT_TEST_RUN variable that is the opposite of GIT_TEST_SKIP > > (instead of just the command-line --run). > > > > - adding GIT_TEST_{RUN,SKIP}_FROM variables to read the values from a > > file rather than the environment (I suppose the caller could just > > stuff the contents into the variable, but I expect that test-lib.sh > > may want to pare down the entries that do not even apply to the > > current script for the sake of efficiency in checking each test). > > > > That infrastructure would then be applicable to other cases, too. Or > > even just useful for using another list (or no list at all) when you > > are looking at whether other tests are leak-free or not. > > I've included a mechanism for whitelisting specific globs, the idea was > not to have that be too detailed, but we'd e.g. get to the point of t00* > or whatever passing. > > Anything that's a lot more granular than that is doing to suck, > e.g. exposing teh GIT_TEST_SKIP and --run features. of specific test > numbers, now you need to count your tests if you add one in the middle > of one of those, and more likely you won't test under the mode and just > see it in CI. I think you can do the same level of skipping with GIT_TEST_SKIP, though. My argument was just that adding a new mechanism does not make sense when we already have one. I.e., running: GIT_SKIP_TESTS=' t[123456789]* t0[^0]* t00[^016]* t000[469] t001[2459] t006[0248] ' make SANITIZE=leak test works already to do the same thing. The only thing we might want is a nicer syntax (e.g., to allow positive and negative patterns, or to read from a file). But that would benefit all users of GIT_SKIP_TESTS, not just people interested in leaks. > It also means everything works by default, you get an appropriate notice > from prove(1), and even if you run one test manually it'll skip, but > emit a message saying you can set the env var to force its run. With GIT_SKIP_TESTS you obviously don't get a message saying "try skipping this test" when it fails. :) But IMHO that is not that big a deal. You'll get a test failure with good LSan output. If you are working on expanding leak-checker coverage, you already know about your options for skipping. If you're adding a new test that leaks, you might consider fixing the leak (though not always, if it's far from code you're touching). -Peff
On Fri, Jul 16, 2021 at 02:09:22PM -0400, Jeff King wrote: > > Anything that's a lot more granular than that is doing to suck, > > e.g. exposing teh GIT_TEST_SKIP and --run features. of specific test > > numbers, now you need to count your tests if you add one in the middle > > of one of those, and more likely you won't test under the mode and just > > see it in CI. > > I think you can do the same level of skipping with GIT_TEST_SKIP, > though. My argument was just that adding a new mechanism does not make > sense when we already have one. I.e., running: > > GIT_SKIP_TESTS=' > t[123456789]* > t0[^0]* > t00[^016]* > t000[469] > t001[2459] > t006[0248] > ' make SANITIZE=leak test > > works already to do the same thing. The only thing we might want is a > nicer syntax (e.g., to allow positive and negative patterns, or to read > from a file). But that would benefit all users of GIT_SKIP_TESTS, not > just people interested in leaks. I cheated a little here; an unrelated bug does cause a failure in t0000 with this pattern. I've just sent: https://lore.kernel.org/git/YPHTY5G9JaQFKlX5@coredump.intra.peff.net/ to fix it. -Peff
On Fri, Jul 16 2021, Jeff King wrote: > On Fri, Jul 16, 2021 at 04:46:12PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > and so on. That requires two new features in test-lib.sh: >> > >> > - making a GIT_TEST_RUN variable that is the opposite of GIT_TEST_SKIP >> > (instead of just the command-line --run). >> > >> > - adding GIT_TEST_{RUN,SKIP}_FROM variables to read the values from a >> > file rather than the environment (I suppose the caller could just >> > stuff the contents into the variable, but I expect that test-lib.sh >> > may want to pare down the entries that do not even apply to the >> > current script for the sake of efficiency in checking each test). >> > >> > That infrastructure would then be applicable to other cases, too. Or >> > even just useful for using another list (or no list at all) when you >> > are looking at whether other tests are leak-free or not. >> >> I've included a mechanism for whitelisting specific globs, the idea was >> not to have that be too detailed, but we'd e.g. get to the point of t00* >> or whatever passing. >> >> Anything that's a lot more granular than that is doing to suck, >> e.g. exposing teh GIT_TEST_SKIP and --run features. of specific test >> numbers, now you need to count your tests if you add one in the middle >> of one of those, and more likely you won't test under the mode and just >> see it in CI. > > I think you can do the same level of skipping with GIT_TEST_SKIP, > though. My argument was just that adding a new mechanism does not make > sense when we already have one. I.e., running: > > GIT_SKIP_TESTS=' > t[123456789]* > t0[^0]* > t00[^016]* > t000[469] > t001[2459] > t006[0248] > ' make SANITIZE=leak test > > works already to do the same thing. The only thing we might want is a > nicer syntax (e.g., to allow positive and negative patterns, or to read > from a file). But that would benefit all users of GIT_SKIP_TESTS, not > just people interested in leaks. A glob in this series is t13*config*, you can't do that with GIT_SKIP_TESTS because it only includes the numeric part of the test, i.e. t1300, not t1300-config, or t1306-xdg-files. But sure, it could happen via some other mechanism than the exact one I picked, or we could add GIT_SKIP_TESTS2 or whatever. I would like to be able to compile with it and run "make test" without a wall of failures by default, i.e. we should be able to tell regressions from known-OK to get anywhere with it, but that's orthagonal to the exact mechanism. >> It also means everything works by default, you get an appropriate notice >> from prove(1), and even if you run one test manually it'll skip, but >> emit a message saying you can set the env var to force its run. > > With GIT_SKIP_TESTS you obviously don't get a message saying "try > skipping this test" when it fails. :) But IMHO that is not that big a > deal. You'll get a test failure with good LSan output. If you are > working on expanding leak-checker coverage, you already know about your > options for skipping. If you're adding a new test that leaks, you might > consider fixing the leak (though not always, if it's far from code > you're touching). I do think it makes sense as a test mode test-lib.sh is aware of, e.g. on obvious next step is to not fail everything right away, but just let the test run and log all failures to a file, then e.g. fail one test at the end, or if we're running in that mode collate all the callstacks and emit a summary for the whole test run. But yes, the message it emits now isn't such a big deal.
On Fri, Jul 16, 2021 at 08:56:04PM +0200, Ævar Arnfjörð Bjarmason wrote: > > I think you can do the same level of skipping with GIT_TEST_SKIP, > > though. My argument was just that adding a new mechanism does not make > > sense when we already have one. I.e., running: > > > > GIT_SKIP_TESTS=' > > t[123456789]* > > t0[^0]* > > t00[^016]* > > t000[469] > > t001[2459] > > t006[0248] > > ' make SANITIZE=leak test > > > > works already to do the same thing. The only thing we might want is a > > nicer syntax (e.g., to allow positive and negative patterns, or to read > > from a file). But that would benefit all users of GIT_SKIP_TESTS, not > > just people interested in leaks. > > A glob in this series is t13*config*, you can't do that with > GIT_SKIP_TESTS because it only includes the numeric part of the test, > i.e. t1300, not t1300-config, or t1306-xdg-files. That seems like a feature that GIT_SKIP_TESTS could learn (though IMHO just using the test number in your patterns is sufficient). > I would like to be able to compile with it and run "make test" without a > wall of failures by default, i.e. we should be able to tell regressions > from known-OK to get anywhere with it, but that's orthagonal to the > exact mechanism. Right, I definitely agree on the goal. I just don't see the need to add a new, very-specific mechanism. The skip-list above is gross and obviously not something you'd want to type. Driving it from a ci script is not too bad, but I agree people who want to leak-check locally would want an easy way to use it, too. That's why I suggested extending it to a file that could be easily specified (and possibly even auto-triggered in the Makefile by SANITIZE=leak). > > With GIT_SKIP_TESTS you obviously don't get a message saying "try > > skipping this test" when it fails. :) But IMHO that is not that big a > > deal. You'll get a test failure with good LSan output. If you are > > working on expanding leak-checker coverage, you already know about your > > options for skipping. If you're adding a new test that leaks, you might > > consider fixing the leak (though not always, if it's far from code > > you're touching). > > I do think it makes sense as a test mode test-lib.sh is aware of, > e.g. on obvious next step is to not fail everything right away, but just > let the test run and log all failures to a file, then e.g. fail one test > at the end, or if we're running in that mode collate all the callstacks > and emit a summary for the whole test run. That's a more compelling reason, if we did implement that feature. My hope was that all of this would be a temporary state, though, and we'd get to a point where you can simply run "make SANITIZE=leak test" and actually run all of the tests. And then such a feature would not be that interesting, because failures would be rare and cause for immediate human attention. -Peff
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 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) 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 + MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}" diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index 3ce81ffee9..5fe047b5c6 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -12,7 +12,7 @@ esac make case "$jobname" in -linux-gcc) +linux-gcc|linux-gcc-sanitize-leak) export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main make test export GIT_TEST_SPLIT_INDEX=yes @@ -29,7 +29,7 @@ linux-gcc) export GIT_TEST_CHECKOUT_WORKERS=2 make test ;; -linux-clang) +linux-clang|linux-clang-sanitize-leak) export GIT_TEST_DEFAULT_HASH=sha1 make test export GIT_TEST_DEFAULT_HASH=sha256 diff --git a/t/README b/t/README index 1a2072b2c8..303d0be817 100644 --- a/t/README +++ b/t/README @@ -448,6 +448,22 @@ GIT_TEST_CHECKOUT_WORKERS=<n> overrides the 'checkout.workers' setting to <n> and 'checkout.thresholdForParallelism' to 0, forcing the execution of the parallel-checkout code. +GIT_TEST_SANITIZE_LEAK=<boolean> will force the tests to run when git +is compiled with SANITIZE=leak (we pick it up via +../GIT-BUILD-OPTIONS). + +By default all tests are skipped when compiled with SANITIZE=leak, and +individual test scripts opt themselves in to leak testing by setting +GIT_TEST_SANITIZE_LEAK=true before sourcing test-lib.sh. Within those +tests use the SANITIZE_LEAK prerequisite to skip individiual tests +(i.e. test_expect_success !SANITIZE_LEAK [...]). + +So the GIT_TEST_SANITIZE_LEAK setting is different in behavior from +both other GIT_TEST_*=[true|false] settings, but more useful given how +SANITIZE=leak works & the state of the test suite. Manually setting +GIT_TEST_SANITIZE_LEAK=true is only useful during development when +finding and fixing memory leaks. + Naming Tests ------------ diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh index 930721f053..d58efb0aa9 100755 --- a/t/t5701-git-serve.sh +++ b/t/t5701-git-serve.sh @@ -243,7 +243,7 @@ test_expect_success 'unexpected lines are not allowed in fetch request' ' # Test the basics of object-info # -test_expect_success 'basics of object-info' ' +test_expect_success !SANITIZE_LEAK 'basics of object-info' ' test-tool pkt-line pack >in <<-EOF && command=object-info object-format=$(test_oid algo) diff --git a/t/test-lib.sh b/t/test-lib.sh index 7036f83b33..9201510e16 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1353,6 +1353,40 @@ then exit 1 fi +# SANITIZE=leak test mode +sanitize_leak_true= +add_sanitize_leak_true () { + sanitize_leak_true="$sanitize_leak_true$1 " +} + +sanitize_leak_false= +add_sanitize_leak_false () { + sanitize_leak_false="$sanitize_leak_false$1 " +} + +sanitize_leak_opt_in_msg="opt-in with GIT_TEST_SANITIZE_LEAK=true" +maybe_skip_all_sanitize_leak () { + # Whitelist patterns + add_sanitize_leak_true 't000*' + add_sanitize_leak_true 't001*' + add_sanitize_leak_true 't006*' + + # Blacklist patterns (overrides whitelist) + add_sanitize_leak_false 't000[469]*' + add_sanitize_leak_false 't001[2459]*' + add_sanitize_leak_false 't006[0248]*' + + if match_pattern_list "$1" "$sanitize_leak_false" + then + skip_all="test $this_test on SANITIZE=leak blacklist, $sanitize_leak_opt_in_msg" + test_done + elif match_pattern_list "$1" "$sanitize_leak_true" + then + return 0 + fi + return 1 +} + # Are we running this test at all? remove_trash= this_test=${0##*/} @@ -1364,6 +1398,31 @@ then test_done fi +# Aggressively skip non-whitelisted tests when compiled with +# SANITIZE=leak +if test -n "$SANITIZE_LEAK" +then + if test -z "$GIT_TEST_SANITIZE_LEAK" && + maybe_skip_all_sanitize_leak "$TEST_NAME" + then + say_color info >&3 "test $this_test on SANITIZE=leak whitelist" + GIT_TEST_SANITIZE_LEAK=true + fi + + # We need to see it in "git env--helper" (via + # test_bool_env) + export GIT_TEST_SANITIZE_LEAK + + if ! test_bool_env GIT_TEST_SANITIZE_LEAK false + then + skip_all="skip all tests in $this_test under SANITIZE=leak, $sanitize_leak_opt_in_msg" + test_done + fi +elif test_bool_env GIT_TEST_SANITIZE_LEAK false +then + error "GIT_TEST_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak" +fi + # Last-minute variable setup HOME="$TRASH_DIRECTORY" GNUPGHOME="$HOME/gnupg-home-not-used" @@ -1516,6 +1575,7 @@ test -z "$NO_PYTHON" && test_set_prereq PYTHON test -n "$USE_LIBPCRE2" && test_set_prereq PCRE test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2 test -z "$NO_GETTEXT" && test_set_prereq GETTEXT +test -n "$SANITIZE_LEAK" && test_set_prereq SANITIZE_LEAK if test -z "$GIT_TEST_CHECK_CACHE_TREE" then
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(-)