From 495ab709fd91d8238e49b3b476c995c64cd5b2fa Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 7 Jan 2025 02:05:01 -0500 Subject: [PATCH 1/3] test-lib: invert return value of check_test_results_san_file_empty We have a function to check whether LSan logged any leaks. It returns success for no leaks, and non-zero otherwise. This is the simplest thing for its callers, who want to say "if no leaks then return early". But because it's implemented as a shell pipeline, you end up with the awkward: ! find ... | xargs grep leaks | grep -v false-positives where the "!" is actually negating the final grep. Switch the return value (and name) to return success when there are leaks. This should make the code a little easier to read, and the negation in the callers still reads pretty naturally. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/test-lib-functions.sh | 2 +- t/test-lib.sh | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 78e054ab50..c25cee0ad8 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -927,7 +927,7 @@ test_expect_success () { test -n "$test_skip_test_preamble" || say >&3 "expecting success of $TEST_NUMBER.$test_count '$1': $test_body" if test_run_ "$test_body" && - check_test_results_san_file_empty_ + ! check_test_results_san_file_has_entries_ then test_ok_ "$1" else diff --git a/t/test-lib.sh b/t/test-lib.sh index d1f62adbf8..be3553e40e 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1169,12 +1169,12 @@ test_atexit_handler () { teardown_malloc_check } -check_test_results_san_file_empty_ () { - test -z "$TEST_RESULTS_SAN_FILE" && return 0 +check_test_results_san_file_has_entries_ () { + test -z "$TEST_RESULTS_SAN_FILE" && return 1 # stderr piped to /dev/null because the directory may have # been "rmdir"'d already. - ! find "$TEST_RESULTS_SAN_DIR" \ + find "$TEST_RESULTS_SAN_DIR" \ -type f \ -name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null | xargs grep ^DEDUP_TOKEN | @@ -1182,7 +1182,7 @@ check_test_results_san_file_empty_ () { } check_test_results_san_file_ () { - if check_test_results_san_file_empty_ + if ! check_test_results_san_file_has_entries_ then return fi && From 75dde9545a434a16d447b5f10b09b08bb7175cba Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 7 Jan 2025 02:07:52 -0500 Subject: [PATCH 2/3] test-lib: simplify lsan results check We want to know if there are any leaks logged by LSan in the results directory, so we run "find" on the containing directory and pipe it to xargs. We can accomplish the same thing by just globbing in the shell and passing the result to grep, which has a few advantages: - it's one fewer process to run - we can glob on the TEST_RESULTS_SAN_FILE pattern, which is what we checked at the beginning of the function, and is the same glob used to show the logs in check_test_results_san_file_ - this correctly handles the case where TEST_OUTPUT_DIRECTORY has a space in it. For example doing: mkdir "/tmp/foo bar" TEST_OUTPUT_DIRECTORY="/tmp/foo bar" make SANITIZE=leak test would yield a lot of: grep: /tmp/foo: No such file or directory grep: bar/test-results/t0006-date.leak/trace.test-tool.582311: No such file or directory when there are leaks. We could do the same thing with "xargs --null", but that isn't portable. We are now subject to command-line length limits, but that is also true of the globbing cat used to show the logs themselves. This hasn't been a problem in practice. We do need to use "grep -s" for the case that the glob does not expand (i.e., there are not any log files at all). This option is in POSIX, and has been used in t7407 for several years without anybody complaining. This also also naturally handles the case where the surrounding directory has already been removed (in which case there are likewise no files!), dropping the need to comment about it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/test-lib.sh | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index be3553e40e..898c2267b8 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1172,12 +1172,7 @@ test_atexit_handler () { check_test_results_san_file_has_entries_ () { test -z "$TEST_RESULTS_SAN_FILE" && return 1 - # stderr piped to /dev/null because the directory may have - # been "rmdir"'d already. - find "$TEST_RESULTS_SAN_DIR" \ - -type f \ - -name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null | - xargs grep ^DEDUP_TOKEN | + grep -s ^DEDUP_TOKEN "$TEST_RESULTS_SAN_FILE".* | grep -qv sanitizer::GetThreadStackTopAndBottom } From ec65acf4c5dcb743f9b5faf28049d87c1aa8f90b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 7 Jan 2025 02:08:31 -0500 Subject: [PATCH 3/3] test-lib: add a few comments to LSan log checking Commit b119a687d4 (test-lib: ignore leaks in the sanitizer's thread code, 2025-01-01) added code to suppress a false positive in the leak checker. But if you're just reading the code, the obscure grep call is a bit of a head-scratcher. Let's add a brief comment explaining what's going on (and anybody digging further can find this commit or that one for all the details). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/test-lib.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/t/test-lib.sh b/t/test-lib.sh index 898c2267b8..9f27a49995 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1172,6 +1172,11 @@ test_atexit_handler () { check_test_results_san_file_has_entries_ () { test -z "$TEST_RESULTS_SAN_FILE" && return 1 + # Lines marked with DEDUP_TOKEN show unique leaks. We only care that we + # found at least one. + # + # But also suppress any false positives caused by bugs or races in the + # sanitizer itself. grep -s ^DEDUP_TOKEN "$TEST_RESULTS_SAN_FILE".* | grep -qv sanitizer::GetThreadStackTopAndBottom }