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

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

[Thiago Jung Bauermann]

Hello,

This version addresses Luis' review comments on v1. Built with
“--enable-targets=all” and regression tested on aarch64-linux native on
Ubuntu 20.04.

Changes since v1:

- Patch “gdb/aarch64: Fix thread's gdbarch when SVE vector length changes”
  - Use aarch64_read_description () (which caches descriptions) to get the
    new target description.
  - Simplify aarch64_features_from_target_desc () a little bit by using the
    existing aarch64_get_tdesc_vq (). In v1, the former duplicated the
    latter's logic.
  - Remove from aarch64_gdbarch_init () code which tries to read vq from
    struct gdbarch_info.id. It was dead code in v1. This also allows
    simplifying the if block a bit further down for the case where tdesc
    doesn't have registers.
  - Remove the id member of struct gdbarch_info, since this patch removes
    its only user.

- Patch “gdb/testsuite: Add test for AArch64 Scalable Vector Extension”
  - Remove mention of QEMU's copyright statement.
  - Use the GNU Coding Style in the C file.
  - Be more specific about what the test does in the Tcl file.
  - Use skip_aarch64_sve_tests to exit early if SVE isn't supported.
  - Use “info all-registers” rather than “info registers” to check whether
    all registers are there after the vector length changes. This makes the
    test take ~15s rather than ~4s. I assume this isn't a problem.
  - Test that the new vector length is reflected in the $vg register.
  - Test that the new vector length is reflected in the size of the $z[0-31]
    registers.
  - Loop the same number of iterations that the inferior is supposed to
    loop.
  - Add small comment documenting the count_newlines procedure.

Original cover letter below:

While working on gdbserver support for the case where the inferior changes
the SVE vector length, I noticed this problem in the GDB native handling of
the same scenario.

Here's a fix and a testcase which fails without the fix, and passes with it.

Regression tested on aarch64-linux native on Ubuntu 20.04.

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                     |  41 +++++-----
 gdb/aarch64-tdep.h                     |   2 +
 gdb/gdbarch.h                          |  13 +---
 gdb/testsuite/gdb.arch/aarch64-sve.c   |  64 ++++++++++++++++
 gdb/testsuite/gdb.arch/aarch64-sve.exp | 101 +++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp              |   6 ++
 gdb/testsuite/lib/mi-support.exp       |   4 -
 8 files changed, 207 insertions(+), 35 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-sve.c
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-sve.exp

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

[Thiago Jung Bauermann]

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.

Since the id member of struct gdbarch_info is now unused, it's removed.
---
 gdb/aarch64-linux-nat.c | 11 ++++++++---
 gdb/aarch64-tdep.c      | 41 ++++++++++++++++++++++++-----------------
 gdb/aarch64-tdep.h      |  2 ++
 gdb/gdbarch.h           | 13 ++-----------
 4 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index a457fcd48ad8..eda79ec6d35c 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_read_description (features);
   return gdbarch_find_by_info (info);
 }
 
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 8670197a8889..c9ce4f95447c 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3372,6 +3372,27 @@ aarch64_get_tdesc_vq (const struct target_desc *tdesc)
   return sve_vq_from_vl (vl);
 }
 
+/* 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;
+
+  if (tdesc == nullptr)
+    return features;
+
+  features.vq = aarch64_get_tdesc_vq (tdesc);
+  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;
+}
+
 /* Implement the "cannot_store_register" gdbarch method.  */
 
 static int
@@ -3422,17 +3443,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   int i, num_regs = 0, num_pseudo_regs = 0;
   int first_pauth_regnum = -1, ra_sign_state_offset = -1;
   int first_mte_regnum = -1, tls_regnum = -1;
