From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 80B97385840F for ; Thu, 22 Sep 2022 09:21:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 80B97385840F Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-558-QeMed1NLNq2RxHdUqjtntQ-1; Thu, 22 Sep 2022 05:21:52 -0400 X-MC-Unique: QeMed1NLNq2RxHdUqjtntQ-1 Received: by mail-wm1-f72.google.com with SMTP id h4-20020a1c2104000000b003b334af7d50so807506wmh.3 for ; Thu, 22 Sep 2022 02:21:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:to:from:x-gm-message-state:from:to:cc:subject :date; bh=NKOZ4ys6xcq4BZUc+LgBK+1v5/N9WhqDnzlNTmwKvlg=; b=EeLK8sFAxcssMyGIn9fUvVL/kLgjI1W6iNVcn73igQSnXJB6t9IfacHbQgiNYyZTIV fsxeKOIDdQLuHgXByW/bkBslGMEtnda2bEf2VY4xcZ/1fDIt9T4tkwjIbeqjQhVQja6u /7z3yh+YgGhrLyYfpP7myeRs7LZUhh3xh6dWLefwK7zEIaEhtRqdRi6+ZhaGNIbanuRV alD9KCLHNU9q8zqlU6Tp7mAoZfyRu9xA3oyUqZL5C3kUuCumOfWLtyJtQjk+yjPVkKzm Z8OIzFcSF3r53mtCPSUQN+Kg2WdXLpc/CFFzyaHUcsj9KkHgcVLmlZOrQYhlia9jsv/B /Pbg== X-Gm-Message-State: ACrzQf10HSsuovqy/3Xz4IGhmHMIUfIlH70JMLq8GtBvjjrF+Eey0r3q uk31e+H3n++EYNmiKVQCSDVl/k1MnF0VOXkJQP342cW7gfu2kU/hAOQOGBna/CgyW8uEzA7BSrd 3C01fF0vLvKDeNkCs0T37PQ== X-Received: by 2002:a05:600c:444b:b0:3b4:cb9e:bd93 with SMTP id v11-20020a05600c444b00b003b4cb9ebd93mr1653486wmn.39.1663838511184; Thu, 22 Sep 2022 02:21:51 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6BD+SruZrjuQ9jEICMIb0v22rbLmMvWhDyUqpwbBm0LYNzam7DJ/eM41JmgaB+PfGCvC0qlg== X-Received: by 2002:a05:600c:444b:b0:3b4:cb9e:bd93 with SMTP id v11-20020a05600c444b00b003b4cb9ebd93mr1653470wmn.39.1663838510862; Thu, 22 Sep 2022 02:21:50 -0700 (PDT) Received: from localhost (52.72.115.87.dyn.plus.net. [87.115.72.52]) by smtp.gmail.com with ESMTPSA id z5-20020a1c4c05000000b003b5054c6f87sm680793wmf.21.2022.09.22.02.21.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Sep 2022 02:21:50 -0700 (PDT) From: Andrew Burgess To: Bruno Larsen , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb/testsuite: handle invalid .exp names passed in TESTS In-Reply-To: References: <20220910164349.1945521-1-aburgess@redhat.com> Date: Thu, 22 Sep 2022 10:21:49 +0100 Message-ID: <87k05vepci.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Sep 2022 09:21:59 -0000 Bruno Larsen writes: > Hi Andrew, > > Once again, very well timed patch as I had just noticed this problem!=20 > Other than a typo (response inlined), my only comment is that, since you= =20 > are already renaming check-single, what do you think about renaming to=20 > 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= =20 > 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 > > --=20 > Cheers, > Bruno > > On 10/09/2022 18:43, Andrew Burgess via Gdb-patches wrote: >> I ran some tests like: >> >> $ make check-gdb TESTS=3D"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=3D"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 >> =09=09=3D=3D=3D gdb Summary =3D=3D=3D >> >> # of expected passes=09=09115 >> /tmp/build/gdb/gdb version 13.0.50.20220831-git -nw -nx -iex "set he= ight 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: =E2=80=98outputs=E2=80=99: 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 outp= ut >> 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 . >> 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 differen= t >> Running *.exp chunks should be in correct order. >> find: =E2=80=98outputs=E2=80=99: 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 outp= ut >> 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 . >> 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 differen= t >> 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 :=3D $(patsubst $(srcdir)/%,%,$(wil= dcard $(addprefix $(srcdir)/,$(T >> expanded_tests_or_none :=3D $(or $(expanded_tests),no-matching-tests-f= ound) >> endif >> =20 >> +# 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-par= allel >> +# in the same situation, we avoid invoking DejaGnu, and instead just ca= ll >> +# the check/no-matching-tests-found rule (which prints a helpful messag= e). >> +# >> +# To get the same behaviour for check-single we decide here, based on h= ow >> +# TESTS expanded, whether check-single should redirect to do-check-sing= le or >> +# to check/no-matching-tests-found. >> +ifeq ($(expanded_tests_or_none),no-matching-tests-found) >> +CHECK_SINGLE_DEP=3Dcheck/no-matching-tests-found >> +else >> +CHECK_SINGLE_DEP=3Ddo-check-single >> +endif >> + >> # Shorthand for running all the tests in a single directory. >> check-gdb.%: >> =09$(MAKE) check TESTS=3D"gdb.$*/*.exp" >> =20 >> -check-single: >> -=09-rm -f *core* >> +do-check-single: >> +=09-rm -f *core* gdb.sum gdb.log >> =09$(DO_RUNTEST) $(RUNTESTFLAGS) $(expanded_tests_or_none) $(TIMESTAMP= ); \ >> =09result=3D$$?; \ >> -=09$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \ >> -=09sed -n '/=3D=3D=3D gdb Summary =3D=3D=3D/,$$ p' gdb.sum; \ >> +=09if test -e gdb.sum; then \ >> +=09 $(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \ >> +=09 sed -n '/=3D=3D=3D gdb Summary =3D=3D=3D/,$$ p' gdb.sum; \ >> +=09fi; \ >> =09exit $$result >> =20 >> +check-single: $(CHECK_SINGLE_DEP) >> + >> check-single-racy: >> =09-rm -rf cache racy_outputs temp >> =09mkdir -p racy_outputs; \ >> @@ -240,12 +258,14 @@ check-parallel: >> =09-rm -rf cache outputs temp >> =09$(MAKE) -k do-check-parallel; \ >> =09result=3D$$?; \ >> -=09$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh \ >> -=09 `find outputs -name gdb.sum -print` > gdb.sum; \ >> -=09$(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \ >> -=09 `find outputs -name gdb.log -print` > gdb.log; \ >> -=09$(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \ >> -=09sed -n '/=3D=3D=3D gdb Summary =3D=3D=3D/,$$ p' gdb.sum; \ >> +=09if test -d outputs; then \ >> +=09 $(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh \ >> +=09 `find outputs -name gdb.sum -print` > gdb.sum; \ >> +=09 $(SHELL) $(srcdir)/../../contrib/dg-extract-results.sh -L \ >> +=09 `find outputs -name gdb.log -print` > gdb.log; \ >> +=09 $(SHELL) $(srcdir)/lib/dg-add-core-file-count.sh; \ >> +=09 sed -n '/=3D=3D=3D gdb Summary =3D=3D=3D/,$$ p' gdb.sum; \ >> +=09fi; \ >> =09exit $$result >> =20 >> check-parallel-racy: