| Message ID | 20250101201721.GD3305462@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 373a4326961c504ad6365fc1e4a9082e387499c7 |
| Headers | show |
| Series | [1/6] test-lib: use individual lsan dir for --stress runs | expand |
On Wed, Jan 01, 2025 at 03:17:21PM -0500, Jeff King wrote: > @@ -1181,8 +1170,14 @@ test_atexit_handler () { > } > > check_test_results_san_file_empty_ () { > - test -z "$TEST_RESULTS_SAN_FILE" || > - test "$(nr_san_dir_leaks_)" = 0 > + test -z "$TEST_RESULTS_SAN_FILE" && return 0 > + > + # 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 -qv "Unable to get registers from thread" Can't we use `-exec grep -qv "Unable to get registers from thread" {} \+` instead of using xargs? Or is that unportable? Might make it a bit easier to reason about the `!` in the presence of a pipe. Patrick
On Fri, Jan 03, 2025 at 01:05:45PM +0100, Patrick Steinhardt wrote: > On Wed, Jan 01, 2025 at 03:17:21PM -0500, Jeff King wrote: > > @@ -1181,8 +1170,14 @@ test_atexit_handler () { > > } > > > > check_test_results_san_file_empty_ () { > > - test -z "$TEST_RESULTS_SAN_FILE" || > > - test "$(nr_san_dir_leaks_)" = 0 > > + test -z "$TEST_RESULTS_SAN_FILE" && return 0 > > + > > + # 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 -qv "Unable to get registers from thread" > > Can't we use `-exec grep -qv "Unable to get registers from thread" {} > \+` instead of using xargs? Or is that unportable? Might make it a bit > easier to reason about the `!` in the presence of a pipe. I don't think that saves us from negating, though. The "grep" will tell us if it matched any "real" lines, but we want to report that we found no real lines. Plus I don't think "find" propagates the exit code from -exec anyway. I think you can check the exit status with more find logic, so you'd then use a conditional -print for each file like: find ... \ -exec grep -qv "Unable to get registers from thread" \{} \; \ -print and you have to check whether the output is empty. The easiest way to do that is with another grep! Which also needs negated. ;) I think if we really want to drop the negation, we'd be best to flip the function's return, like: have_leaks() { # not leak-checking test -z "$TEST_RESULTS_SAN_FILE" && return 1 find "$TEST_RESULTS_SAN_DIR" \ -type f \ -name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null | xargs grep ^DEDUP_TOKEN | grep -qv sanitizer::GetThreadStackTopAndBottom } And then you could switch the initial "grep" to -exec if you want, but there's no negation to get rid of, so it is only a preference of -exec versus xargs. -Peff
On Fri, Jan 03, 2025 at 03:24:10PM -0500, Jeff King wrote: > On Fri, Jan 03, 2025 at 01:05:45PM +0100, Patrick Steinhardt wrote: > > > On Wed, Jan 01, 2025 at 03:17:21PM -0500, Jeff King wrote: > > > @@ -1181,8 +1170,14 @@ test_atexit_handler () { > > > } > > > > > > check_test_results_san_file_empty_ () { > > > - test -z "$TEST_RESULTS_SAN_FILE" || > > > - test "$(nr_san_dir_leaks_)" = 0 > > > + test -z "$TEST_RESULTS_SAN_FILE" && return 0 > > > + > > > + # 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 -qv "Unable to get registers from thread" > > > > Can't we use `-exec grep -qv "Unable to get registers from thread" {} > > \+` instead of using xargs? Or is that unportable? Might make it a bit > > easier to reason about the `!` in the presence of a pipe. > > I don't think that saves us from negating, though. The "grep" will tell > us if it matched any "real" lines, but we want to report that we found > no real lines. > > Plus I don't think "find" propagates the exit code from -exec anyway. I > think you can check the exit status with more find logic, so you'd then > use a conditional -print for each file like: It should. Quoting find(1): If any invocation with the `+' form returns a non-zero value as exit status, then find returns a non-zero exit status. > find ... \ > -exec grep -qv "Unable to get registers from thread" \{} \; \ > -print > > and you have to check whether the output is empty. The easiest way to do > that is with another grep! Which also needs negated. ;) Yup, I didn't mean to say that we can drop the negation, but that it makes it easier to reason about what the negation applies to (the whole pipe or just the find(1) command)). > I think if we really want to drop the negation, we'd be best to flip the > function's return, like: > > have_leaks() { > # not leak-checking > test -z "$TEST_RESULTS_SAN_FILE" && return 1 > > find "$TEST_RESULTS_SAN_DIR" \ > -type f \ > -name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null | > xargs grep ^DEDUP_TOKEN | > grep -qv sanitizer::GetThreadStackTopAndBottom > } > > And then you could switch the initial "grep" to -exec if you want, but > there's no negation to get rid of, so it is only a preference of -exec > versus xargs. Yup. Patrick
On Mon, Jan 06, 2025 at 08:56:57AM +0100, Patrick Steinhardt wrote: > > Plus I don't think "find" propagates the exit code from -exec anyway. I > > think you can check the exit status with more find logic, so you'd then > > use a conditional -print for each file like: > > It should. Quoting find(1): > > If any invocation with the `+' form returns a non-zero value as exit > status, then find returns a non-zero exit status. Ah, right. I tried using ';' to look at individual files, and it does ignore the code. But of course we don't need to know which logs had leaks, only that there was at least one. I think we can make it even simpler, though. I'll post patches in a moment. -Peff
diff --git a/t/test-lib.sh b/t/test-lib.sh index dd2ba6e6cc..23eb26bfbe 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -340,17 +340,6 @@ case "$TRASH_DIRECTORY" in *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;; esac -# Utility functions using $TEST_RESULTS_* variables -nr_san_dir_leaks_ () { - # 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 -lv "Unable to get registers from thread" | - wc -l -} - # If --stress was passed, run this test repeatedly in several parallel loops. if test "$GIT_TEST_STRESS_STARTED" = "done" then @@ -1181,8 +1170,14 @@ test_atexit_handler () { } check_test_results_san_file_empty_ () { - test -z "$TEST_RESULTS_SAN_FILE" || - test "$(nr_san_dir_leaks_)" = 0 + test -z "$TEST_RESULTS_SAN_FILE" && return 0 + + # 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 -qv "Unable to get registers from thread" } check_test_results_san_file_ () {
We have a function to count the number of leaks found (actually, it is the number of processes which produced a log file). Once upon a time we cared about seeing if this number increased between runs. But we simplified that away in 95c679ad86 (test-lib: stop showing old leak logs, 2024-09-24), and now we only care if it returns any results or not. In preparation for refactoring it further, let's drop the counting function entirely, and roll it into the "is it empty" check. The outcome should be the same, but we'll be free to return a boolean "did we find anything" without worrying about somebody adding a new call to the counting function. Signed-off-by: Jeff King <peff@peff.net> --- We need the extra "!" to invert the exit code of the final grep, which made my head spin a little at first. Maybe it would be less confusing if this was reporting non-empty results, as "check_test_results_has_leaks" or something? We'd have to update the callers, but there are only 2 of them. I dunno. t/test-lib.sh | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)