-
-  /* Use the vector length passed via the target info.  Here -1 is used for no
-     SVE, and 0 is unset.  If unset then use the vector length from the existing
-     tdesc.  */
-  uint64_t vq = 0;
-  if (info.id == (int *) -1)
-    vq = 0;
-  else if (info.id != 0)
-    vq = (uint64_t) info.id;
-  else
-    vq = aarch64_get_tdesc_vq (info.target_desc);
+  uint64_t vq = aarch64_get_tdesc_vq (info.target_desc);
 
   if (vq > AARCH64_MAX_SVE_VQ)
     internal_error (__FILE__, __LINE__, _("VQ out of bounds: %s (max %d)"),
@@ -3452,12 +3463,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Ensure we always have a target descriptor, and that it is for the given VQ
      value.  */
   const struct target_desc *tdesc = info.target_desc;
-  if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
-    {
-      aarch64_features features;
-      features.vq = vq;
-      tdesc = aarch64_read_description (features);
-    }
+  if (!tdesc_has_registers (tdesc))
+    tdesc = aarch64_read_description ({});
   gdb_assert (tdesc);
 
   feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
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);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 324958469a7e..1287f0214885 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -250,17 +250,8 @@ struct gdbarch_info
 
   bfd *abfd = nullptr;
 
-  union
-    {
-      /* Architecture-specific target description data.  Numerous targets
-	 need only this, so give them an easy way to hold it.  */
-      struct tdesc_arch_data *tdesc_data;
-
-      /* SPU file system ID.  This is a single integer, so using the
-	 generic form would only complicate code.  Other targets may
-	 reuse this member if suitable.  */
-      int *id;
-    };
+  /* Architecture-specific target description data.  */
+  struct tdesc_arch_data *tdesc_data;
 
   enum gdb_osabi osabi = GDB_OSABI_UNKNOWN;
 

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

[Luis Machado]

Hi,

On 8/5/22 01:46, 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.
> 
> Since the id member of struct gdbarch_info is now unused, it's removed.
> ---
>   gdb/aarch64-linux-nat.c | 11 ++++++++---
>   gdb/aarch64-tdep.c      | 41 ++++++++++++++++++++++++-----------------
>   gdb/aarch64-tdep.h      |  2 ++
>   gdb/gdbarch.h           | 13 ++-----------
>   4 files changed, 36 insertions(+), 31 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index a457fcd48ad8..eda79ec6d35c 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_read_description (features);
>     return gdbarch_find_by_info (info);
>   }
>   
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 8670197a8889..c9ce4f95447c 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3372,6 +3372,27 @@ aarch64_get_tdesc_vq (const struct target_desc *tdesc)
>     return sve_vq_from_vl (vl);
>   }
>   
> +/* 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;
> +
> +  if (tdesc == nullptr)
> +    return features;
> +
> +  features.vq = aarch64_get_tdesc_vq (tdesc);
> +  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;
> +}
> +
>   /* Implement the "cannot_store_register" gdbarch method.  */
>   
>   static int
> @@ -3422,17 +3443,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>     int i, num_regs = 0, num_pseudo_regs = 0;
>     int first_pauth_regnum = -1, ra_sign_state_offset = -1;
>     int first_mte_regnum = -1, tls_regnum = -1;
> -
> -  /* Use the vector length passed via the target info.  Here -1 is used for no
> -     SVE, and 0 is unset.  If unset then use the vector length from the existing
> -     tdesc.  */
> -  uint64_t vq = 0;
> -  if (info.id == (int *) -1)
> -    vq = 0;
> -  else if (info.id != 0)
> -    vq = (uint64_t) info.id;
> -  else
> -    vq = aarch64_get_tdesc_vq (info.target_desc);
> +  uint64_t vq = aarch64_get_tdesc_vq (info.target_desc);
>   
>     if (vq > AARCH64_MAX_SVE_VQ)
>       internal_error (__FILE__, __LINE__, _("VQ out of bounds: %s (max %d)"),
> @@ -3452,12 +3463,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>     /* Ensure we always have a target descriptor, and that it is for the given VQ
>        value.  */
>     const struct target_desc *tdesc = info.target_desc;
> -  if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
> -    {
> -      aarch64_features features;
> -      features.vq = vq;
> -      tdesc = aarch64_read_description (features);
> -    }
> +  if (!tdesc_has_registers (tdesc))
> +    tdesc = aarch64_read_description ({});
>     gdb_assert (tdesc);
>   
>     feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
> 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);
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 324958469a7e..1287f0214885 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -250,17 +250,8 @@ struct gdbarch_info
>   
>     bfd *abfd = nullptr;
>   
> -  union
> -    {
> -      /* Architecture-specific target description data.  Numerous targets
> -	 need only this, so give them an easy way to hold it.  */
> -      struct tdesc_arch_data *tdesc_data;
> -
> -      /* SPU file system ID.  This is a single integer, so using the
> -	 generic form would only complicate code.  Other targets may
> -	 reuse this member if suitable.  */
> -      int *id;
> -    };
> +  /* Architecture-specific target description data.  */
> +  struct tdesc_arch_data *tdesc_data;
>   
>     enum gdb_osabi osabi = GDB_OSABI_UNKNOWN;
>   

