[PATCH 0/2] Fix bug in aarch64-linux GDB when inferior changes SVE vector length

[PATCH 0/2] Fix bug in aarch64-linux GDB when inferior changes SVE vector length

[Thiago Jung Bauermann via Gdb-patches]

Hello,

While working on gdbserver support for the case where the inferior changes
the SVE vector length, I noticed aproblem in the same scenario when doing
native debugging (details are in patch 1).

The testcase fails without the fix, and passes with it. Regression tested on
aarch64-linux native on Ubuntu 20.04.

Thanks,
Thiago

Thiago Jung Bauermann (2):
  gdb/aarch64: Fix thread's gdbarch when SVE vector length changes
  gdb/testsuite: Add test for AArch64 Scalable Vector Extension

 gdb/aarch64-linux-nat.c                | 11 +++-
 gdb/aarch64-tdep.c                     | 25 ++++++++
 gdb/aarch64-tdep.h                     |  2 +
 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 --
 7 files changed, 181 insertions(+), 7 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-sve.c
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-sve.exp

[PATCH 1/2] gdb/aarch64: Fix thread's gdbarch when SVE vector length changes

[Thiago Jung Bauermann via Gdb-patches]

When the inferior program changes the SVE length, GDB can stop tracking
some registers as it obtains the new gdbarch that corresponds to the
updated length:

  Breakpoint 1, do_sve_ioctl_test () at sve-ioctls.c:44
  44              res = prctl(PR_SVE_SET_VL, i, 0, 0, 0, 0);
  (gdb) print i
  $2 = 32
  (gdb) info registers
          ⋮
  [ snip registers x0 to x30 ]
          ⋮
  sp             0xffffffffeff0      0xffffffffeff0
  pc             0xaaaaaaaaa8ac      0xaaaaaaaaa8ac <do_sve_ioctl_test+112>
  cpsr           0x60000000          [ EL=0 BTYPE=0 C Z ]
  fpsr           0x0                 0
  fpcr           0x0                 0
  vg             0x8                 8
  tpidr          0xfffff7fcb320      0xfffff7fcb320
  (gdb) next
  45              if (res < 0) {
  (gdb) info registers
          ⋮
  [ snip registers x0 to x30 ]
          ⋮
  sp             0xffffffffeff0      0xffffffffeff0
  pc             0xaaaaaaaaa8cc      0xaaaaaaaaa8cc <do_sve_ioctl_test+144>
  cpsr           0x200000            [ EL=0 BTYPE=0 SS ]
  fpsr           0x0                 0
  fpcr           0x0                 0
  vg             0x4                 4
  (gdb)

Notice that register tpidr disappeared when vg (which holds the vector
length) changed from 8 to 4.  The tpidr register is provided by the
org.gnu.gdb.aarch64.tls feature.

This happens because the code that searches for a new gdbarch to match the
new vector length in aarch64_linux_nat_target::thread_architecture doesn't
take into account the features present in the target description associated
with the previous gdbarch.  This patch makes it do that.
---
 gdb/aarch64-linux-nat.c | 11 ++++++++---
 gdb/aarch64-tdep.c      | 25 +++++++++++++++++++++++++
 gdb/aarch64-tdep.h      |  2 ++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index a457fcd48ad8..5963e246b43f 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -900,11 +900,16 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
 
   /* We reach here if the vector length for the thread is different from its
      value at process start.  Lookup gdbarch via info (potentially creating a
-     new one), stashing the vector length inside id.  Use -1 for when SVE
-     unavailable, to distinguish from an unset value of 0.  */
+     new one) by using a target description that corresponds to the new vq value
+     and the current architecture features.  */
+
+  const struct target_desc *tdesc = gdbarch_target_desc (inf->gdbarch);
+  aarch64_features features = aarch64_features_from_target_desc (tdesc);
+  features.vq = vq;
+
   struct gdbarch_info info;
   info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
-  info.id = (int *) (vq == 0 ? -1 : vq);
+  info.target_desc = aarch64_create_target_description (features);
   return gdbarch_find_by_info (info);
 }
 
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 8670197a8889..8b89b877f8f0 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3352,6 +3352,31 @@ aarch64_read_description (const aarch64_features &features)
   return tdesc;
 }
 
+/* Get the AArch64 features present in the given target description. */
+
+aarch64_features
+aarch64_features_from_target_desc (const struct target_desc *tdesc)
+{
+  aarch64_features features;
+  const struct tdesc_feature *feature;
+
+  if (tdesc == nullptr)
+    return features;
+
+  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.sve");
+  if (feature != nullptr)
+    features.vq = tdesc_register_bitsize (feature, "z0") / 128;
+
+  features.pauth
+      = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth") != nullptr);
+  features.mte
+      = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.mte") != nullptr);
+  features.tls
+      = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.tls") != nullptr);
+
+  return features;
+}
+
 /* Return the VQ used when creating the target description TDESC.  */
 
 static uint64_t
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index 5bdd733dce32..d8513023c376 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -121,6 +121,8 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep_base
 };
 
 const target_desc *aarch64_read_description (const aarch64_features &features);
+aarch64_features
+aarch64_features_from_target_desc (const struct target_desc *tdesc);
 
 extern int aarch64_process_record (struct gdbarch *gdbarch,
 			       struct regcache *regcache, CORE_ADDR addr);

Re: [PATCH 1/2] gdb/aarch64: Fix thread's gdbarch when SVE vector length changes

[Luis Machado via Gdb-patches]

Hi,

Thanks for the patch.

On 7/28/22 02:23, Thiago Jung Bauermann via Gdb-patches wrote:
> When the inferior program changes the SVE length, GDB can stop tracking
> some registers as it obtains the new gdbarch that corresponds to the
> updated length:
> 
>    Breakpoint 1, do_sve_ioctl_test () at sve-ioctls.c:44
>    44              res = prctl(PR_SVE_SET_VL, i, 0, 0, 0, 0);
>    (gdb) print i
>    $2 = 32
>    (gdb) info registers
>            ⋮
>    [ snip registers x0 to x30 ]
>            ⋮
>    sp             0xffffffffeff0      0xffffffffeff0
>    pc             0xaaaaaaaaa8ac      0xaaaaaaaaa8ac <do_sve_ioctl_test+112>
>    cpsr           0x60000000          [ EL=0 BTYPE=0 C Z ]
>    fpsr           0x0                 0
>    fpcr           0x0                 0
>    vg             0x8                 8
>    tpidr          0xfffff7fcb320      0xfffff7fcb320
>    (gdb) next
>    45              if (res < 0) {
>    (gdb) info registers
>            ⋮
>    [ snip registers x0 to x30 ]
>            ⋮
>    sp             0xffffffffeff0      0xffffffffeff0
>    pc             0xaaaaaaaaa8cc      0xaaaaaaaaa8cc <do_sve_ioctl_test+144>
>    cpsr           0x200000            [ EL=0 BTYPE=0 SS ]
>    fpsr           0x0                 0
>    fpcr           0x0                 0
>    vg             0x4                 4
>    (gdb)
> 
> Notice that register tpidr disappeared when vg (which holds the vector
> length) changed from 8 to 4.  The tpidr register is provided by the
> org.gnu.gdb.aarch64.tls feature.
> 
> This happens because the code that searches for a new gdbarch to match the
> new vector length in aarch64_linux_nat_target::thread_architecture doesn't
> take into account the features present in the target description associated
> with the previous gdbarch.  This patch makes it do that.
> ---
>   gdb/aarch64-linux-nat.c | 11 ++++++++---
>   gdb/aarch64-tdep.c      | 25 +++++++++++++++++++++++++
>   gdb/aarch64-tdep.h      |  2 ++
>   3 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index a457fcd48ad8..5963e246b43f 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -900,11 +900,16 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>   
>     /* We reach here if the vector length for the thread is different from its
>        value at process start.  Lookup gdbarch via info (potentially creating a
> -     new one), stashing the vector length inside id.  Use -1 for when SVE
> -     unavailable, to distinguish from an unset value of 0.  */
> +     new one) by using a target description that corresponds to the new vq value
> +     and the current architecture features.  */
> +
> +  const struct target_desc *tdesc = gdbarch_target_desc (inf->gdbarch);
> +  aarch64_features features = aarch64_features_from_target_desc (tdesc);
> +  features.vq = vq;
> +
>     struct gdbarch_info info;
>     info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
> -  info.id = (int *) (vq == 0 ? -1 : vq);

Even though we're removing this code, we're still actively using info.id in aarch64_gdbarch_init (). Will that work correctly?

> +  info.target_desc = aarch64_create_target_description (features);

