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

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

Hi,

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

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.

> @@ -0,0 +1,61 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   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/>.  */
> +
> +/* Exercise AArch64's Scalable Vector Extension.  */

Same comment here. If we're aiming at testing target description re-creation, we should probably mention
that. Unless you have future plans, of course.

> +
> +/* 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.

> +
> +#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.

> +{
> +    int i, res, init_vl;
> +
> +    res = prctl(PR_SVE_GET_VL, 0, 0, 0, 0);
> +    if (res < 0) {
> +        printf("FAILED to PR_SVE_GET_VL (%d)", res);
> +        return -1;
> +    }
> +    init_vl = res & PR_SVE_VL_LEN_MASK;
> +
> +    for (i = init_vl; i > 15; i /= 2) {
> +        printf("Checking PR_SVE_SET_VL=%d\n", i);
> +        res = prctl(PR_SVE_SET_VL, i, 0, 0, 0, 0); /* break here */
> +        if (res < 0) {
> +            printf("FAILED to PR_SVE_SET_VL (%d)", res);
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    if (getauxval(AT_HWCAP) & HWCAP_SVE) {
> +        return do_sve_ioctl_test();
> +    } else {
> +        printf("SKIP: no HWCAP_SVE on this system\n");
> +        return 1;
> +    }
> +}
> diff --git a/gdb/testsuite/gdb.arch/aarch64-sve.exp b/gdb/testsuite/gdb.arch/aarch64-sve.exp
> new file mode 100644
> index 000000000000..5c4a9fa2d770
> --- /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.

> +
> +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

> +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?

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

> +	-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.

> +    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?

>   # 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.