From: Thiago Jung Bauermann via Gdb-patches <gdb-patches@sourceware.org>
To: Luis Machado <luis.machado@arm.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v2 2/2] gdb/testsuite: Add test for AArch64 Scalable Vector Extension
Date: Fri, 05 Aug 2022 21:57:23 +0000 [thread overview]
Message-ID: <87o7wynyet.fsf@linaro.org> (raw)
In-Reply-To: <a8ca12eb-ee9c-211a-84c2-c5bda6aa50c2@arm.com>
Hello Luis,
Luis Machado <luis.machado@arm.com> writes:
> Hi,
>
> Sorry I didn't reply to the previous v1 discussion. See notes below.
No problem. Thank you for quickly reviewing the patches!
> On 8/5/22 01:46, Thiago Jung Bauermann via Gdb-patches wrote:
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/aarch64-sve.exp
>> @@ -0,0 +1,101 @@
>> +# Copyright 2022 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# Test a binary that uses SVE and exercise changing the SVE vector length.
>> +
>> +if {[skip_aarch64_sve_tests]} {
>> + verbose "Skipping ${gdb_test_file_name}."
>> + return
>> +}
>> +
>> +standard_testfile
>> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
>> + return
>> +}
>> +
>> +set linespec ${srcfile}:[gdb_get_line_number "break here"]
>> +
>> +if ![runto ${linespec}] {
>> + return
>> +}
>> +
>> +# Count number of lines in "info registers" output.
>> +proc count_info_registers {} {
>> + global gdb_prompt
>> + set ret 0
>> +
>> + gdb_test_multiple "info all-registers" "" {
>
> The use of "info all-registers" seems to introduce a lot of verbosity to the test and make it much slower (as you pointed out).
>
> Checking the code, it seems we always have either FPU or SVE. So at first glance it may not be worth it to list all of the possible
> fpu/vector registers here.
>
> With that said, there are upcoming features that may add registers to the vector group (SME), so might as well check all the registers.
>
> That's a very verbose way to say the above looks good. :-)
Thank yo for the analysis. I think that the test completeness is worth
the extra time it takes.
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index a8f25b5f0dd5..b6bfa8eaafa5 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -7885,6 +7885,12 @@ proc multi_line_input { args } {
>> return [join $args "\n"]
>> }
>> +# Return how many newlines there are in the given string.
>> +
>> +proc count_newlines { string } {
>> + return [regexp -all "\n" $string]
>> +}
>> +
>> # Return the version of the DejaGnu framework.
>> #
>> # The return value is a list containing the major, minor and patch version
>> diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
>> index ca56e12b06bf..e821c0f6914f 100644
>> --- a/gdb/testsuite/lib/mi-support.exp
>> +++ b/gdb/testsuite/lib/mi-support.exp
>> @@ -1728,10 +1728,6 @@ set mi_autotest_data ""
>> # The name of the source file for autotesting.
>> set mi_autotest_source ""
>> -proc count_newlines { string } {
>> - return [regexp -all "\n" $string]
>> -}
>> -
>> # Prepares for running inline tests in FILENAME.
>> # See comments for mi_run_inline_test for detailed
>> # explanation of the idea and syntax.
>
> This looks good to me overall. I'd be more comfortable with someone else approving the generic bits (though they look
> quite obvious to me).
Ok, hopefully others will agree. :-)
> Again, thanks for the patch.
Thank you again for reviewing both versions of these patches!
--
Thiago
next prev parent reply other threads:[~2022-08-05 22:01 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-05 0:46 [PATCH v2 0/2] Fix bug in aarch64-linux GDB when inferior changes SVE vector length Thiago Jung Bauermann via Gdb-patches
2022-08-05 0:46 ` [PATCH v2 1/2] gdb/aarch64: Fix thread's gdbarch when SVE vector length changes Thiago Jung Bauermann via Gdb-patches
2022-08-05 17:26 ` Luis Machado via Gdb-patches
2022-08-05 0:46 ` [PATCH v2 2/2] gdb/testsuite: Add test for AArch64 Scalable Vector Extension Thiago Jung Bauermann via Gdb-patches
2022-08-05 17:23 ` Luis Machado via Gdb-patches
2022-08-05 21:57 ` Thiago Jung Bauermann via Gdb-patches [this message]
2022-08-17 20:46 ` Thiago Jung Bauermann via Gdb-patches
2022-08-18 8:56 ` Luis Machado via Gdb-patches
2022-08-18 11:37 ` Thiago Jung Bauermann via Gdb-patches
2022-08-18 13:47 ` Luis Machado via Gdb-patches
2022-08-18 14:08 ` Thiago Jung Bauermann via Gdb-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87o7wynyet.fsf@linaro.org \
--to=gdb-patches@sourceware.org \
--cc=luis.machado@arm.com \
--cc=thiago.bauermann@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox