| Message ID | cover-v5-0.3-00000000000-20210907T212626Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add a test mode for SANITIZE=leak, run it in CI | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > We can compile git with SANITIZE=leak, and have had various efforts in > the past such as 31f9acf9ce2 (Merge branch 'ah/plugleaks', 2021-08-04) > to plug memory leaks, but have had no CI testing of it to ensure that > we don't get regressions. This series adds a GIT_TEST_* mode for > checking those regressions, and runs it in CI. > > Since I submitted v2 the delta between origin/master..origin/seen > broke even t0001-init.sh when run under SANITIZE=leak, so this series > will cause test smoke on "seen". > > That failure is due to a bug in es/config-based-hooks [1] and the > hn/reftable topic, i.e. these patches are legitimately catching > regressions in "seen" from day 1. So is there a point in sending this out to the list, before sending fixes to these broken topic and making sure they get corrected? Because the CI does not "bisect" to tell us "ok, up to this point in 'seen', all the topics merged play well together", the overall effect in the bigger picture is that 'seen' with this series would cause CI to stay in failed state. For now, I'll keep this near the tip of 'seen'. Thanks.
On Wed, Sep 08 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> We can compile git with SANITIZE=leak, and have had various efforts in >> the past such as 31f9acf9ce2 (Merge branch 'ah/plugleaks', 2021-08-04) >> to plug memory leaks, but have had no CI testing of it to ensure that >> we don't get regressions. This series adds a GIT_TEST_* mode for >> checking those regressions, and runs it in CI. >> >> Since I submitted v2 the delta between origin/master..origin/seen >> broke even t0001-init.sh when run under SANITIZE=leak, so this series >> will cause test smoke on "seen". >> >> That failure is due to a bug in es/config-based-hooks [1] and the >> hn/reftable topic, i.e. these patches are legitimately catching >> regressions in "seen" from day 1. > > So is there a point in sending this out to the list, before sending > fixes to these broken topic and making sure they get corrected? > > Because the CI does not "bisect" to tell us "ok, up to this point in > 'seen', all the topics merged play well together", the overall > effect in the bigger picture is that 'seen' with this series would > cause CI to stay in failed state. > > For now, I'll keep this near the tip of 'seen'. The breakages with it are in combination with: ab/config-based-hooks-base es/config-based-hooks hn/reftable You've got v4 of ab/config-based-hooks-base, the v5 is at [1], but we've been waiting on emily to re-roll hers on top. As noted in that E-Mail I've got a working re-roll of it as avar-nasamuffin/config-based-hooks-restart-3 in my repo. That'll leave hn/reftable, which given [2] I thought you were planning to eject, and wiht the number of fixups for it / the planned re-doing of it by Han-Wen[3] maybe it's better to do that now? What do you think about that plan? I.e. ejecting hn/reftable while waiting on a re-roll, and either ejecting es/config-based-hooks while waiting, or I can submit the avar-nasamuffin/config-based-hooks-restart-3 I've got pending Emily's own re-roll (which may or may not be different from that). That along with picking up the v5 of my ab/config-based-hooks-base should make "seen" pass with SANITIZE=leak on these tests, unless there's other just-introduced regressions. I tried re-building it a few days ago, I haven't done that just now. 1. https://lore.kernel.org/git/cover-v5-00.36-00000000000-20210902T125110Z-avarab@gmail.com/ 2. https://lore.kernel.org/git/xmqq4kaxe5dt.fsf@gitster.g/ 3. https://lore.kernel.org/git/CAFQ2z_N8pUsp3cdBpybHBD-V9_1sARCZvSxr0UkMfcwCoQfCbw@mail.gmail.com/
On Wed, Sep 08, 2021 at 02:03:30PM +0200, Ævar Arnfjörð Bjarmason wrote: > > > On Wed, Sep 08 2021, Junio C Hamano wrote: > > > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > >> We can compile git with SANITIZE=leak, and have had various efforts in > >> the past such as 31f9acf9ce2 (Merge branch 'ah/plugleaks', 2021-08-04) > >> to plug memory leaks, but have had no CI testing of it to ensure that > >> we don't get regressions. This series adds a GIT_TEST_* mode for > >> checking those regressions, and runs it in CI. > >> > >> Since I submitted v2 the delta between origin/master..origin/seen > >> broke even t0001-init.sh when run under SANITIZE=leak, so this series > >> will cause test smoke on "seen". > >> > >> That failure is due to a bug in es/config-based-hooks [1] and the > >> hn/reftable topic, i.e. these patches are legitimately catching > >> regressions in "seen" from day 1. > > > > So is there a point in sending this out to the list, before sending > > fixes to these broken topic and making sure they get corrected? > > > > Because the CI does not "bisect" to tell us "ok, up to this point in > > 'seen', all the topics merged play well together", the overall > > effect in the bigger picture is that 'seen' with this series would > > cause CI to stay in failed state. > > > > For now, I'll keep this near the tip of 'seen'. > > The breakages with it are in combination with: > > ab/config-based-hooks-base > es/config-based-hooks > hn/reftable > > You've got v4 of ab/config-based-hooks-base, the v5 is at [1], but we've > been waiting on emily to re-roll hers on top. As noted in that E-Mail > I've got a working re-roll of it as > avar-nasamuffin/config-based-hooks-restart-3 in my repo. > > That'll leave hn/reftable, which given [2] I thought you were planning > to eject, and wiht the number of fixups for it / the planned re-doing of > it by Han-Wen[3] maybe it's better to do that now? > > What do you think about that plan? > > I.e. ejecting hn/reftable while waiting on a re-roll, and either > ejecting es/config-based-hooks while waiting, or I can submit the > avar-nasamuffin/config-based-hooks-restart-3 I've got pending Emily's > own re-roll (which may or may not be different from that). My own reroll is waiting on some feedback internally and probably won't show up this week at all, so I suggest to kick mine out and prioritize the reftable stuff for now. - Emily > > That along with picking up the v5 of my ab/config-based-hooks-base > should make "seen" pass with SANITIZE=leak on these tests, unless > there's other just-introduced regressions. I tried re-building it a few > days ago, I haven't done that just now. > > 1. https://lore.kernel.org/git/cover-v5-00.36-00000000000-20210902T125110Z-avarab@gmail.com/ > 2. https://lore.kernel.org/git/xmqq4kaxe5dt.fsf@gitster.g/ > 3. https://lore.kernel.org/git/CAFQ2z_N8pUsp3cdBpybHBD-V9_1sARCZvSxr0UkMfcwCoQfCbw@mail.gmail.com/
We can compile git with SANITIZE=leak, and have had various efforts in the past such as 31f9acf9ce2 (Merge branch 'ah/plugleaks', 2021-08-04) to plug memory leaks, but have had no CI testing of it to ensure that we don't get regressions. This series adds a GIT_TEST_* mode for checking those regressions, and runs it in CI. Since I submitted v2 the delta between origin/master..origin/seen broke even t0001-init.sh when run under SANITIZE=leak, so this series will cause test smoke on "seen". That failure is due to a bug in es/config-based-hooks [1] and the hn/reftable topic, i.e. these patches are legitimately catching regressions in "seen" from day 1. Changes since v4 (see https://lore.kernel.org/git/cover-v4-0.3-00000000000-20210907T151855Z-avarab@gmail.com/): * Renamed the jobs to linux-leaks and osx-leaks, per Jeff King's suggestion. * Took all the suggestions from Jeff King for commit message improvements, and tried to make some of my own fixing overly verbose wording/grammar errors etc. * Ditto the small typo fix Eric Sunshine pointed out. Thanks both! See https://github.com/avar/git/runs/3538356269 for the CI run for this version. Ævar Arnfjörð Bjarmason (3): 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 .github/workflows/main.yml | 6 ++++++ Makefile | 5 +++++ ci/install-dependencies.sh | 6 +++--- ci/lib.sh | 31 +++++++++++++++++++++---------- ci/run-build-and-tests.sh | 2 +- t/README | 7 +++++++ t/t0000-basic.sh | 1 + t/t0004-unwritable.sh | 3 ++- t/test-lib.sh | 21 +++++++++++++++++++++ 9 files changed, 67 insertions(+), 15 deletions(-) Range-diff against v4: 1: bdfe2279271 = 1: bdfe2279271 Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS 2: 6aaa60e3759 = 2: 6aaa60e3759 CI: refactor "if" to "case" statement 3: fffbfc35c00 ! 3: f3cd04b16d1 tests: add a test mode for SANITIZE=leak, run it in CI @@ Metadata ## Commit message ## tests: add a test mode for SANITIZE=leak, run it in CI - While git can be compiled with SANITIZE=leak we have not run - regression tests under that mode, memory leaks have only been fixed as + While git can be compiled with SANITIZE=leak, we have not run + regression tests under that mode. Memory leaks have only been fixed as one-offs without structured regression testing. - This change add CI testing for it. We'll now build with GCC under - Linux and test t000[04]*.sh with SANITIZE=leak, and likewise with GCC - on OSX. The new jobs are called "linux-SANITIZE=leak" and - "osx-SANITIZE=leak". + This change adds CI testing for it. We'll now build and test + t000[04]*.sh under both Linux and OSX. The new jobs are called + "linux-leaks" and "osx-leaks". The CI target uses a new GIT_TEST_PASSING_SANITIZE_LEAK=true test mode. When running in that mode, we'll assert that we were compiled - with SANITIZE=leak, and then skip all tests except those that we've - opted-in by setting "TEST_PASSES_SANITIZE_LEAK=true" before sourcing - test-lib.sh (see discussion in t/README). + with SANITIZE=leak. We'll then skip all tests, except those that we've + opted-in by setting "TEST_PASSES_SANITIZE_LEAK=true". - The tests using the "TEST_PASSES_SANITIZE_LEAK=true" setting can in + A test tests setting "TEST_PASSES_SANITIZE_LEAK=true" setting can in turn make use of the "SANITIZE_LEAK" prerequisite, should they wish to selectively skip tests even under "GIT_TEST_PASSING_SANITIZE_LEAK=true". In a preceding commit we started doing this in "t0004-unwritable.sh" under SANITIZE=leak, now it'll combine nicely with "GIT_TEST_PASSING_SANITIZE_LEAK=true". - Now tests that don't set "TEST_PASSES_SANITIZE_LEAK=true" will be - skipped under GIT_TEST_PASSING_SANITIZE_LEAK=true: + This is how tests that don't set "TEST_PASSES_SANITIZE_LEAK=true" will + be skipped under GIT_TEST_PASSING_SANITIZE_LEAK=true: $ GIT_TEST_PASSING_SANITIZE_LEAK=true ./t0001-init.sh 1..0 # SKIP skip all tests in t0001 under SANITIZE=leak, TEST_PASSES_SANITIZE_LEAK not set @@ Commit message commit-graph tests). To be clear having a prerequisite could also be accomplished by using "LSAN_OPTIONS" directly. - On the topi of "LSAN_OPTIONS": It would be nice to have a mode to + On the topic of "LSAN_OPTIONS": It would be nice to have a mode to aggregate all failures in our various scripts, see [2] for a start at doing that which sets "log_path" in "LSAN_OPTIONS". I've punted on - that for now, it can be added later, and that proposed patch is also - hindered by us wanting to test e.g. test-tool leaks (and by proxy, any - API leaks they uncover), not just the "common-main.c" entry point. + that for now, it can be added later. As of writing this we've got major regressions between master..seen, i.e. the t000*.sh tests and more fixed since 31f9acf9ce2 (Merge branch 'ah/plugleaks', 2021-08-04) have regressed recently. - See the discussion at <87czsv2idy.fsf@evledraar.gmail.com> 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 the discussion at <87czsv2idy.fsf@evledraar.gmail.com>[3] 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. - The reason for using gcc on OSX over the clang default is because - it'll currently fail to build with: + The reason for using gcc on OSX over the clang default is because when + used with clang on "macos-latest" it'll currently fail to build with: clang: error: unsupported option '-fsanitize=leak' for target 'x86_64-apple-darwin19.6.0' If that's sorted out in the future we might want to run that job with "clang" merely to make use of the default, and also to add some compiler variance into the mix. Both use the - "AddressSanitizerLeakSanitizer" library[3], so in they shouldn't be - have differently under GCC or clang. + "AddressSanitizerLeakSanitizer" library[4], so in they shouldn't + behave differently under GCC or clang. 1. https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer 2. https://lore.kernel.org/git/YS9OT%2Fpn5rRK9cGB@coredump.intra.peff.net/ - 3. https://lore.kernel.org/git/YS9ZIDpANfsh7N+S@coredump.intra.peff.net/ + 3. https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/ + 4. https://lore.kernel.org/git/YS9ZIDpANfsh7N+S@coredump.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> @@ .github/workflows/main.yml: jobs: - jobname: linux-gcc-default cc: gcc pool: ubuntu-latest -+ - jobname: linux-SANITIZE=leak ++ - jobname: linux-leaks + cc: gcc + pool: ubuntu-latest -+ - jobname: osx-SANITIZE=leak ++ - jobname: osx-leaks + cc: gcc + pool: macos-latest env: @@ ci/install-dependencies.sh: UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl case "$jobname" in -linux-clang|linux-gcc) -+linux-clang|linux-gcc|linux-SANITIZE=leak) ++linux-clang|linux-gcc|linux-leaks) 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-SANITIZE=leak) ++ linux-gcc|linux-leaks) sudo apt-get -q -y install gcc-8 ;; esac @@ ci/install-dependencies.sh: linux-clang|linux-gcc) popd ;; -osx-clang|osx-gcc) -+osx-clang|osx-gcc|osx-SANITIZE=leak) ++osx-clang|osx-gcc|osx-leaks) export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1 # Uncomment this if you want to run perf tests: # brew install gnu-time @@ ci/lib.sh: export GIT_TEST_CLONE_2GB=true case "$jobname" in -linux-clang|linux-gcc) -+linux-clang|linux-gcc|linux-SANITIZE=leak) ++linux-clang|linux-gcc|linux-leaks) case "$jobname" in - linux-gcc) -+ linux-gcc|linux-SANITIZE=leak) ++ linux-gcc|linux-leaks) export CC=gcc-8 MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3" ;; @@ ci/lib.sh: linux-clang|linux-gcc) export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH" ;; -osx-clang|osx-gcc) -+osx-clang|osx-gcc|osx-SANITIZE=leak) ++osx-clang|osx-gcc|osx-leaks) case "$jobname" in - osx-gcc) -+ osx-gcc|osx-SANITIZE=leak) ++ osx-gcc|osx-leaks) export CC=gcc-9 MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)" ;; @@ ci/lib.sh: linux-musl) esac +case "$jobname" in -+linux-SANITIZE=leak|osx-SANITIZE=leak) ++linux-leaks|osx-leaks) + export SANITIZE=leak + export GIT_TEST_PASSING_SANITIZE_LEAK=true + ;; @@ ci/run-build-and-tests.sh: esac make case "$jobname" in -linux-gcc) -+linux-gcc|linux-SANITIZE=leak) ++linux-gcc|linux-leaks) export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main make test export GIT_TEST_SPLIT_INDEX=yes @@ t/README: excluded as so much relies on it, but this might change in the future. +themselves as passing with no memory leaks. Tests can be whitelisted +by setting "TEST_PASSES_SANITIZE_LEAK=true" before sourcing +"test-lib.sh" itself at the top of the test script. This test mode is -+used by the "linux-SANITIZE=leak" CI target. ++used by the "linux-leaks" CI target. + GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version' default to n.