{"thread":{"id":"60152","subject":"[PATCH 0/3] leak tests: mark remaining tests leak-free as such","startedAt":"2023-08-24T18:41:12Z","lastAt":"2023-08-29T16:44:02Z","messageCount":21,"participants":["Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"481004","messageId":"cover.1692902414.git.me@ttaylorr.com","threadId":"60152","inReplyTo":null,"subject":"[PATCH 0/3] leak tests: mark remaining tests leak-free as such","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-24T18:40:34Z","receivedAt":"2023-08-24T18:41:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"While working on another topic that cleared up some leaks, I wanted to\nsee if any new tests became leak-free, so I ran:\n\n    $ make SANITIZE=leak\n    $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test\n\n, and was surprised to see so many tests appear in the \"failed\" list\n(indicating that they are free of leaks, but did not mark themselves as\nsuch).\n\nIn fact, the patch I had written at the time didn't make a dent one way\nor another in the list of leak-free tests, as the same results were\nproduced on 'master' without my changes.\n\nThis series marks all leak-free tests as such, meaning that the above\n\"make test\" invocation will pass after this series. The bulk of the\ntests which are marked here in the first patch were always\nleak-free[^1]. The remaining two patches address a couple of special\ncases of tests which are also leak-free.\n\nThanks in advance for your review!\n\n[^1]: At least as far back as v2.38.0, when the \"check\" mode of\n  GIT_TEST_PASSING_SANITIZE_LEAK was first introduced.\n\nTaylor Blau (3):\n  leak tests: mark a handful of tests as leak-free\n  leak tests: mark t3321-notes-stripspace.sh as leak-free\n  leak tests: mark t5583-push-branches.sh as leak-free\n\n t/t3321-notes-stripspace.sh | 1 +\n t/t5571-pre-push-hook.sh    | 1 +\n t/t5583-push-branches.sh    | 1 +\n t/t7516-commit-races.sh     | 2 ++\n 4 files changed, 5 insertions(+)\n\n-- \n2.42.0.3.g4011eb6a8b\n"},{"id":"481005","messageId":"cfeca8894201963b7443fab7ed59c6a596e0ac3a.1692902414.git.me@ttaylorr.com","threadId":"60152","inReplyTo":"cover.1692902414.git.me@ttaylorr.com","subject":"[PATCH 2/3] leak tests: mark t3321-notes-stripspace.sh as leak-free","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-24T18:40:41Z","receivedAt":"2023-08-24T18:41:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This test was leak-free when t3321 was originally introduced, but never\nmarked as such:\n\n    $ rev=\"$(git log --format='%H' --reverse -1 HEAD^ -- t/t3321-notes-stripspace.sh)\"\n    $ git checkout $rev\n\n    $ make SANITIZE=leak\n    [...]\n\n    $ make -C t GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=--immediate t3321-notes-stripspace.sh\n    [...]\n    # passed all 27 test(s)\n    1..27\n    # faking up non-zero exit with --invert-exit-code\n    make: *** [Makefile:66: t3321-notes-stripspace.sh] Error 1\n    make: Leaving directory '/home/ttaylorr/src/git/t'\n\nMark this test as leak-free accordingly.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t3321-notes-stripspace.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t3321-notes-stripspace.sh b/t/t3321-notes-stripspace.sh\nindex 028d825e8f..36abdca5ee 100755\n--- a/t/t3321-notes-stripspace.sh\n+++ b/t/t3321-notes-stripspace.sh\n@@ -5,6 +5,7 @@\n \n test_description='Test commit notes with stripspace behavior'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n MULTI_LF=\"$LF$LF$LF\"\n-- \n2.42.0.3.g4011eb6a8b\n\n"},{"id":"481006","messageId":"b1711c4c817f95011bc477a9485c115b4926c7da.1692902414.git.me@ttaylorr.com","threadId":"60152","inReplyTo":"cover.1692902414.git.me@ttaylorr.com","subject":"[PATCH 1/3] leak tests: mark a handful of tests as leak-free","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-24T18:40:38Z","receivedAt":"2023-08-24T18:41:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the topic merged via 5a4f8381b6 (Merge branch\n'ab/mark-leak-free-tests', 2021-10-25), a handful of tests in the suite\nwere marked as leak-free.\n\nAs far as I can tell, each patch from that series ran tests from a\nhandful of subject areas, such as \"some ls-files tests\", or \"all trace2\ntests\". This left some gaps in which tests had and hadn't been audited\nto be leak-free.\n\nThis patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true\nbefore sourcing t/test-lib.sh on most remaining leak-free tests. This\nlist was compiled by doing:\n\n    $ make SANITIZE=leak\n    $ make \\\n        GIT_TEST_PASSING_SANITIZE_LEAK=check \\\n        GIT_TEST_SANITIZE_LEAK_LOG=true \\\n        GIT_TEST_OPTS=-vi test\n\nand looking through the list of failing tests in the output.\n\nThere are a couple of other tests which are similarly leak-free, but not\nincluded in the list of tests touched by this patch. The remaining tests\nwill be addressed in the subsequent two patches.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5571-pre-push-hook.sh | 1 +\n t/t7516-commit-races.sh  | 2 ++\n 2 files changed, 3 insertions(+)\n\ndiff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh\nindex a11b20e378..448134c4bf 100755\n--- a/t/t5571-pre-push-hook.sh\n+++ b/t/t5571-pre-push-hook.sh\n@@ -4,6 +4,7 @@ test_description='check pre-push hooks'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh\nindex 2d38a16480..bb95f09810 100755\n--- a/t/t7516-commit-races.sh\n+++ b/t/t7516-commit-races.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='git commit races'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'race to create orphan commit' '\n-- \n2.42.0.3.g4011eb6a8b\n\n"},{"id":"481007","messageId":"4011eb6a8b6200da88082a313f67bfeeca536d6a.1692902414.git.me@ttaylorr.com","threadId":"60152","inReplyTo":"cover.1692902414.git.me@ttaylorr.com","subject":"[PATCH 3/3] leak tests: mark t5583-push-branches.sh as leak-free","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-24T18:40:43Z","receivedAt":"2023-08-24T18:41:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When t5583-push-branches.sh was originally introduced via 425b4d7f47\n(push: introduce '--branches' option, 2023-05-06), it was not leak-free.\nIn fact, the test did not even run correctly until 022fbb655d (t5583:\nfix shebang line, 2023-05-12), but after applying that patch, we see a\nfailure at t5583.8:\n\n    ==2529087==ERROR: LeakSanitizer: detected memory leaks\n\n    Direct leak of 384 byte(s) in 1 object(s) allocated from:\n        #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98\n        #1 0x55e07606cbf9 in xrealloc wrapper.c:140\n        #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42\n        #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917\n        #4 0x55e075fe9cce in add_missing_tags remote.c:1518\n        #5 0x55e075fea1e4 in match_push_refs remote.c:1665\n        #6 0x55e076050a8e in transport_push transport.c:1378\n        #7 0x55e075e2eb74 in push_with_options builtin/push.c:401\n        #8 0x55e075e2edb0 in do_push builtin/push.c:458\n        #9 0x55e075e2ff7a in cmd_push builtin/push.c:702\n        #10 0x55e075d8aaf0 in run_builtin git.c:452\n        #11 0x55e075d8af08 in handle_builtin git.c:706\n        #12 0x55e075d8b12c in run_argv git.c:770\n        #13 0x55e075d8b6a0 in cmd_main git.c:905\n        #14 0x55e075e81f07 in main common-main.c:60\n        #15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n        #16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360\n        #17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453)\n\n    SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s).\n\nThis leak was addressed independently via 68b51172e3 (commit-reach: fix\nmemory leak in get_reachable_subset(), 2023-06-03), which makes t5583\nleak-free.\n\nBut t5583 was not in the tree when 68b51172e3 was written, and the two\nonly met after the latter was merged back in via 693bde461c (Merge\nbranch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20).\n\nAt that point, t5583 was leak-free. Let's mark it as such accordingly.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5583-push-branches.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t5583-push-branches.sh b/t/t5583-push-branches.sh\nindex e7e1b6dab6..320f49c753 100755\n--- a/t/t5583-push-branches.sh\n+++ b/t/t5583-push-branches.sh\n@@ -5,6 +5,7 @@ test_description='check the consisitency of behavior of --all and --branches'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n delete_refs() {\n-- \n2.42.0.3.g4011eb6a8b\n"},{"id":"481009","messageId":"xmqqbkewb7g4.fsf@gitster.g","threadId":"60152","inReplyTo":"cover.1692902414.git.me@ttaylorr.com","subject":"Re: [PATCH 0/3] leak tests: mark remaining tests leak-free as such","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-24T18:50:51Z","receivedAt":"2023-08-24T18:51:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> This series marks all leak-free tests as such, meaning that the above\n> \"make test\" invocation will pass after this series. The bulk of the\n> tests which are marked here in the first patch were always\n> leak-free[^1]. The remaining two patches address a couple of special\n> cases of tests which are also leak-free.\n>\n> Thanks in advance for your review!\n>\n> [^1]: At least as far back as v2.38.0, when the \"check\" mode of\n>   GIT_TEST_PASSING_SANITIZE_LEAK was first introduced.\n\nNice to see \"bugs\" gone without any code changes ;-) It would have\ntaken a lot of patience from your part, though, and your effort is\nvery much appreciated.\n\n\n\n> Taylor Blau (3):\n>   leak tests: mark a handful of tests as leak-free\n>   leak tests: mark t3321-notes-stripspace.sh as leak-free\n>   leak tests: mark t5583-push-branches.sh as leak-free\n>\n>  t/t3321-notes-stripspace.sh | 1 +\n>  t/t5571-pre-push-hook.sh    | 1 +\n>  t/t5583-push-branches.sh    | 1 +\n>  t/t7516-commit-races.sh     | 2 ++\n>  4 files changed, 5 insertions(+)\n"},{"id":"481012","messageId":"20230824205009.GA1516@coredump.intra.peff.net","threadId":"60152","inReplyTo":"cover.1692902414.git.me@ttaylorr.com","subject":"Re: [PATCH 0/3] leak tests: mark remaining tests leak-free as such","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-24T20:50:09Z","receivedAt":"2023-08-24T20:51:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 24, 2023 at 02:40:34PM -0400, Taylor Blau wrote:\n\n> While working on another topic that cleared up some leaks, I wanted to\n> see if any new tests became leak-free, so I ran:\n> \n>     $ make SANITIZE=leak\n>     $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test\n\nIs that exactly what you ran? Because I'd expect the second \"make\"\ninvocation to rebuild Git _without_ SANITIZE=leak enabled in that case.\n(Though I would have then expected most of the scripts to complain\nloudly about the mismatch; did you \"cd t\" in between the two?).\n\n>  t/t3321-notes-stripspace.sh | 1 +\n>  t/t5571-pre-push-hook.sh    | 1 +\n>  t/t5583-push-branches.sh    | 1 +\n>  t/t7516-commit-races.sh     | 2 ++\n>  4 files changed, 5 insertions(+)\n\nIf I run a single:\n\n  make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test\n\non v2.42.0, I get many hits. All of the ones you mentioned, plus:\n\n  t7408 t5407 t7008 t5811 t3407 t6001 t4058 t2016\n\nIf I run a few by hand, I _do_ see leaks in them, but the exit codes are\nhidden from the test suite (they are sub-programs of scripts, etc). I\nguess you also have:\n\n  GIT_TEST_SANITIZE_LEAK_LOG=true\n\nset, which should find those (and which you mention in your first\ncommit). Turning that on eliminates some of them, but I'm left with:\n\n  t5614 t5317 t5503\n\nnot in your list. Which is super weird, because t5614 is marked with\nTEST_PASSES_SANITIZE_LEAK. Hrm. And if I run it again, I get a\n_different_ set (t5614 again, along with your 4, but also t5303, t7701,\nand t4050). I wonder if we have a race in the leak-log code or\nsomething (I'm running under prove with -j32, naturally).\n\n> This series marks all leak-free tests as such, meaning that the above\n> \"make test\" invocation will pass after this series. The bulk of the\n> tests which are marked here in the first patch were always\n> leak-free[^1]. The remaining two patches address a couple of special\n> cases of tests which are also leak-free.\n\nHmm. If I check t5571, for example, by bisecting on:\n\n  make SANITIZE=leak && (cd t && ./t5571-pre-push-hook.sh -v -i)\n\nit shows that it was fixed by 861c56f6f9 (branch: fix a leak in\nsetup_tracking, 2023-06-11), which make sense. There are a bunch of leak\nfixes in the same series, which makes me wonder if they're responsible\nfor most of these.\n\nIf the leaks are gone, I am happy that we are marking them. But it is\nweird to me that we are getting different results.\n\n-Peff\n"},{"id":"481013","messageId":"20230824205456.GA2309362@coredump.intra.peff.net","threadId":"60152","inReplyTo":"20230824205009.GA1516@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] leak tests: mark remaining tests leak-free as such","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-24T20:54:56Z","receivedAt":"2023-08-24T20:56:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 24, 2023 at 04:50:10PM -0400, Jeff King wrote:\n\n> If I run a few by hand, I _do_ see leaks in them, but the exit codes are\n> hidden from the test suite (they are sub-programs of scripts, etc). I\n> guess you also have:\n> \n>   GIT_TEST_SANITIZE_LEAK_LOG=true\n> \n> set, which should find those (and which you mention in your first\n> commit). Turning that on eliminates some of them, but I'm left with:\n> \n>   t5614 t5317 t5503\n> \n> not in your list. Which is super weird, because t5614 is marked with\n> TEST_PASSES_SANITIZE_LEAK. Hrm. And if I run it again, I get a\n> _different_ set (t5614 again, along with your 4, but also t5303, t7701,\n> and t4050). I wonder if we have a race in the leak-log code or\n> something (I'm running under prove with -j32, naturally).\n\nArgh. It is this again:\n\n  https://lore.kernel.org/git/Yxl62zODF4oy1QL9@coredump.intra.peff.net/\n\nCan we revisit that patch? Included again below for reference.\n\n-- >8 --\nSubject: [PATCH] test-lib: ignore uninteresting LSan output\n\nWhen I run the tests in leak-checking mode the same way our CI job does,\nlike:\n\n  make SANITIZE=leak \\\n       GIT_TEST_PASSING_SANITIZE_LEAK=true \\\n       GIT_TEST_SANITIZE_LEAK_LOG=true \\\n       test\n\nthen LSan can racily produce useless entries in the log files that look\nlike this:\n\n  ==git==3034393==Unable to get registers from thread 3034307.\n\nI think they're mostly harmless based on the source here:\n\n  https://github.com/llvm/llvm-project/blob/7e0a52e8e9ef6394bb62e0b56e17fa23e7262411/compiler-rt/lib/lsan/lsan_common.cpp#L414\n\nwhich reads:\n\n    PtraceRegistersStatus have_registers =\n        suspended_threads.GetRegistersAndSP(i, &registers, &sp);\n    if (have_registers != REGISTERS_AVAILABLE) {\n      Report(\"Unable to get registers from thread %llu.\\n\", os_id);\n      // If unable to get SP, consider the entire stack to be reachable unless\n      // GetRegistersAndSP failed with ESRCH.\n      if (have_registers == REGISTERS_UNAVAILABLE_FATAL)\n        continue;\n      sp = stack_begin;\n    }\n\nThe program itself still runs fine and LSan doesn't cause us to abort.\nBut test-lib.sh looks for any non-empty LSan logs and marks the test as\na failure anyway, under the assumption that we simply missed the failing\nexit code somehow.\n\nI don't think I've ever seen this happen in the CI job, but running\nlocally using clang-14 on an 8-core machine, I can't seem to make it\nthrough a full run of the test suite without having at least one\nfailure. And it's a different one every time (though they do seem to\noften be related to packing tests, which makes sense, since that is one\nof our biggest users of threaded code).\n\nWe can hack around this by only counting LSan log files that contain a\nline that doesn't match our known-uninteresting pattern.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 293caf0f20..5ea5d1d62a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -334,6 +334,7 @@ nr_san_dir_leaks_ () {\n \tfind \"$TEST_RESULTS_SAN_DIR\" \\\n \t\t-type f \\\n \t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n+\txargs grep -lv \"Unable to get registers from thread\" |\n \twc -l\n }\n \n-- \n2.42.0.448.g0caf9a9e14\n\n"},{"id":"481019","messageId":"20230824210238.GA940724@coredump.intra.peff.net","threadId":"60152","inReplyTo":"b1711c4c817f95011bc477a9485c115b4926c7da.1692902414.git.me@ttaylorr.com","subject":"Re: [PATCH 1/3] leak tests: mark a handful of tests as leak-free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-24T21:02:38Z","receivedAt":"2023-08-24T21:03:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 24, 2023 at 02:40:38PM -0400, Taylor Blau wrote:\n\n> In the topic merged via 5a4f8381b6 (Merge branch\n> 'ab/mark-leak-free-tests', 2021-10-25), a handful of tests in the suite\n> were marked as leak-free.\n> \n> As far as I can tell, each patch from that series ran tests from a\n> handful of subject areas, such as \"some ls-files tests\", or \"all trace2\n> tests\". This left some gaps in which tests had and hadn't been audited\n> to be leak-free.\n> \n> This patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true\n> before sourcing t/test-lib.sh on most remaining leak-free tests. This\n> list was compiled by doing:\n> \n>     $ make SANITIZE=leak\n>     $ make \\\n>         GIT_TEST_PASSING_SANITIZE_LEAK=check \\\n>         GIT_TEST_SANITIZE_LEAK_LOG=true \\\n>         GIT_TEST_OPTS=-vi test\n\nSo having resolved my \"oops, lsan logs are racy\" problem, my system now\nagrees with yours on which tests are now leak-free. And we definitely\n_were_ leak free less than year ago when I posted that other patch. So\nI'm not sure I buy the \"these were missed in 5a4f8381b6\" logic.\n\nThe one in t5571, I mentioned earlier that I bisected to 861c56f6f9\n(branch: fix a leak in setup_tracking, 2023-06-11).\n\nThe one in t7516 seems to have been fixed by 866b43e644\n(do_read_index(): always mark index as initialized unless erroring out,\n2023-06-29).\n\nI found both by bisecting between v2.39.0 (which shows the leak) and\nv2.42.0 (which doesn't).\n\n-Peff\n"},{"id":"481034","messageId":"ZOj7XqRSmIFxaI3P@nand.local","threadId":"60152","inReplyTo":"20230824210238.GA940724@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] leak tests: mark a handful of tests as leak-free","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-25T19:05:02Z","receivedAt":"2023-08-25T19:05:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 24, 2023 at 05:02:38PM -0400, Jeff King wrote:\n> On Thu, Aug 24, 2023 at 02:40:38PM -0400, Taylor Blau wrote:\n>\n> > In the topic merged via 5a4f8381b6 (Merge branch\n> > 'ab/mark-leak-free-tests', 2021-10-25), a handful of tests in the suite\n> > were marked as leak-free.\n> >\n> > As far as I can tell, each patch from that series ran tests from a\n> > handful of subject areas, such as \"some ls-files tests\", or \"all trace2\n> > tests\". This left some gaps in which tests had and hadn't been audited\n> > to be leak-free.\n> >\n> > This patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true\n> > before sourcing t/test-lib.sh on most remaining leak-free tests. This\n> > list was compiled by doing:\n> >\n> >     $ make SANITIZE=leak\n> >     $ make \\\n> >         GIT_TEST_PASSING_SANITIZE_LEAK=check \\\n> >         GIT_TEST_SANITIZE_LEAK_LOG=true \\\n> >         GIT_TEST_OPTS=-vi test\n>\n> So having resolved my \"oops, lsan logs are racy\" problem, my system now\n> agrees with yours on which tests are now leak-free. And we definitely\n> _were_ leak free less than year ago when I posted that other patch. So\n> I'm not sure I buy the \"these were missed in 5a4f8381b6\" logic.\n\nPhew ;-). Thanks for chasing that down, I'm glad that our two sides\nagree.\n\n> The one in t5571, I mentioned earlier that I bisected to 861c56f6f9\n> (branch: fix a leak in setup_tracking, 2023-06-11).\n>\n> The one in t7516 seems to have been fixed by 866b43e644\n> (do_read_index(): always mark index as initialized unless erroring out,\n> 2023-06-29).\n>\n> I found both by bisecting between v2.39.0 (which shows the leak) and\n> v2.42.0 (which doesn't).\n\nMuch appreciated. I'm happy to fold those details into a new round if\nyou think they are useful enough to live in the commit history. I could\ngrab your patch as a preparatory step, too. But if you are happy with\nthis as-is, I am too.\n\nThanks,\nTaylor\n"},{"id":"481035","messageId":"ZOj8N4OS8zp7h9Vc@nand.local","threadId":"60152","inReplyTo":"20230824205009.GA1516@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] leak tests: mark remaining tests leak-free as such","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-25T19:08:39Z","receivedAt":"2023-08-25T19:11:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 24, 2023 at 04:50:09PM -0400, Jeff King wrote:\n> On Thu, Aug 24, 2023 at 02:40:34PM -0400, Taylor Blau wrote:\n>\n> > While working on another topic that cleared up some leaks, I wanted to\n> > see if any new tests became leak-free, so I ran:\n> >\n> >     $ make SANITIZE=leak\n> >     $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test\n>\n> Is that exactly what you ran? Because I'd expect the second \"make\"\n> invocation to rebuild Git _without_ SANITIZE=leak enabled in that case.\n> (Though I would have then expected most of the scripts to complain\n> loudly about the mismatch; did you \"cd t\" in between the two?).\n\nArgh. No, I wrote instead:\n\n  make SANITIZE=leak\n  make -C t GIT_TEST_PASSING_SANITIZE_LEAK=check ... test\n\n> >  t/t3321-notes-stripspace.sh | 1 +\n> >  t/t5571-pre-push-hook.sh    | 1 +\n> >  t/t5583-push-branches.sh    | 1 +\n> >  t/t7516-commit-races.sh     | 2 ++\n> >  4 files changed, 5 insertions(+)\n>\n> If I run a single:\n>\n>   make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test\n>\n> on v2.42.0, I get many hits. All of the ones you mentioned, plus:\n>\n>   t7408 t5407 t7008 t5811 t3407 t6001 t4058 t2016\n>\n> If I run a few by hand, I _do_ see leaks in them, but the exit codes are\n> hidden from the test suite (they are sub-programs of scripts, etc). I\n> guess you also have:\n>\n>   GIT_TEST_SANITIZE_LEAK_LOG=true\n\nYep, that is in the patch message, and definitely necessary (as you\nfound ;-)) to get accurate results here.\n\nThanks,\nTaylor\n"},{"id":"481036","messageId":"20230825203502.GB2382334@coredump.intra.peff.net","threadId":"60152","inReplyTo":"ZOj8N4OS8zp7h9Vc@nand.local","subject":"Re: [PATCH 0/3] leak tests: mark remaining tests leak-free as such","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-25T20:35:02Z","receivedAt":"2023-08-25T20:36:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 25, 2023 at 03:08:39PM -0400, Taylor Blau wrote:\n\n> On Thu, Aug 24, 2023 at 04:50:09PM -0400, Jeff King wrote:\n> > On Thu, Aug 24, 2023 at 02:40:34PM -0400, Taylor Blau wrote:\n> >\n> > > While working on another topic that cleared up some leaks, I wanted to\n> > > see if any new tests became leak-free, so I ran:\n> > >\n> > >     $ make SANITIZE=leak\n> > >     $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test\n> >\n> > Is that exactly what you ran? Because I'd expect the second \"make\"\n> > invocation to rebuild Git _without_ SANITIZE=leak enabled in that case.\n> > (Though I would have then expected most of the scripts to complain\n> > loudly about the mismatch; did you \"cd t\" in between the two?).\n> \n> Argh. No, I wrote instead:\n> \n>   make SANITIZE=leak\n>   make -C t GIT_TEST_PASSING_SANITIZE_LEAK=check ... test\n\nAh, that makes sense. The sample command in the commit message of the\nfirst patch has the same situation, I think.\n\n(I don't usually run \"make test\" from \"t\" myself because I prefer\n\"prove\", and an explicit \"make test\" skips the DEFAULT_TEST_TARGET\nmagic).\n\n-Peff\n"},{"id":"481037","messageId":"20230825203819.GC2382334@coredump.intra.peff.net","threadId":"60152","inReplyTo":"ZOj7XqRSmIFxaI3P@nand.local","subject":"Re: [PATCH 1/3] leak tests: mark a handful of tests as leak-free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-25T20:38:19Z","receivedAt":"2023-08-25T20:39:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 25, 2023 at 03:05:02PM -0400, Taylor Blau wrote:\n\n> > The one in t5571, I mentioned earlier that I bisected to 861c56f6f9\n> > (branch: fix a leak in setup_tracking, 2023-06-11).\n> >\n> > The one in t7516 seems to have been fixed by 866b43e644\n> > (do_read_index(): always mark index as initialized unless erroring out,\n> > 2023-06-29).\n> >\n> > I found both by bisecting between v2.39.0 (which shows the leak) and\n> > v2.42.0 (which doesn't).\n> \n> Much appreciated. I'm happy to fold those details into a new round if\n> you think they are useful enough to live in the commit history. I could\n> grab your patch as a preparatory step, too. But if you are happy with\n> this as-is, I am too.\n\nI would definitely mention them if writing the commit message from\nscratch. I'm not sure if it is worth a re-roll or not.\n\nI do think we should apply the racy-thread log fix, though. I thought we\nhad discussed it at the time, but there doesn't seem to be anything in\nthe archive. And I was willing to let it go as a weird one-off at the\ntime, but now that it wasted another 30 minutes of my life discovering\nthe problem again, I'm in favor of applying it.\n\nWhether it happens as part of your re-rolled series, or is applied\nseparately, I am OK either way. :)\n\n-Peff\n"},{"id":"481059","messageId":"xmqqttsjgh3h.fsf@gitster.g","threadId":"60152","inReplyTo":"20230825203819.GC2382334@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] leak tests: mark a handful of tests as leak-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-28T18:24:50Z","receivedAt":"2023-08-28T18:25:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I do think we should apply the racy-thread log fix, though. I thought we\n> had discussed it at the time, but there doesn't seem to be anything in\n> the archive. And I was willing to let it go as a weird one-off at the\n> time, but now that it wasted another 30 minutes of my life discovering\n> the problem again, I'm in favor of applying it.\n>\n> Whether it happens as part of your re-rolled series, or is applied\n> separately, I am OK either way. :)\n\nWhether it comes from you or Taylor, I do favor to see it as a new\nmessage at lore archive than having to fish an older message from it\n;-)\n\nThanks.\n"},{"id":"481060","messageId":"20230828183735.GA3015072@coredump.intra.peff.net","threadId":"60152","inReplyTo":"xmqqttsjgh3h.fsf@gitster.g","subject":"[PATCH] test-lib: ignore uninteresting LSan output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-28T18:37:35Z","receivedAt":"2023-08-28T18:38:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 28, 2023 at 11:24:50AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I do think we should apply the racy-thread log fix, though. I thought we\n> > had discussed it at the time, but there doesn't seem to be anything in\n> > the archive. And I was willing to let it go as a weird one-off at the\n> > time, but now that it wasted another 30 minutes of my life discovering\n> > the problem again, I'm in favor of applying it.\n> >\n> > Whether it happens as part of your re-rolled series, or is applied\n> > separately, I am OK either way. :)\n> \n> Whether it comes from you or Taylor, I do favor to see it as a new\n> message at lore archive than having to fish an older message from it\n> ;-)\n\nI re-sent it already as part of this thread. But I guess the definition\nof \"older\" may depend on whether you were paying attention to the thread\nat that point. ;)\n\nHere it is again. Let's just consider it as its own topic. There is\nnothing in Taylor's series that depends on it (it's just that it was\nuseful for me to reproduce his findings).\n\n-- >8 --\nSubject: [PATCH] test-lib: ignore uninteresting LSan output\n\nWhen I run the tests in leak-checking mode the same way our CI job does,\nlike:\n\n  make SANITIZE=leak \\\n       GIT_TEST_PASSING_SANITIZE_LEAK=true \\\n       GIT_TEST_SANITIZE_LEAK_LOG=true \\\n       test\n\nthen LSan can racily produce useless entries in the log files that look\nlike this:\n\n  ==git==3034393==Unable to get registers from thread 3034307.\n\nI think they're mostly harmless based on the source here:\n\n  https://github.com/llvm/llvm-project/blob/7e0a52e8e9ef6394bb62e0b56e17fa23e7262411/compiler-rt/lib/lsan/lsan_common.cpp#L414\n\nwhich reads:\n\n    PtraceRegistersStatus have_registers =\n        suspended_threads.GetRegistersAndSP(i, &registers, &sp);\n    if (have_registers != REGISTERS_AVAILABLE) {\n      Report(\"Unable to get registers from thread %llu.\\n\", os_id);\n      // If unable to get SP, consider the entire stack to be reachable unless\n      // GetRegistersAndSP failed with ESRCH.\n      if (have_registers == REGISTERS_UNAVAILABLE_FATAL)\n        continue;\n      sp = stack_begin;\n    }\n\nThe program itself still runs fine and LSan doesn't cause us to abort.\nBut test-lib.sh looks for any non-empty LSan logs and marks the test as\na failure anyway, under the assumption that we simply missed the failing\nexit code somehow.\n\nI don't think I've ever seen this happen in the CI job, but running\nlocally using clang-14 on an 8-core machine, I can't seem to make it\nthrough a full run of the test suite without having at least one\nfailure. And it's a different one every time (though they do seem to\noften be related to packing tests, which makes sense, since that is one\nof our biggest users of threaded code).\n\nWe can hack around this by only counting LSan log files that contain a\nline that doesn't match our known-uninteresting pattern.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 293caf0f20..5ea5d1d62a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -334,6 +334,7 @@ nr_san_dir_leaks_ () {\n \tfind \"$TEST_RESULTS_SAN_DIR\" \\\n \t\t-type f \\\n \t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n+\txargs grep -lv \"Unable to get registers from thread\" |\n \twc -l\n }\n \n-- \n2.42.0.448.g0caf9a9e14\n\n"},{"id":"481092","messageId":"164f37cade68104e7cae7613cad833eeeb87b4f1.1693263171.git.me@ttaylorr.com","threadId":"60152","inReplyTo":"cover.1693263171.git.me@ttaylorr.com","subject":"[PATCH v2 2/4] leak tests: mark a handful of tests as leak-free","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-28T22:52:59Z","receivedAt":"2023-08-28T22:53:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the topic merged via 5a4f8381b6 (Merge branch\n'ab/mark-leak-free-tests', 2021-10-25), a handful of tests in the suite\nwere marked as leak-free.\n\nSince then, a handful of tests have become leak-free due to changes like\n\n  - 861c56f6f9 (branch: fix a leak in setup_tracking, 2023-06-11), and\n  - 866b43e644 (do_read_index(): always mark index as initialized unless\n    erroring out, 2023-06-29)\n\n, but weren't updated at the time to mark themselves as such. This leads\nto test \"failures\" when running:\n\n    $ make SANITIZE=leak\n    $ make -C t \\\n        GIT_TEST_PASSING_SANITIZE_LEAK=check \\\n        GIT_TEST_SANITIZE_LEAK_LOG=true \\\n        GIT_TEST_OPTS=-vi test\n\nThis patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true\nbefore sourcing t/test-lib.sh on most remaining leak-free tests.\n\nThere are a couple of other tests which are similarly leak-free, but not\nincluded in the list of tests touched by this patch. The remaining tests\nwill be addressed in the subsequent two patches.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5571-pre-push-hook.sh | 1 +\n t/t7516-commit-races.sh  | 2 ++\n 2 files changed, 3 insertions(+)\n\ndiff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh\nindex a11b20e378..448134c4bf 100755\n--- a/t/t5571-pre-push-hook.sh\n+++ b/t/t5571-pre-push-hook.sh\n@@ -4,6 +4,7 @@ test_description='check pre-push hooks'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh\nindex 2d38a16480..bb95f09810 100755\n--- a/t/t7516-commit-races.sh\n+++ b/t/t7516-commit-races.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='git commit races'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'race to create orphan commit' '\n-- \n2.42.0.49.g03c54e21ee\n\n"},{"id":"481093","messageId":"cover.1693263171.git.me@ttaylorr.com","threadId":"60152","inReplyTo":"cover.1692902414.git.me@ttaylorr.com","subject":"[PATCH v2 0/4] leak tests: mark remaining tests leak-free as such","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-28T22:52:52Z","receivedAt":"2023-08-28T22:53:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a reroll of my (I guess now mine and Peff's!) series to update\nour test scripts to accurately mark which ones are leak-free.\n\nThis is mostly unchanged from the previous round, modulo cleaning up the\nfirst (now second) patch's message, and inserting a new patch from Peff\nat the beginning to ignore noisy LSan output.\n\nThanks in advance for your review!\n\nJeff King (1):\n  test-lib: ignore uninteresting LSan output\n\nTaylor Blau (3):\n  leak tests: mark a handful of tests as leak-free\n  leak tests: mark t3321-notes-stripspace.sh as leak-free\n  leak tests: mark t5583-push-branches.sh as leak-free\n\n t/t3321-notes-stripspace.sh | 1 +\n t/t5571-pre-push-hook.sh    | 1 +\n t/t5583-push-branches.sh    | 1 +\n t/t7516-commit-races.sh     | 2 ++\n t/test-lib.sh               | 1 +\n 5 files changed, 6 insertions(+)\n\nRange-diff against v1:\n-:  ---------- > 1:  7dd42212c0 test-lib: ignore uninteresting LSan output\n1:  b1711c4c81 ! 2:  164f37cade leak tests: mark a handful of tests as leak-free\n    @@ Commit message\n         'ab/mark-leak-free-tests', 2021-10-25), a handful of tests in the suite\n         were marked as leak-free.\n     \n    -    As far as I can tell, each patch from that series ran tests from a\n    -    handful of subject areas, such as \"some ls-files tests\", or \"all trace2\n    -    tests\". This left some gaps in which tests had and hadn't been audited\n    -    to be leak-free.\n    +    Since then, a handful of tests have become leak-free due to changes like\n     \n    -    This patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true\n    -    before sourcing t/test-lib.sh on most remaining leak-free tests. This\n    -    list was compiled by doing:\n    +      - 861c56f6f9 (branch: fix a leak in setup_tracking, 2023-06-11), and\n    +      - 866b43e644 (do_read_index(): always mark index as initialized unless\n    +        erroring out, 2023-06-29)\n    +\n    +    , but weren't updated at the time to mark themselves as such. This leads\n    +    to test \"failures\" when running:\n     \n             $ make SANITIZE=leak\n    -        $ make \\\n    +        $ make -C t \\\n                 GIT_TEST_PASSING_SANITIZE_LEAK=check \\\n                 GIT_TEST_SANITIZE_LEAK_LOG=true \\\n                 GIT_TEST_OPTS=-vi test\n     \n    -    and looking through the list of failing tests in the output.\n    +    This patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true\n    +    before sourcing t/test-lib.sh on most remaining leak-free tests.\n     \n         There are a couple of other tests which are similarly leak-free, but not\n         included in the list of tests touched by this patch. The remaining tests\n         will be addressed in the subsequent two patches.\n     \n    +    Helped-by: Jeff King <peff@peff.net>\n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n     \n      ## t/t5571-pre-push-hook.sh ##\n2:  cfeca88942 = 3:  116555fc02 leak tests: mark t3321-notes-stripspace.sh as leak-free\n3:  4011eb6a8b = 4:  a16a0b2cac leak tests: mark t5583-push-branches.sh as leak-free\n-- \n2.42.0.49.g03c54e21ee\n"},{"id":"481094","messageId":"a16a0b2cac0923febf8738b59299608f1d813f7a.1693263171.git.me@ttaylorr.com","threadId":"60152","inReplyTo":"cover.1693263171.git.me@ttaylorr.com","subject":"[PATCH v2 4/4] leak tests: mark t5583-push-branches.sh as leak-free","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-28T22:53:04Z","receivedAt":"2023-08-28T22:53:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When t5583-push-branches.sh was originally introduced via 425b4d7f47\n(push: introduce '--branches' option, 2023-05-06), it was not leak-free.\nIn fact, the test did not even run correctly until 022fbb655d (t5583:\nfix shebang line, 2023-05-12), but after applying that patch, we see a\nfailure at t5583.8:\n\n    ==2529087==ERROR: LeakSanitizer: detected memory leaks\n\n    Direct leak of 384 byte(s) in 1 object(s) allocated from:\n        #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98\n        #1 0x55e07606cbf9 in xrealloc wrapper.c:140\n        #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42\n        #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917\n        #4 0x55e075fe9cce in add_missing_tags remote.c:1518\n        #5 0x55e075fea1e4 in match_push_refs remote.c:1665\n        #6 0x55e076050a8e in transport_push transport.c:1378\n        #7 0x55e075e2eb74 in push_with_options builtin/push.c:401\n        #8 0x55e075e2edb0 in do_push builtin/push.c:458\n        #9 0x55e075e2ff7a in cmd_push builtin/push.c:702\n        #10 0x55e075d8aaf0 in run_builtin git.c:452\n        #11 0x55e075d8af08 in handle_builtin git.c:706\n        #12 0x55e075d8b12c in run_argv git.c:770\n        #13 0x55e075d8b6a0 in cmd_main git.c:905\n        #14 0x55e075e81f07 in main common-main.c:60\n        #15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n        #16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360\n        #17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453)\n\n    SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s).\n\nThis leak was addressed independently via 68b51172e3 (commit-reach: fix\nmemory leak in get_reachable_subset(), 2023-06-03), which makes t5583\nleak-free.\n\nBut t5583 was not in the tree when 68b51172e3 was written, and the two\nonly met after the latter was merged back in via 693bde461c (Merge\nbranch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20).\n\nAt that point, t5583 was leak-free. Let's mark it as such accordingly.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5583-push-branches.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t5583-push-branches.sh b/t/t5583-push-branches.sh\nindex e7e1b6dab6..320f49c753 100755\n--- a/t/t5583-push-branches.sh\n+++ b/t/t5583-push-branches.sh\n@@ -5,6 +5,7 @@ test_description='check the consisitency of behavior of --all and --branches'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n delete_refs() {\n-- \n2.42.0.49.g03c54e21ee\n"},{"id":"481095","messageId":"116555fc023553586ffbb88e1bdde43ca3b919a4.1693263171.git.me@ttaylorr.com","threadId":"60152","inReplyTo":"cover.1693263171.git.me@ttaylorr.com","subject":"[PATCH v2 3/4] leak tests: mark t3321-notes-stripspace.sh as leak-free","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-28T22:53:01Z","receivedAt":"2023-08-28T22:53:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This test was leak-free when t3321 was originally introduced, but never\nmarked as such:\n\n    $ rev=\"$(git log --format='%H' --reverse -1 HEAD^ -- t/t3321-notes-stripspace.sh)\"\n    $ git checkout $rev\n\n    $ make SANITIZE=leak\n    [...]\n\n    $ make -C t GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=--immediate t3321-notes-stripspace.sh\n    [...]\n    # passed all 27 test(s)\n    1..27\n    # faking up non-zero exit with --invert-exit-code\n    make: *** [Makefile:66: t3321-notes-stripspace.sh] Error 1\n    make: Leaving directory '/home/ttaylorr/src/git/t'\n\nMark this test as leak-free accordingly.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t3321-notes-stripspace.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t3321-notes-stripspace.sh b/t/t3321-notes-stripspace.sh\nindex 028d825e8f..36abdca5ee 100755\n--- a/t/t3321-notes-stripspace.sh\n+++ b/t/t3321-notes-stripspace.sh\n@@ -5,6 +5,7 @@\n \n test_description='Test commit notes with stripspace behavior'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n MULTI_LF=\"$LF$LF$LF\"\n-- \n2.42.0.49.g03c54e21ee\n\n"},{"id":"481096","messageId":"7dd42212c0b0debe40504cca474f2a5d9207155a.1693263171.git.me@ttaylorr.com","threadId":"60152","inReplyTo":"cover.1693263171.git.me@ttaylorr.com","subject":"[PATCH v2 1/4] test-lib: ignore uninteresting LSan output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-28T22:52:56Z","receivedAt":"2023-08-28T22:53:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nWhen I run the tests in leak-checking mode the same way our CI job does,\nlike:\n\n  make SANITIZE=leak \\\n       GIT_TEST_PASSING_SANITIZE_LEAK=true \\\n       GIT_TEST_SANITIZE_LEAK_LOG=true \\\n       test\n\nthen LSan can racily produce useless entries in the log files that look\nlike this:\n\n  ==git==3034393==Unable to get registers from thread 3034307.\n\nI think they're mostly harmless based on the source here:\n\n  https://github.com/llvm/llvm-project/blob/7e0a52e8e9ef6394bb62e0b56e17fa23e7262411/compiler-rt/lib/lsan/lsan_common.cpp#L414\n\nwhich reads:\n\n    PtraceRegistersStatus have_registers =\n        suspended_threads.GetRegistersAndSP(i, &registers, &sp);\n    if (have_registers != REGISTERS_AVAILABLE) {\n      Report(\"Unable to get registers from thread %llu.\\n\", os_id);\n      // If unable to get SP, consider the entire stack to be reachable unless\n      // GetRegistersAndSP failed with ESRCH.\n      if (have_registers == REGISTERS_UNAVAILABLE_FATAL)\n        continue;\n      sp = stack_begin;\n    }\n\nThe program itself still runs fine and LSan doesn't cause us to abort.\nBut test-lib.sh looks for any non-empty LSan logs and marks the test as\na failure anyway, under the assumption that we simply missed the failing\nexit code somehow.\n\nI don't think I've ever seen this happen in the CI job, but running\nlocally using clang-14 on an 8-core machine, I can't seem to make it\nthrough a full run of the test suite without having at least one\nfailure. And it's a different one every time (though they do seem to\noften be related to packing tests, which makes sense, since that is one\nof our biggest users of threaded code).\n\nWe can hack around this by only counting LSan log files that contain a\nline that doesn't match our known-uninteresting pattern.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 293caf0f20..5ea5d1d62a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -334,6 +334,7 @@ nr_san_dir_leaks_ () {\n \tfind \"$TEST_RESULTS_SAN_DIR\" \\\n \t\t-type f \\\n \t\t-name \"$TEST_RESULTS_SAN_FILE_PFX.*\" 2>/dev/null |\n+\txargs grep -lv \"Unable to get registers from thread\" |\n \twc -l\n }\n \n-- \n2.42.0.49.g03c54e21ee\n\n"},{"id":"481107","messageId":"20230829010000.GC3876652@coredump.intra.peff.net","threadId":"60152","inReplyTo":"cover.1693263171.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/4] leak tests: mark remaining tests leak-free as such","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-29T01:00:00Z","receivedAt":"2023-08-29T01:00:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 28, 2023 at 06:52:52PM -0400, Taylor Blau wrote:\n\n> Here is a reroll of my (I guess now mine and Peff's!) series to update\n> our test scripts to accurately mark which ones are leak-free.\n> \n> This is mostly unchanged from the previous round, modulo cleaning up the\n> first (now second) patch's message, and inserting a new patch from Peff\n> at the beginning to ignore noisy LSan output.\n\nPerhaps needless to say, but this all looks good to me. :)\n\n-Peff\n"},{"id":"481119","messageId":"xmqq4jkhg5oz.fsf@gitster.g","threadId":"60152","inReplyTo":"20230829010000.GC3876652@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/4] leak tests: mark remaining tests leak-free as such","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-29T16:43:24Z","receivedAt":"2023-08-29T16:44:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Aug 28, 2023 at 06:52:52PM -0400, Taylor Blau wrote:\n>\n>> Here is a reroll of my (I guess now mine and Peff's!) series to update\n>> our test scripts to accurately mark which ones are leak-free.\n>> \n>> This is mostly unchanged from the previous round, modulo cleaning up the\n>> first (now second) patch's message, and inserting a new patch from Peff\n>> at the beginning to ignore noisy LSan output.\n>\n> Perhaps needless to say, but this all looks good to me. :)\n\nThanks, both of you.  These look good to me, too ;-).\n"}]}