Message ID | fa9c69e5-8da6-bf94-cf07-a6f9532dfebf@simark.ca |
---|---|
State | New, archived |
Headers |
Received: (qmail 100199 invoked by alias); 26 Mar 2019 14:47:01 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 99986 invoked by uid 89); 26 Mar 2019 14:47:00 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Languages-Length:1875 X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 26 Mar 2019 14:46:59 +0000 Received: from [172.16.0.89] (192-222-157-41.qc.cable.ebox.net [192.222.157.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 72F321E478; Tue, 26 Mar 2019 10:46:57 -0400 (EDT) From: Simon Marchi <simark@simark.ca> Subject: Re: GDB version as convenience variable To: Eli Zaretskii <eliz@gnu.org> Cc: gdb-patches@sourceware.org, brobecker@adacore.com References: <20181128001435.12703-1-tom@tromey.com> <83k1kxfzwo.fsf@gnu.org> <8736rja4i8.fsf@tromey.com> <83r2brhw8k.fsf@gnu.org> <87h8cmh1wg.fsf@tromey.com> <83va12gz8j.fsf@gnu.org> <87mumeb935.fsf@tromey.com> <83d0n8eyzw.fsf@gnu.org> <87d0n6adk2.fsf@tromey.com> <83imwyee29.fsf@gnu.org> <87d0n67d29.fsf@tromey.com> <83imwwc7pj.fsf@gnu.org> <83k1gts5it.fsf@gnu.org> <83r2aun9mk.fsf@gnu.org> <9d445bd2-28dd-be84-5414-510e061e4db1@simark.ca> <83mulin7ui.fsf@gnu.org> <f7bd2059933735ed8ff84d78102f46eb@simark.ca> <83k1gmn6b2.fsf@gnu.org> <a8084b7bdf7fa624e906824056016117@simark.ca> <83imw6n4ny.fsf@gnu.org> Message-ID: <fa9c69e5-8da6-bf94-cf07-a6f9532dfebf@simark.ca> Date: Tue, 26 Mar 2019 10:46:56 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: <83imw6n4ny.fsf@gnu.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit |
Commit Message
Simon Marchi
March 26, 2019, 2:46 p.m. UTC
On 2019-03-25 3:18 p.m., Eli Zaretskii wrote: >> Date: Mon, 25 Mar 2019 14:51:36 -0400 >> From: Simon Marchi <simark@simark.ca> >> Cc: gdb-patches@sourceware.org, brobecker@adacore.com >> >> comparing the output with gdb --version, or some other automated >> mean, would be perfect. > > Well, I never wrote any tests for GDB, and I'm not sure I have a > system on which to run such a test. So I hope someone else will add > this test. > Apparently, the test default.exp needs to be changed in any case, because it adds new convenience variables. Without modification, the "show convenience" test fails because there is a new entry it doesn't know about. So for now, I would suggest adding the modification below to your patch to keep the test passing. It is not obvious to change the test to match the convenience variable value against a variable or even against a regexp, because it uses gdb_test_list_exact, which does straight string comparison. Also, I am not sure if reading the output of "show version" and testing against that would be a really good test, because the implementation and the test would work essentially work the same way. So if the version string erroneously becomes 0.0.0 because of some bug in some script, the test would still pass, even though the values are wrong. At least, the following is simple and robust. Simon
Comments
> Apparently, the test default.exp needs to be changed in any case, > because it adds new convenience variables. Without modification, the > "show convenience" test fails because there is a new entry it doesn't > know about. > > So for now, I would suggest adding the modification below to your > patch to keep the test passing. It is not obvious to change the test > to match the convenience variable value against a variable or even > against a regexp, because it uses gdb_test_list_exact, which does > straight string comparison. > > Also, I am not sure if reading the output of "show version" and > testing against that would be a really good test, because the > implementation and the test would work essentially work the same way. > So if the version string erroneously becomes 0.0.0 because of some bug > in some script, the test would still pass, even though the values are > wrong. > > At least, the following is simple and robust. Hmmm, the direct string comparison thing is a bit annoying. This is going to be one more manual test to do after each branch, and I'm a bit concerned about that. We can start with that, as we want the test to pass. But I'm wondering if, instead of getting the output from "show version" to determine the expected values, we could parse gdb/version.in instead. If needed, we could so the parsing as part of the build process (ig do it in the makefile, and store the result in a couple of files). Something like that. > diff --git a/gdb/testsuite/gdb.base/default.exp b/gdb/testsuite/gdb.base/default.exp > index ece1428e617e..9ff5144448d8 100644 > --- a/gdb/testsuite/gdb.base/default.exp > +++ b/gdb/testsuite/gdb.base/default.exp > @@ -602,6 +602,8 @@ set show_conv_list \ > {$_probe_arg10 = <error: No frame selected>} \ > {$_probe_arg11 = <error: No frame selected>} \ > {$_isvoid = <internal function _isvoid>} \ > + {$_gdb_major = 8} \ > + {$_gdb_minor = 4} \ > } > if ![skip_python_tests] { > append show_conv_list \ > > > > Simon
> Date: Tue, 26 Mar 2019 13:57:16 -0700 > From: Joel Brobecker <brobecker@adacore.com> > Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sourceware.org > > This is going to be one more manual test to do after each branch, > and I'm a bit concerned about that. We can start with that, as we want > the test to pass. But I'm wondering if, instead of getting the output > from "show version" to determine the expected values, we could parse > gdb/version.in instead. If needed, we could so the parsing as part of > the build process (ig do it in the makefile, and store the result in > a couple of files). Something like that. Yes, if version.in is updated by some automated method, the same method could update the test suite.
> > This is going to be one more manual test to do after each branch, > > and I'm a bit concerned about that. We can start with that, as we want > > the test to pass. But I'm wondering if, instead of getting the output > > from "show version" to determine the expected values, we could parse > > gdb/version.in instead. If needed, we could so the parsing as part of > > the build process (ig do it in the makefile, and store the result in > > a couple of files). Something like that. > > Yes, if version.in is updated by some automated method, the same > method could update the test suite. We're actually thinking of getting rid of that daily bump, and doing this would make it more difficult. There has to be a way to parse version.in during the build and make that information available to the testsuite, somehow. If not, I'd rather adjust the testing to allow regexps, and just verify that it prints a positive number for the major version, and a non-zero number for the minor version.
> From: Simon Marchi <simark@simark.ca> > Cc: gdb-patches@sourceware.org, brobecker@adacore.com > Date: Tue, 26 Mar 2019 10:46:56 -0400 > > Apparently, the test default.exp needs to be changed in any case, because > it adds new convenience variables. Without modification, the > "show convenience" test fails because there is a new entry it doesn't know > about. > > So for now, I would suggest adding the modification below to your patch to keep > the test passing. It is not obvious to change the test to match the convenience > variable value against a variable or even against a regexp, because it uses > gdb_test_list_exact, which does straight string comparison. > > Also, I am not sure if reading the output of "show version" and testing against > that would be a really good test, because the implementation and the test would > work essentially work the same way. So if the version string erroneously becomes > 0.0.0 because of some bug in some script, the test would still pass, even though > the values are wrong. > > At least, the following is simple and robust. Thanks, I used that, and pushed the changes including this one.
On 2019-03-27 8:56 a.m., Joel Brobecker wrote: >>> This is going to be one more manual test to do after each branch, >>> and I'm a bit concerned about that. We can start with that, as we want >>> the test to pass. But I'm wondering if, instead of getting the output >>> from "show version" to determine the expected values, we could parse >>> gdb/version.in instead. If needed, we could so the parsing as part of >>> the build process (ig do it in the makefile, and store the result in >>> a couple of files). Something like that. >> >> Yes, if version.in is updated by some automated method, the same >> method could update the test suite. > > We're actually thinking of getting rid of that daily bump, and doing > this would make it more difficult. There has to be a way to parse > version.in during the build and make that information available to > the testsuite, somehow. If not, I'd rather adjust the testing to > allow regexps, and just verify that it prints a positive number for > the major version, and a non-zero number for the minor version. > I wrote a patch to make the test read the version number from version.in directly, see here: https://sourceware.org/ml/gdb-patches/2019-03/msg00762.html Simon
diff --git a/gdb/testsuite/gdb.base/default.exp b/gdb/testsuite/gdb.base/default.exp index ece1428e617e..9ff5144448d8 100644 --- a/gdb/testsuite/gdb.base/default.exp +++ b/gdb/testsuite/gdb.base/default.exp @@ -602,6 +602,8 @@ set show_conv_list \ {$_probe_arg10 = <error: No frame selected>} \ {$_probe_arg11 = <error: No frame selected>} \ {$_isvoid = <internal function _isvoid>} \ + {$_gdb_major = 8} \ + {$_gdb_minor = 4} \ } if ![skip_python_tests] { append show_conv_list \