Re: [PATCH] gdb/testsuite: handle invalid .exp names passed in TESTS

[Andrew Burgess <aburgess@redhat.com>]

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: