[PATCH] gdb/testsuite: handle invalid .exp names passed in TESTS
Andrew Burgess
aburgess@redhat.com
Thu Sep 22 09:21:49 GMT 2022
Bruno Larsen <blarsen@redhat.com> writes:
> Hi Andrew,
>
> Once again, very well timed patch as I had just noticed this problem!
> Other than a typo (response inlined), my only comment is that, since you
> are already renaming check-single, what do you think about renaming to
> check-single-thread or check-sequential?
Except I'm not really renaming check-single. I'm adding a dependency to
check-single, and replacing the recipe for check-single. But
check-single itself continues to exist just as it did before.
I'm not against renaming check-single to something else, but I think it
should probably be done as a separate patch with its own justification.
> My reasoning is that when first reading the message and diff, I thought
> this rule would test a single test, which left me confused for a minute.
I can see that.
>
> Either way, I like the patch, and encourage you to approve it!
I'll give this a little longer to see if anyone else has any thoughts,
then I'll merge it.
Thanks,
Andrew
>
> --
> Cheers,
> Bruno
>
> On 10/09/2022 18:43, Andrew Burgess via Gdb-patches wrote:
>> I ran some tests like:
>>
>> $ make check-gdb TESTS="gdb.base/break.exp"
>>
>> then, then I went to rerun the tests later, I managed to corrupt the
>> command line, like this:
>>
>> $ make check-gdb TESTS="gdb.base/breakff.exp"
>>
>> the make command did exit with an error, but DejaGnu appeared to
>> report that every test passed! The tail end of the output looks like
>> this:
>>
>> Illegal Argument "no-matching-tests-found"
>> try "runtest --help" for option list
>> === gdb Summary ===
>>
>> # of expected passes 115
>> /tmp/build/gdb/gdb version 13.0.50.20220831-git -nw -nx -iex "set height 0" -iex "set width 0" -data-directory /tmp/build/gdb/testsuite/../data-directory
>>
>> make[3]: *** [Makefile:212: check-single] Error 1
>> make[3]: Leaving directory '/tmp/build/gdb/testsuite'
>> make[2]: *** [Makefile:161: check] Error 2
>> make[2]: Leaving directory '/tmp/build/gdb/testsuite'
>> make[1]: *** [Makefile:1916: check] Error 2
>> make[1]: Leaving directory '/tmp/build/gdb'
>> make: *** [Makefile:13565: check-gdb] Error 2
>>
>> For a while, I didn't spot that DejaGnu had failed at all, I saw the
>> 115 passes, and thought everything had run correctly - though I was
>> puzzled that make was reporting an error.
>>
>> What happens is that in gdb/testsuite/Makefile, in the check-single
>> rule, we first run DejaGnu, then run the dg-add-core-file-count.sh
>> script, and finally, we use sed to extract the results from the
>> gdb.sum file.
>>
>> In my case, with the invalid test name, DejaGnu fails, but the
>> following steps are still run, the final result, the 115 passes, is
>> then extracted from the pre-existing gdb.sum file.
>>
>> If I use 'make -jN' then the 'check-parallel' rule, rather than the
>> 'check-single' rule is used. In this case the behaviour is slightly
>> different, the tail end of the output now looks like this:
>>
>> No matching tests found.
>>
>> make[4]: Leaving directory '/tmp/build/gdb/testsuite'
>> find: ‘outputs’: No such file or directory
>> Usage: ../../../src/gdb/testsuite/../../contrib/dg-extract-results.py [-t tool] [-l variant-list] [-L] log-or-sum-file ...
>>
>> tool The tool (e.g. g++, libffi) for which to create a
>> new test summary file. If not specified then output
>> is created for all tools.
>> variant-list One or more test variant names. If the list is
>> not specified then one is constructed from all
>> variants in the files for <tool>.
>> sum-file A test summary file with the format of those
>> created by runtest from DejaGnu.
>> If -L is used, merge *.log files instead of *.sum. In this
>> mode the exact order of lines may not be preserved, just different
>> Running *.exp chunks should be in correct order.
>> find: ‘outputs’: No such file or directory
>> Usage: ../../../src/gdb/testsuite/../../contrib/dg-extract-results.py [-t tool] [-l variant-list] [-L] log-or-sum-file ...
>>
>> tool The tool (e.g. g++, libffi) for which to create a
>> new test summary file. If not specified then output
>> is created for all tools.
>> variant-list One or more test variant names. If the list is
>> not specified then one is constructed from all
>> variants in the files for <tool>.
>> sum-file A test summary file with the format of those
>> created by runtest from DejaGnu.
>> If -L is used, merge *.log files instead of *.sum. In this
>> mode the exact order of lines may not be preserved, just different
>> Running *.exp chunks should be in correct order.
>> make[3]: Leaving directory '/tmp/build/gdb/testsuite'
>> make[2]: Leaving directory '/tmp/build/gdb/testsuite'
>> make[1]: Leaving directory '/tmp/build/gdb'
>>
>> Rather than DejaGnu failing, we now get a nice 'No matching tests
>> found' message, followed by some other noise. This other noise is
>> first `find` failing, followed by the dg-extract-results.py script
>> failing.
>>
>> What happens here is that, in the check-parallel rule, the outputs
>> directory is deleted before DejaGnu is invoked. Then we try to run
>> all the tests, and finally we use find and dg-extract-results.py to
>> combine all the separate .sun
> .sum*
>> and .log files together. However, if
>> there are no tests run then the outputs/ directory is never created,
>> so the find command and consequently the dg-extract-results.py script,
>> fail.
>>
>> This commit aims to fix the following issues:
>>
>> (1) For check-single and check-parallel rules, don't run any of the
>> post-processing steps if DejaGnu failed to run. This will avoid all
>> the noise after the initial failure of DejaGnu,
>>
>> (2) For check-single ensure that we don't accidentally report
>> previous results, this is related to the above, but is worth calling
>> out as a separate point, and
>>
>> (3) For check-single, print the 'No matching tests found' message
>> just like we do for a parallel test run. This makes the parallel and
>> non-parallel testing behaviour more similar, and I think is clearer
>> than the current 'Illegal Argument' error message.
>>
>> Points (1) and (2) will be handled by moving the post processing steps
>> inside an if block within the recipe. For check-single I propose
>> deleting the gdb.sum and gdb.log files before running DejaGnu, this is
>> similar (I think) to how we delete the outputs/ directory in the
>> check-parallel rule.
>>
>> For point (3) I plan to split the check-single rule in two, the
>> existing check-single will be renamed do-check-single, then a new
>> check-single rule will be added. The new check-single rule can either
>> depend on the new do-check-single, or will ensure the 'No matching
>> tests found' message is printed when appropriate.
>> ---
>> gdb/testsuite/Makefile.in | 40 +++++++++++++++++++++++++++++----------
>> 1 file changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
>> index 87ba522c9e0..49b8a60a35c 100644
>> --- a/gdb/testsuite/Makefile.in
>> +++ b/gdb/testsuite/Makefile.in
>> @@ -203,18 +203,36 @@ expanded_tests := $(patsubst $(srcdir)/%,%,$(wildcard $(addprefix $(srcdir)/,$(T
>> expanded_tests_or_none := $(or $(expanded_tests),no-matching-tests-found)
>> endif
>>
>> +# With check-single, if TESTS was expanded to "no-matching-tests-found" then
>> +# this will be passed to DejaGnu, resuling in an error. With check-parallel
>> +# in the same situation, we avoid invoking DejaGnu, and instead just call
>> +# the check/no-matching-tests-found rule (which prints a helpful message).
>> +#
>> +# To get the same behaviour for check-single we decide here, based on how
>> +# TESTS expanded, whether check-single should redirect to do-check-single or
>> +# to check/no-matching-tests-found.
>> +ifeq ($(expanded_tests_or_none),no-matching-tests-found)
>> +CHECK_SINGLE_DEP=check/no-matching-tests-found
>> +else
>> +CHECK_SINGLE_DEP=do-check-single
>> +endif
>> +
>> # Shorthand for running all the tests in a single directory.
>> check-gdb.%:
>> $(MAKE) check TESTS="gdb.$*/*.exp"
>>
>> -check-single:
>> - -rm -f *core*
>> +do-check-single:
>> + -rm -f *core* gdb.sum gdb.log
>> $(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP); \
>> result=$$?; \
>> - $(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
>> - sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
>> + if test -e gdb.sum; then \
>> + $(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
>> + sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
>> + fi; \
>> exit $$result
>>
>> +check-single: $(CHECK_SINGLE_DEP)
>> +
>> check-single-racy:
>> -rm -rf cache racy_outputs temp
>> mkdir -p racy_outputs; \
>> @@ -240,12 +258,14 @@ check-parallel:
>> -rm -rf cache outputs temp
>> $(MAKE) -k do-check-parallel; \
>> result=$$?; \
>> - $(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh \
>> - `find outputs -name gdb.sum -print` > gdb.sum; \
>> - $(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \
>> - `find outputs -name gdb.log -print` > gdb.log; \
>> - $(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
>> - sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
>> + if test -d outputs; then \
>> + $(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh \
>> + `find outputs -name gdb.sum -print` > gdb.sum; \
>> + $(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \
>> + `find outputs -name gdb.log -print` > gdb.log; \
>> + $(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \
>> + sed -n '/=== gdb Summary ===/,$$ p' gdb.sum; \
>> + fi; \
>> exit $$result
>>
>> check-parallel-racy:
More information about the Gdb-patches
mailing list