The current mechanism caches potentially multiple target description (to account for threads having different VL's). Instead of creating a fresh target description here, would it make
more sense to figure out the features from this particular thread and then check if we have the right target description already?

Maybe through aarch64_linux_nat_target::read_description ()?

See the code in gdb/aarch64-tdep.c:aarch64_read_description (), which fetches a cached target description and creates a fresh one if it doesn't exist.

>     return gdbarch_find_by_info (info);
>   }
>   
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 8670197a8889..8b89b877f8f0 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3352,6 +3352,31 @@ aarch64_read_description (const aarch64_features &features)
>     return tdesc;
>   }
>   
> +/* Get the AArch64 features present in the given target description. */
> +
> +aarch64_features
> +aarch64_features_from_target_desc (const struct target_desc *tdesc)
> +{
> +  aarch64_features features;
> +  const struct tdesc_feature *feature;
> +
> +  if (tdesc == nullptr)
> +    return features;
> +
> +  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.sve");
> +  if (feature != nullptr)
> +    features.vq = tdesc_register_bitsize (feature, "z0") / 128;
> +
> +  features.pauth
> +      = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth") != nullptr);
> +  features.mte
> +      = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.mte") != nullptr);
> +  features.tls
> +      = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.tls") != nullptr);
> +
> +  return features;
> +}
> +
>   /* Return the VQ used when creating the target description TDESC.  */
>   
>   static uint64_t
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index 5bdd733dce32..d8513023c376 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -121,6 +121,8 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep_base
>   };
>   
>   const target_desc *aarch64_read_description (const aarch64_features &features);
> +aarch64_features
> +aarch64_features_from_target_desc (const struct target_desc *tdesc);
>   
>   extern int aarch64_process_record (struct gdbarch *gdbarch,
>   			       struct regcache *regcache, CORE_ADDR addr);

Re: [PATCH 1/2] gdb/aarch64: Fix thread's gdbarch when SVE vector length changes

[Thiago Jung Bauermann via Gdb-patches]

Hello Luis,

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

> Thanks for the patch.

Thank you for your prompt review!

> On 7/28/22 02:23, Thiago Jung Bauermann via Gdb-patches wrote:
>> --- a/gdb/aarch64-linux-nat.c
>> +++ b/gdb/aarch64-linux-nat.c
>> @@ -900,11 +900,16 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>>       /* We reach here if the vector length for the thread is different from its
>>        value at process start.  Lookup gdbarch via info (potentially creating a
>> -     new one), stashing the vector length inside id.  Use -1 for when SVE
>> -     unavailable, to distinguish from an unset value of 0.  */
>> +     new one) by using a target description that corresponds to the new vq value
>> +     and the current architecture features.  */
>> +
>> +  const struct target_desc *tdesc = gdbarch_target_desc (inf->gdbarch);
>> +  aarch64_features features = aarch64_features_from_target_desc (tdesc);
>> +  features.vq = vq;
>> +
>>     struct gdbarch_info info;
>>     info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
>> -  info.id = (int *) (vq == 0 ? -1 : vq);
>
> Even though we're removing this code, we're still actively using
> info.id in aarch64_gdbarch_init (). Will that work correctly?

Ah, well spotted. It will — the statements that check info.id become
dead code. Your comment made me notice that the line I remove above was
the only place in all of GDB that was setting info.id, and
aarch64_gdbarch_init () was the only place reading it.

So v2 will remove the id member of struct gdbarch_info altogether (and
thus the anonymous union it was part of) and simplify a bit the
vq-related code in aarch64_gdbarch_init ().

>> +  info.target_desc = aarch64_create_target_description (features);
>
> The current mechanism caches potentially multiple target description
> (to account for threads having different VL's). Instead of creating a
> fresh target description here,

Well spotted again. This was actually a thinko on my part. I thought
I was using the function that does the target description caching. In v2
this code will call aarch64_read_description () instead of
aarch64_create_target_description ().

> would it make more sense to figure out the features from this
> particular thread and then check if we have the right target
> description already?
>
> Maybe through aarch64_linux_nat_target::read_description ()?

This patch is figuring out the features from this particular thread by
looking at its current target description so I don't think it's
necessary to use aarch64_linux_nat_target::read_description (), unless
I'm misunderstanding your point.

I'll now work on your comments for the testcase patch and will respond
to it tomorrow.

-- 
Thiago

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

[Thiago Jung Bauermann via Gdb-patches]

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
@@ -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.  */
+
+/* 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 */
+
+#include <stdio.h>
+#include <sys/auxv.h>
+#include <sys/prctl.h>
+
+static int do_sve_ioctl_test(void)
+{
+    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.
+
+if {![is_aarch64_target]} {
+    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 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 {
+    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]
+}
+
 # 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.

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

[Luis Machado via Gdb-patches]

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.

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

[Thiago Jung Bauermann via Gdb-patches]

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