This version looks good to me. Thanks for addressing the comments.

I think the gdbarch.h change is reasonably obvious that folks wouldn't mind it going in
along with this fix.

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

[Thiago Jung Bauermann]

It exercises a bug that GDB previously had where it would lose track of
some registers when the inferior changed its vector length.

It also checks that the vg register and the size of the z0-z31 registers
correctly reflect the new vector length.
---
 gdb/testsuite/gdb.arch/aarch64-sve.c   |  64 ++++++++++++++++
 gdb/testsuite/gdb.arch/aarch64-sve.exp | 101 +++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp              |   6 ++
 gdb/testsuite/lib/mi-support.exp       |   4 -
 4 files changed, 171 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..916b5cd82b38
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-sve.c
@@ -0,0 +1,64 @@
+/* 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.  */
+
+#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..803aae5fdfb2
--- /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" "" {
+	-re ".*$gdb_prompt $" {
+	    set ret [count_newlines $expect_out(buffer)]
+	}
+    }
+
+    return ${ret}
+}
+
+proc get_register_value {register} {
+    global gdb_prompt
+    set ret ""
+
+    gdb_test_multiple "print \$${register}" "" {
+	-re ". = \[0-9\]+\r\n$gdb_prompt $" {
+	    regexp {. = ([0-9]+)} $expect_out(buffer) matched ret
+	}
+	-re ".*$gdb_prompt $" {
+	}
+    }
+
+    return ${ret}
+}
+
+# The test executable halves the vector length in a loop, so loop along
+# to check it.
+for {set i [get_register_value "vg"]} {$i > 1} {set i [expr $i / 2]} {
+    set lines_before [count_info_registers]
+
+    gdb_test "next" ".*if .res < 0." "step over prctl vg = ${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 vg = ${i}"
+    } else {
+	fail "same number of registers vg = ${i}"
+    }
+
+    gdb_test "print \$vg" ". = ${i}" "vg was changed to ${i}"
+
+    set size_after [expr {$i * 8}]
+
+    for {set j 0} {$j < 32} {set j [incr j]} {
+	gdb_test "print sizeof(\$z$j)" ". = ${size_after}" "z$j has ${size_after} bytes"
+    }
+
+    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..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.

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

[Luis Machado]

Hi,

Sorry I didn't reply to the previous v1 discussion. See notes below.

On 8/5/22 01:46, 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.
> 
> It also checks that the vg register and the size of the z0-z31 registers
> correctly reflect the new vector length.
> ---
>   gdb/testsuite/gdb.arch/aarch64-sve.c   |  64 ++++++++++++++++
>   gdb/testsuite/gdb.arch/aarch64-sve.exp | 101 +++++++++++++++++++++++++
>   gdb/testsuite/lib/gdb.exp              |   6 ++
>   gdb/testsuite/lib/mi-support.exp       |   4 -
>   4 files changed, 171 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..916b5cd82b38
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/aarch64-sve.c
> @@ -0,0 +1,64 @@
> +/* 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.  */
> +
> +#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..803aae5fdfb2
> --- /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. :-)

