| Message ID | cover.1692902414.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | leak tests: mark remaining tests leak-free as such | expand |
Taylor Blau <me@ttaylorr.com> writes: > This series marks all leak-free tests as such, meaning that the above > "make test" invocation will pass after this series. The bulk of the > tests which are marked here in the first patch were always > leak-free[^1]. The remaining two patches address a couple of special > cases of tests which are also leak-free. > > Thanks in advance for your review! > > [^1]: At least as far back as v2.38.0, when the "check" mode of > GIT_TEST_PASSING_SANITIZE_LEAK was first introduced. Nice to see "bugs" gone without any code changes ;-) It would have taken a lot of patience from your part, though, and your effort is very much appreciated. > Taylor Blau (3): > leak tests: mark a handful of tests as leak-free > leak tests: mark t3321-notes-stripspace.sh as leak-free > leak tests: mark t5583-push-branches.sh as leak-free > > t/t3321-notes-stripspace.sh | 1 + > t/t5571-pre-push-hook.sh | 1 + > t/t5583-push-branches.sh | 1 + > t/t7516-commit-races.sh | 2 ++ > 4 files changed, 5 insertions(+)
On Thu, Aug 24, 2023 at 02:40:34PM -0400, Taylor Blau wrote: > While working on another topic that cleared up some leaks, I wanted to > see if any new tests became leak-free, so I ran: > > $ make SANITIZE=leak > $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test Is that exactly what you ran? Because I'd expect the second "make" invocation to rebuild Git _without_ SANITIZE=leak enabled in that case. (Though I would have then expected most of the scripts to complain loudly about the mismatch; did you "cd t" in between the two?). > t/t3321-notes-stripspace.sh | 1 + > t/t5571-pre-push-hook.sh | 1 + > t/t5583-push-branches.sh | 1 + > t/t7516-commit-races.sh | 2 ++ > 4 files changed, 5 insertions(+) If I run a single: make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test on v2.42.0, I get many hits. All of the ones you mentioned, plus: t7408 t5407 t7008 t5811 t3407 t6001 t4058 t2016 If I run a few by hand, I _do_ see leaks in them, but the exit codes are hidden from the test suite (they are sub-programs of scripts, etc). I guess you also have: GIT_TEST_SANITIZE_LEAK_LOG=true set, which should find those (and which you mention in your first commit). Turning that on eliminates some of them, but I'm left with: t5614 t5317 t5503 not in your list. Which is super weird, because t5614 is marked with TEST_PASSES_SANITIZE_LEAK. Hrm. And if I run it again, I get a _different_ set (t5614 again, along with your 4, but also t5303, t7701, and t4050). I wonder if we have a race in the leak-log code or something (I'm running under prove with -j32, naturally). > This series marks all leak-free tests as such, meaning that the above > "make test" invocation will pass after this series. The bulk of the > tests which are marked here in the first patch were always > leak-free[^1]. The remaining two patches address a couple of special > cases of tests which are also leak-free. Hmm. If I check t5571, for example, by bisecting on: make SANITIZE=leak && (cd t && ./t5571-pre-push-hook.sh -v -i) it shows that it was fixed by 861c56f6f9 (branch: fix a leak in setup_tracking, 2023-06-11), which make sense. There are a bunch of leak fixes in the same series, which makes me wonder if they're responsible for most of these. If the leaks are gone, I am happy that we are marking them. But it is weird to me that we are getting different results. -Peff
On Thu, Aug 24, 2023 at 04:50:10PM -0400, Jeff King wrote: > If I run a few by hand, I _do_ see leaks in them, but the exit codes are > hidden from the test suite (they are sub-programs of scripts, etc). I > guess you also have: > > GIT_TEST_SANITIZE_LEAK_LOG=true > > set, which should find those (and which you mention in your first > commit). Turning that on eliminates some of them, but I'm left with: > > t5614 t5317 t5503 > > not in your list. Which is super weird, because t5614 is marked with > TEST_PASSES_SANITIZE_LEAK. Hrm. And if I run it again, I get a > _different_ set (t5614 again, along with your 4, but also t5303, t7701, > and t4050). I wonder if we have a race in the leak-log code or > something (I'm running under prove with -j32, naturally). Argh. It is this again: https://lore.kernel.org/git/Yxl62zODF4oy1QL9@coredump.intra.peff.net/ Can we revisit that patch? Included again below for reference. -- >8 -- Subject: [PATCH] test-lib: ignore uninteresting LSan output When I run the tests in leak-checking mode the same way our CI job does, like: make SANITIZE=leak \ GIT_TEST_PASSING_SANITIZE_LEAK=true \ GIT_TEST_SANITIZE_LEAK_LOG=true \ test then LSan can racily produce useless entries in the log files that look like this: ==git==3034393==Unable to get registers from thread 3034307. I think they're mostly harmless based on the source here: https://github.com/llvm/llvm-project/blob/7e0a52e8e9ef6394bb62e0b56e17fa23e7262411/compiler-rt/lib/lsan/lsan_common.cpp#L414 which reads: PtraceRegistersStatus have_registers = suspended_threads.GetRegistersAndSP(i, ®isters, &sp); if (have_registers != REGISTERS_AVAILABLE) { Report("Unable to get registers from thread %llu.\n", os_id); // If unable to get SP, consider the entire stack to be reachable unless // GetRegistersAndSP failed with ESRCH. if (have_registers == REGISTERS_UNAVAILABLE_FATAL) continue; sp = stack_begin; } The program itself still runs fine and LSan doesn't cause us to abort. But test-lib.sh looks for any non-empty LSan logs and marks the test as a failure anyway, under the assumption that we simply missed the failing exit code somehow. I don't think I've ever seen this happen in the CI job, but running locally using clang-14 on an 8-core machine, I can't seem to make it through a full run of the test suite without having at least one failure. And it's a different one every time (though they do seem to often be related to packing tests, which makes sense, since that is one of our biggest users of threaded code). We can hack around this by only counting LSan log files that contain a line that doesn't match our known-uninteresting pattern. Signed-off-by: Jeff King <peff@peff.net> --- t/test-lib.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/test-lib.sh b/t/test-lib.sh index 293caf0f20..5ea5d1d62a 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -334,6 +334,7 @@ nr_san_dir_leaks_ () { find "$TEST_RESULTS_SAN_DIR" \ -type f \ -name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null | + xargs grep -lv "Unable to get registers from thread" | wc -l }
On Thu, Aug 24, 2023 at 04:50:09PM -0400, Jeff King wrote: > On Thu, Aug 24, 2023 at 02:40:34PM -0400, Taylor Blau wrote: > > > While working on another topic that cleared up some leaks, I wanted to > > see if any new tests became leak-free, so I ran: > > > > $ make SANITIZE=leak > > $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test > > Is that exactly what you ran? Because I'd expect the second "make" > invocation to rebuild Git _without_ SANITIZE=leak enabled in that case. > (Though I would have then expected most of the scripts to complain > loudly about the mismatch; did you "cd t" in between the two?). Argh. No, I wrote instead: make SANITIZE=leak make -C t GIT_TEST_PASSING_SANITIZE_LEAK=check ... test > > t/t3321-notes-stripspace.sh | 1 + > > t/t5571-pre-push-hook.sh | 1 + > > t/t5583-push-branches.sh | 1 + > > t/t7516-commit-races.sh | 2 ++ > > 4 files changed, 5 insertions(+) > > If I run a single: > > make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test > > on v2.42.0, I get many hits. All of the ones you mentioned, plus: > > t7408 t5407 t7008 t5811 t3407 t6001 t4058 t2016 > > If I run a few by hand, I _do_ see leaks in them, but the exit codes are > hidden from the test suite (they are sub-programs of scripts, etc). I > guess you also have: > > GIT_TEST_SANITIZE_LEAK_LOG=true Yep, that is in the patch message, and definitely necessary (as you found ;-)) to get accurate results here. Thanks, Taylor
On Fri, Aug 25, 2023 at 03:08:39PM -0400, Taylor Blau wrote: > On Thu, Aug 24, 2023 at 04:50:09PM -0400, Jeff King wrote: > > On Thu, Aug 24, 2023 at 02:40:34PM -0400, Taylor Blau wrote: > > > > > While working on another topic that cleared up some leaks, I wanted to > > > see if any new tests became leak-free, so I ran: > > > > > > $ make SANITIZE=leak > > > $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test > > > > Is that exactly what you ran? Because I'd expect the second "make" > > invocation to rebuild Git _without_ SANITIZE=leak enabled in that case. > > (Though I would have then expected most of the scripts to complain > > loudly about the mismatch; did you "cd t" in between the two?). > > Argh. No, I wrote instead: > > make SANITIZE=leak > make -C t GIT_TEST_PASSING_SANITIZE_LEAK=check ... test Ah, that makes sense. The sample command in the commit message of the first patch has the same situation, I think. (I don't usually run "make test" from "t" myself because I prefer "prove", and an explicit "make test" skips the DEFAULT_TEST_TARGET magic). -Peff
While working on another topic that cleared up some leaks, I wanted to see if any new tests became leak-free, so I ran: $ make SANITIZE=leak $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test , and was surprised to see so many tests appear in the "failed" list (indicating that they are free of leaks, but did not mark themselves as such). In fact, the patch I had written at the time didn't make a dent one way or another in the list of leak-free tests, as the same results were produced on 'master' without my changes. This series marks all leak-free tests as such, meaning that the above "make test" invocation will pass after this series. The bulk of the tests which are marked here in the first patch were always leak-free[^1]. The remaining two patches address a couple of special cases of tests which are also leak-free. Thanks in advance for your review! [^1]: At least as far back as v2.38.0, when the "check" mode of GIT_TEST_PASSING_SANITIZE_LEAK was first introduced. Taylor Blau (3): leak tests: mark a handful of tests as leak-free leak tests: mark t3321-notes-stripspace.sh as leak-free leak tests: mark t5583-push-branches.sh as leak-free t/t3321-notes-stripspace.sh | 1 + t/t5571-pre-push-hook.sh | 1 + t/t5583-push-branches.sh | 1 + t/t7516-commit-races.sh | 2 ++ 4 files changed, 5 insertions(+)