Re: [PATCH 2/2] gdb/testsuite: Add test for AArch64 Scalable Vector Extension

[Thiago Jung Bauermann via Gdb-patches <gdb-patches@sourceware.org>]

Hello Luis,

Luis Machado <luis.machado@arm.com> writes:

> Thanks for spotting this. A few comments below, mostly just small adjustments/suggestions.

Thanks for your review!

> On 7/28/22 02:23, Thiago Jung Bauermann via Gdb-patches wrote:
>> It exercises a bug that GDB previously had where it would lose track of
>> some registers when the inferior changed its vector length.
>> ---
>>   gdb/testsuite/gdb.arch/aarch64-sve.c   | 61 +++++++++++++++++++
>>   gdb/testsuite/gdb.arch/aarch64-sve.exp | 81 ++++++++++++++++++++++++++
>>   gdb/testsuite/lib/gdb.exp              |  4 ++
>>   gdb/testsuite/lib/mi-support.exp       |  4 --
>>   4 files changed, 146 insertions(+), 4 deletions(-)
>>   create mode 100644 gdb/testsuite/gdb.arch/aarch64-sve.c
>>   create mode 100644 gdb/testsuite/gdb.arch/aarch64-sve.exp
>> diff --git a/gdb/testsuite/gdb.arch/aarch64-sve.c b/gdb/testsuite/gdb.arch/aarch64-sve.c
>> new file mode 100644
>> index 000000000000..f56a5799a522
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/aarch64-sve.c
>
> Given this exercises the target description re-creation for a particular thread, should we
> name the test
> with something that hints at that purpose?
>
> Unless you have future plans for this test to exercise changing the vector length
> mid-execution.

The test changes the vector length mid-execution but doesn't check the
new vector length. Doing that is a good idea, will do in v2.

I haven't implemented anything else yet, but I was considering extending
it to set and check the SVE registers. I can submit it as a separate
patch later, if it's considered a useful addition.

>> +/* This test was based on QEMU's sve-ioctls.c test file, which at the time had
>> +   the following copyright statement:
>> +
>> +   Copyright (c) 2019 Linaro Ltd
>> +
>> +   SPDX-License-Identifier: GPL-2.0-or-later */
>
> I don't see other obvious occurrences of these copyright statements. It might be OK to
> just mention
> its origin, as it is also GPL.

Indeed. In v2 I'll just mention “This test was based on QEMU's
sve-ioctls.c test file.”

>> +#include <stdio.h>
>> +#include <sys/auxv.h>
>> +#include <sys/prctl.h>
>> +
>> +static int do_sve_ioctl_test(void)
>
> The usual formatting fun, space before ( and other bits across the file.

I didn't worry about formatting of the test file because I have a vague
memory of someone saying a long time ago that it was good for the
testsuite to have a variety of formatting styles so that GDB can be
exposed to them during testing. I see now that the Internals wiki says
that “unless a test requires a particular style (which is rare) you
can't go wrong following the GDB coding standards”¹.

I'll fix the formatting.

>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/aarch64-sve.exp
>> @@ -0,0 +1,81 @@
>> +# 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 SVE-related scenarios.
>
> Same comment about mentioning the purpose of the test more precisely.

Ok, I'll be more specific.

>> +if {![is_aarch64_target]} {
>> +    verbose "Skipping ${gdb_test_file_name}."
>> +    return
>> +}
>> +
>
> In additional to the above guard, we should also guard against aarch64 targets that don't
> support SVE, with skip_aarch64_sve_tests.
>
> Otherwise we get the following:
>
> FAIL: gdb.arch/aarch64-sve.exp: runto: run to aarch64-sve.c:44

Ah, I thought the C code checking AT_HWCAP and returning was enough, but
I forgot to verify it. I'll use skip_aarch64_sve_tests.

>> +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 registers" "" {
>
> The fp/vector registers are usually not displayed by "info registers". Does it make sense
> to use "info all-registers" instead?

Good idea, it does allow the test to catch potential bugs affecting
registers not displayed by “info registers”. The downside is that it
makes the test slower: using “info registers”, the test takes about 2.8s
on my development machine. Changing it to use “info all-registers”
increases the test time to 14.5s. Is this a problem?

> Its output is quite lengthy. Alternatively, there is also "maint print xml-tdesc".

It's an interesting idea and it takes the test time back down to 3.5s.
Unfortunately, then the test doesn't catch the bug (tpidr is present in
“maint print xml-tdesc” but not shown in “info registers”).

>> +	-re ".*$gdb_prompt $" {
>> +	    set ret [count_newlines $expect_out(buffer)]
>> +	}
>> +    }
>> +
>> +    return ${ret}
>> +}
>> +
>> +# The test executable halves the vector length in a loop, so loop along
>> +# to check it.
>> +set i 0
>> +while 1 {
>
> Instead of having an infinite loop, should we iterate over this test only as many times as
> needed?
>
> For example, if we have 8 different VL values, we'd iterate over this 8 times.
>
> My worry is that we might run into a failure and keep looping indefinitely.

I addressed this problem by adding an “unexpected output” case to the
gdb_test_multiple call that continues the loop, and verified that it
works by changing the C code to segfault within the loop. The test
noticed the problem and exited.

But I understand that it looks fragile. I'll change the test to
calculate how many times the C code is supposed to loop and iterate just
that many times.

>> +    incr i
>> +
>> +    set lines_before [count_info_registers]
>> +
>> +    gdb_test "next" ".*if .res < 0. ." "step over prctl iteration ${i}"
>> +
>> +    set lines_after [count_info_registers]
>> +
>> +    # There was a bug where GDB would lose track of some registers when the
>> +    # vector length changed.  Make sure they're still all there.
>> +    if {${lines_before} == ${lines_after}} {
>> +	pass "same number of registers iteration ${i}"
>> +    } else {
>> +	fail "same number of registers iteration ${i}"
>> +    }
>> +
>> +    gdb_test_multiple "continue" "" {
>> +	-re ".*Breakpoint $decimal, do_sve_ioctl_test .*$gdb_prompt $" {
>> +	    # Next iteration.
>> +	}
>> +	-re "Inferior 1 .* exited normally.*$gdb_prompt $" {
>> +	    # We're done.
>> +	    break
>> +	}
>> +	-re "$gdb_prompt $" {
>> +	    fail "unexpected output"
>> +	    break;
>> +	}
>> +    }
>> +}
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index a8f25b5f0dd5..3a8b880c074d 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -7885,6 +7885,10 @@ proc multi_line_input { args } {
>>       return [join $args "\n"]
>>   }
>>   +proc count_newlines { string } {
>> +    return [regexp -all "\n" $string]
>> +}
>> +
>
> Given you're moving this, how about adding some light documentation to it?

Ok, will do.

-- 
Thiago

¹ https://sourceware.org/gdb/wiki/Internals%20GDB-Testsuite-Coding-Standards