> +	-re ".*$gdb_prompt $" {
> +	    set ret [count_newlines $expect_out(buffer)]
> +	}
> +    }
> +
> +    return ${ret}
> +}
> +
> +proc get_register_value {register} {
> +    global gdb_prompt
> +    set ret ""
> +
> +    gdb_test_multiple "print \$${register}" "" {
> +	-re ". = \[0-9\]+\r\n$gdb_prompt $" {
> +	    regexp {. = ([0-9]+)} $expect_out(buffer) matched ret
> +	}
> +	-re ".*$gdb_prompt $" {
> +	}
> +    }
> +
> +    return ${ret}
> +}
> +
> +# The test executable halves the vector length in a loop, so loop along
> +# to check it.
> +for {set i [get_register_value "vg"]} {$i > 1} {set i [expr $i / 2]} {
> +    set lines_before [count_info_registers]
> +
> +    gdb_test "next" ".*if .res < 0." "step over prctl vg = ${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 vg = ${i}"
> +    } else {
> +	fail "same number of registers vg = ${i}"
> +    }
> +
> +    gdb_test "print \$vg" ". = ${i}" "vg was changed to ${i}"
> +
> +    set size_after [expr {$i * 8}]
> +
> +    for {set j 0} {$j < 32} {set j [incr j]} {
> +	gdb_test "print sizeof(\$z$j)" ". = ${size_after}" "z$j has ${size_after} bytes"
> +    }
> +
> +    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..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).

Again, thanks for the patch.

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

[Thiago Jung Bauermann]

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

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

[Thiago Jung Bauermann]

Hello,

Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:

> Luis Machado <luis.machado@arm.com> writes:
>
>> 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. :-)

Ping... Does any other maintainer have an opinion on the generic parts
of this patch series?

-- 
Thiago

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

[Luis Machado]

On 8/17/22 21:46, Thiago Jung Bauermann wrote:
> 
> Hello,
> 
> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
> 
>> Luis Machado <luis.machado@arm.com> writes:
>>
>>> 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. :-)
> 
> Ping... Does any other maintainer have an opinion on the generic parts
> of this patch series?
> 

I think this can be pushed, as it is merely moving/sharing a bit of generic testsuite
code. Seems obvious enough.

Same thing for the other patch cleaning up the once-SPU-specific id field.

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

[Thiago Jung Bauermann]

Hi Luis,

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

> On 8/17/22 21:46, Thiago Jung Bauermann wrote:
>> Hello,
>> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
>> 
>>> Luis Machado <luis.machado@arm.com> writes:
>>>
>>>> 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. :-)
>> Ping... Does any other maintainer have an opinion on the generic parts
>> of this patch series?
>> 
>
> I think this can be pushed, as it is merely moving/sharing a bit of generic testsuite
> code. Seems obvious enough.
>
> Same thing for the other patch cleaning up the once-SPU-specific id field.

Nice, thank you! I don't have commit access to the git repository, so
would it be possible for you or some other committer to push them for
me? I just verified that the patches still apply cleanly.

-- 
Thiago

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

[Luis Machado]

On 8/18/22 12:37, Thiago Jung Bauermann wrote:
> 
> Hi Luis,
> 
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 8/17/22 21:46, Thiago Jung Bauermann wrote:
>>> Hello,
>>> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
>>>
>>>> Luis Machado <luis.machado@arm.com> writes:
>>>>
>>>>> 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. :-)
>>> Ping... Does any other maintainer have an opinion on the generic parts
>>> of this patch series?
>>>
>>
>> I think this can be pushed, as it is merely moving/sharing a bit of generic testsuite
>> code. Seems obvious enough.
>>
>> Same thing for the other patch cleaning up the once-SPU-specific id field.
> 
> Nice, thank you! I don't have commit access to the git repository, so
> would it be possible for you or some other committer to push them for
> me? I just verified that the patches still apply cleanly.
> 

Pushed now. Thanks!

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

[Thiago Jung Bauermann]

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

> On 8/18/22 12:37, Thiago Jung Bauermann wrote:
>> Hi Luis,
>> Luis Machado <luis.machado@arm.com> writes:
>> 
>>> On 8/17/22 21:46, Thiago Jung Bauermann wrote:
>>>> Hello,
>>>> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
>>>>
>>>>> Luis Machado <luis.machado@arm.com> writes:
>>>>>
>>>>>> 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. :-)
>>>> Ping... Does any other maintainer have an opinion on the generic parts
>>>> of this patch series?
>>>>
>>>
>>> I think this can be pushed, as it is merely moving/sharing a bit of generic testsuite
>>> code. Seems obvious enough.
>>>
>>> Same thing for the other patch cleaning up the once-SPU-specific id field.
>> Nice, thank you! I don't have commit access to the git repository, so
>> would it be possible for you or some other committer to push them for
>> me? I just verified that the patches still apply cleanly.
>> 
>
> Pushed now. Thanks!

Thank you!

-- 
Thiago