On Tue, Oct 26 2021, Taylor Blau wrote: > On Fri, Oct 22, 2021 at 12:32:17PM +0200, Ævar Arnfjörð Bjarmason wrote: >> Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add >> a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally >> be able to add a "suppressions" file to the $LSAN_OPTIONS. >> >> This allows for marking tests as passing with >> "TEST_PASSES_SANITIZE_LEAK=true" when they still have failure due more >> general widespread memory leaks, such as from the "git log" family of >> commands. We can now mark the "git -C" tests as passing. >> >> For getting specific tests to pass this is preferable to using >> UNLEAK() in these codepaths, as I'll have fixes for those leaks soon, >> and being able to atomically mark relevant tests as passing with >> "TEST_PASSES_SANITIZE_LEAK=true" helps to explain those changes. See >> [1] for more details. >> >> 1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@xxxxxxxxxxxxxxxxxxx/ >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> >> --- >> >> On Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote: >> >> > On Fri, Oct 22 2021, Taylor Blau wrote: >> > >> >> On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote: >> >>> >> >>> On Wed, Oct 20 2021, Taylor Blau wrote: >> [...] >> > If you want to pick that approach up and run with it I think it would >> > probably make sense to factor that suggested test_expect_success out >> > into a function in test-lib-functions.sh or whatever, and call it as >> > e.g.: >> > >> > TEST_PASSES_SANITIZE_LEAK=true >> > . ./test-lib.sh >> > declare_known_leaks <<-\EOF >> > add_rev_cmdline >> > [...] >> > EOF >> > >> > Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for >> > you. >> > >> > Doing it that way would be less boilerplate for each test that wants it, >> > and is also more likely to work with other non-LSAN leak appoaches, >> > i.e. as long as something can take a list of lines matching stack traces >> > we can feed that to that leak checker's idea of a whitelist. >> >> I just went ahead and hacked that up. If you're OK with that approach >> it would really help reduce the work for leak changes I've got >> planned, and as noted gives you the end-state of a passing 5319. >> >> I don't know if it makes more sense for you to base on top of this >> if/when Junio picks it up, or to integrate it into your series >> etc. Maybe Junio will chime in ... > > Hmm. This seems neat, but I haven't been able to get it to work without > encountering a rather annoying bug along the way. > > Here is the preamble of my modified t5319 to include all of the leaks I > found in the previous round (but decided not to fix): > > TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > todo_leaks <<-\EOF > ^add_rev_cmdline$ > ^cmd_log_init_finish$ > ^prepare_revision_walk$ > ^prep_parse_options$ > EOF > > So running that when git is compiled with SANITIZE=leak *should* work, > but instead produces this rather annoying leak detection after t5319.7: > > ================================================================= > ==1785298==ERROR: LeakSanitizer: detected memory leaks > > Indirect leak of 41 byte(s) in 3 object(s) allocated from: > #0 0x7f2f2f866db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54 > #1 0x7f2f2f64b4aa in __GI___strdup string/strdup.c:42 > > ----------------------------------------------------- > Suppressions used: > count bytes template > 1 576 ^add_rev_cmdline$ > ----------------------------------------------------- > > SUMMARY: LeakSanitizer: 41 byte(s) leaked in 3 allocation(s). > Aborted > > Huh? Looking past __GI___strdup (which I assume is just a > symbolification failure on ASan's part), who calls strdup()? That trace > is annoyingly incomplete, and doesn't really give us anything to go off > of. > > It does seem to remind me of this: > > https://github.com/google/sanitizers/issues/148 > > though that issue goes in so many different directions that I'm not sure > whether it really is the same issue or not. In any case, that leak > *still* shows up even when suppressing xmalloc() and xrealloc(), so I > almost think that it's a leak within ASan itself. > > But most interesting is that those calls go away when I stop setting > `suppressions` in $LSAN_OPTIONS by dropping the call to your todo_leaks. > > So this all feels like a bug in ASan to me. I'm curious if it works on > your system, but in the meantime I think the best path forward is to > drop the last patch of my original series (the one with the three > UNLEAK() calls) and to avoid relying on this patch for the time being. There are similar cases where LSAN doesn't provide as meaningful of a stacktrace as valgrind, sometimes when tracing leaks I get a relatively bad stacktrace like that ending in some __GI___*<stdlib-name> function. I'll usually compile without SANITIZE=leak and just run a slower: valgrind --leak-check=full --show-leak-kinds=all In this case however it's not a bug or bad leak tracing, but an artifact of us using these stacktrace exclusions. If you run this manually you'll see: $ ~/g/git/git -c core.multiPackIndex=false rev-list --objects --all cd0747a9352b58d112f0010134351efc7bbad4a6 [... snipped output ...] ================================================================= ==58023==ERROR: LeakSanitizer: detected memory leaks Direct leak of 576 byte(s) in 1 object(s) allocated from: #0 0x7fd0bfae50c5 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:82 #1 0x5637b3bd9163 in xrealloc /home/avar/g/git/wrapper.c:126 #2 0x5637b3b6a1e4 in add_rev_cmdline /home/avar/g/git/revision.c:1482 #3 0x5637b3b6a412 in handle_one_ref /home/avar/g/git/revision.c:1534 #4 0x5637b3b49c4e in do_for_each_ref_helper /home/avar/g/git/refs.c:1483 #5 0x5637b3b54afc in do_for_each_repo_ref_iterator refs/iterator.c:418 #6 0x5637b3b49cc8 in do_for_each_ref /home/avar/g/git/refs.c:1498 #7 0x5637b3b49d07 in refs_for_each_ref /home/avar/g/git/refs.c:1504 #8 0x5637b3b6a563 in handle_refs /home/avar/g/git/revision.c:1578 #9 0x5637b3b6e0e7 in handle_revision_pseudo_opt /home/avar/g/git/revision.c:2597 #10 0x5637b3b6e9d5 in setup_revisions /home/avar/g/git/revision.c:2738 #11 0x5637b39ebc58 in cmd_rev_list builtin/rev-list.c:550 #12 0x5637b3932a89 in run_builtin /home/avar/g/git/git.c:461 #13 0x5637b3932e98 in handle_builtin /home/avar/g/git/git.c:713 #14 0x5637b3933105 in run_argv /home/avar/g/git/git.c:780 #15 0x5637b39335ae in cmd_main /home/avar/g/git/git.c:911 #16 0x5637b3a1a898 in main /home/avar/g/git/common-main.c:52 #17 0x7fd0bf860d09 in __libc_start_main ../csu/libc-start.c:308 Indirect leak of 41 byte(s) in 3 object(s) allocated from: #0 0x7fd0bfae4db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54 #1 0x7fd0bf8c7e4a in __GI___strdup string/strdup.c:42 SUMMARY: LeakSanitizer: 617 byte(s) leaked in 4 allocation(s). Which is why I put the "strdup" there, but forgot to tell you when I submitted the real PATCH version that that one shouldn't have the ^$ anchoring. FWIW this will work on top of your series: diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index a3c72b68f7c..891de720b2c 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -2,6 +2,17 @@ test_description='multi-pack-indexes' . ./test-lib.sh +todo_leaks <<-\EOF +^add_rev_cmdline$ +^cmd_log_init_finish$ +^prep_parse_options$ +^prepare_revision_walk$ +^start_progress_delay$ + +# An indirect leak reported because we excluded the leaks above +strdup$ +EOF + GIT_TEST_MULTI_PACK_INDEX=0 objdir=.git/objects
