[PATCH v2 0/6] gdbserver improvements for AArch64 SVE support

[PATCH v2 0/6] gdbserver improvements for AArch64 SVE support

[Thiago Jung Bauermann]

Hello,

This is version 2 of the patch series adding support to gdbserver for
inferiors that change the SVE vector length at runtime. This is already
supported by GDB, but not by gdbserver. Version 1 was posted here:

https://inbox.sourceware.org/gdb-patches/20220908064151.3959930-1-thiago.bauermann@linaro.org/

The main difference in this version is that it implements Luis' suggestion
of keeping the per-process target description and using per-thread ones
only when necessary (i.e., when the inferior supports SVE).

This simplified the patches significantly (thank you for the suggestion,
Luis!), and they now cause minimal change on non-SVE targets.

To recap, GDB supports different vector lengths by calling
target_ops::thread_architecture, and aarch64_linux_nat_target provides an
implementation of that method.

This patch series' idea is to provide a remote_target::thread_architecture
method so that the same mechanism can be used in the case of remote
debugging. It returns a gdbarch based on the expedited registers seen in
the most recent stop reply.

To arrive at that solution some preparation is necessary. Most patches are
small cleanups or code reorganisation to make review easier. The biggest
change is in patch 4, which adds a per-thread target description to
gdbserver. As mentioned earlier though, only SVE-supporting aarch64-linux
targets will actually use different per-thread tdescs. All other targets
continue using the per-process target description.

Also, contrary to what I mentioned to some people at the GNU Tools
Cauldron, XML target descriptions are not transmitted over the wire. What
happens is:

1. When the inferior thread stops, gdbserver fetches its vector length. If
   it's different from the last time it stopped, gdbserver sets a
   thread-specific tdesc (in aarch64_target::arch_update_tdesc).
2. gdbserver sends the new vector length as “VG” an expedited
   pseudo-register (it was already doing that, this patch series doesn't
   change this part).
3. When GDB receives a stop reply, it gets the vector length from the
   expedited registers and uses it to get a new target description and
   gdbarch, then stores the latter in remote_thread_info->expedited_arch
   (in remote_target::process_stop_reply).
4. When GDB needs to know the thread's gdbarch, it calls the process
   stratum's thread_architecture method. With these patches remote_target
   now provides said method, which returns the gdbarch obtained from the
   last stop reply.

So GDB and gdbserver independently update their own target descriptions
when they notice that the vector length changed.

With this series applied, gdb.arch/aarch64-sve.exp passes all tests on
extended-remote aarch64-linux. Without them, it fails tests after the
testcase changes the vector length.

Tested on native and extended-remote aarch64-linux, x86_64-linux and
armv7l-linux-gnueabihf (the last one on QEMU TCG).

Changes since v1:

- Patch 1: gdbserver: Add asserts in register_size and register_data functions
  - No change.
- Patch 2: gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap
  - Changed to add an “int pid” parameter rather than a “struct thread_info”
    one. Suggested by Luis.
- Patch 3: gdb,gdbserver/aarch64-linux: Allow aarch64_sve_get_vq to return error
  - Dropped, since as Luis mentioned “we will always be able to validate
    supported features when we start/attach to a process”, and also the
    situation it helped with doesn't happen anymore in the new version of
    the code.
- Patch 4: gdbserver/linux-aarch64: Factor out function to get aarch64_features
  - Changed aarch64_get_arch_features to return “struct aarch64_features”
    rather than “gdb::optional<struct aarch64_features>” since we can
    assume that aarch64_sve_get_vq always works.
- Patch 5: gdbserver: Move target description from being per-process to
  being per-thread
  - Squashed into the next patch, since it became much simpler after
    implementing Luis' suggestion of keeping per-process target
    descriptions.
- Patch 6: gdbserver/linux-aarch64: When thread stops, update its target
  description
  - Added “has_sve” bool to linux-aarch64-low's “struct arch_process_info”.
  - Changed aarch64_target::arch_update_tdesc to return early if the given
    thread's has_sve property is false.
  - Changed get_thread_regcache to look for a target description in the
    thread before looking for one in the process.
  - Changed current_target_desc to look for a target description in the
    current_thread before looking for one in the current_process.
- Patch 7: gdb/aarch64: Factor out most of the thread_architecture method
  - Fixed memory leak where aarch64_update_gdbarch was calling
    aarch64_create_target_description instead of aarch64_read_description.
- Patch 8: gdb/aarch64: Detect vector length changes when debugging remotely
  - Changed the title and description to try making them a bit clearer.

Thiago Jung Bauermann (6):
  gdbserver: Add asserts in register_size and register_data functions
  gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap
  gdbserver/linux-aarch64: Factor out function to get aarch64_features
  gdbserver/linux-aarch64: When thread stops, update its target
    description
  gdb/aarch64: Factor out most of the thread_architecture method
  gdb/aarch64: Detect vector length changes when debugging remotely

 gdb/aarch64-linux-nat.c        | 28 +-------------
 gdb/aarch64-tdep.c             | 59 +++++++++++++++++++++++++++++
 gdb/aarch64-tdep.h             |  2 +
 gdb/arch-utils.c               |  9 +++++
 gdb/arch-utils.h               |  5 +++
 gdb/gdbarch-components.py      | 16 ++++++++
 gdb/gdbarch-gen.h              | 11 ++++++
 gdb/gdbarch.c                  | 22 +++++++++++
 gdb/remote.c                   | 42 +++++++++++++++++++++
 gdbserver/gdbthread.h          |  2 +
 gdbserver/linux-aarch64-low.cc | 69 ++++++++++++++++++++++++++++------
 gdbserver/linux-arm-low.cc     |  2 +-
 gdbserver/linux-low.cc         | 36 +++++++++++++-----
 gdbserver/linux-low.h          | 14 ++++---
 gdbserver/linux-ppc-low.cc     |  6 +--
 gdbserver/linux-s390-low.cc    |  2 +-
 gdbserver/netbsd-low.cc        |  4 +-
 gdbserver/netbsd-low.h         |  2 +-
 gdbserver/regcache.cc          | 15 ++++++--
 gdbserver/server.cc            |  2 +-
 gdbserver/target.cc            |  4 +-
 gdbserver/target.h             |  2 +-
 gdbserver/tdesc.cc             |  3 ++
 23 files changed, 289 insertions(+), 68 deletions(-)


base-commit: 31c1130f35e0ef800ea4d92224a72872ffe4a5db

[PATCH v2 1/6] gdbserver: Add asserts in register_size and register_data functions

[Thiago Jung Bauermann]

These helped me during development, catching bugs closer to when they
actually happened.
---
 gdbserver/regcache.cc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 5cbcea978a05..14236069f712 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -286,6 +286,8 @@ register_cache_size (const struct target_desc *tdesc)
 int
 register_size (const struct target_desc *tdesc, int n)
 {
+  gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
+
   return find_register_by_number (tdesc, n).size / 8;
 }
 
@@ -300,6 +302,8 @@ regcache_register_size (const struct regcache *regcache, int n)
 static unsigned char *
 register_data (const struct regcache *regcache, int n)
 {
+  gdb_assert(n >= 0 && n < regcache->tdesc->reg_defs.size());
+
   return (regcache->registers
 	  + find_register_by_number (regcache->tdesc, n).offset / 8);
 }

[PATCH v2 2/6] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap

[Thiago Jung Bauermann]

This patch doesn't change gdbserver behaviour, but after later changes are
made it avoids a null pointer dereference when HWCAP needs to be obtained
for a specific process while current_thread is nullptr.

Fixing linux_read_auxv, linux_get_hwcap and linux_get_hwcap2 to take a PID
parameter seems more correct than setting current_thread in one particular
code path.

Changes are propagated to allow passing the new parameter through the call
chain.
---
 gdbserver/linux-aarch64-low.cc |  7 ++++---
 gdbserver/linux-arm-low.cc     |  2 +-
 gdbserver/linux-low.cc         | 18 +++++++++---------
 gdbserver/linux-low.h          |  9 ++++-----
 gdbserver/linux-ppc-low.cc     |  6 +++---
 gdbserver/linux-s390-low.cc    |  2 +-
 gdbserver/netbsd-low.cc        |  4 +---
 gdbserver/netbsd-low.h         |  2 +-
 gdbserver/server.cc            |  2 +-
 gdbserver/target.cc            |  4 ++--
 gdbserver/target.h             |  2 +-
 11 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index db5086962612..a6ed68f93029 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -823,12 +823,13 @@ aarch64_target::low_arch_setup ()
   if (is_elf64)
     {
       struct aarch64_features features;
+      int pid = pid_of (current_thread);
 
       features.vq = aarch64_sve_get_vq (tid);
       /* A-profile PAC is 64-bit only.  */
-      features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
+      features.pauth = (linux_get_hwcap (pid, 8) & AARCH64_HWCAP_PACA);
       /* A-profile MTE is 64-bit only.  */
-      features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
+      features.mte = linux_get_hwcap2 (pid, 8) & HWCAP2_MTE;
       features.tls = true;
 
       current_process ()->tdesc = aarch64_linux_read_description (features);
@@ -3299,7 +3300,7 @@ aarch64_target::supports_memory_tagging ()
 #endif
     }
 
-  return (linux_get_hwcap2 (8) & HWCAP2_MTE) != 0;
+  return (linux_get_hwcap2 (pid_of (current_thread), 8) & HWCAP2_MTE) != 0;
 }
 
 bool
diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
index a458b0f14a68..1420909626e8 100644
--- a/gdbserver/linux-arm-low.cc
+++ b/gdbserver/linux-arm-low.cc
@@ -958,7 +958,7 @@ get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
 static const struct target_desc *
 arm_read_description (void)
 {
-  unsigned long arm_hwcap = linux_get_hwcap (4);
+  unsigned long arm_hwcap = linux_get_hwcap (pid_of (current_thread), 4);
 
   if (arm_hwcap & HWCAP_IWMMXT)
     return arm_linux_read_description (ARM_FP_TYPE_IWMMXT);
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index a896b37528b6..fc496275d6a3 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -5480,12 +5480,11 @@ linux_process_target::supports_read_auxv ()
    to debugger memory starting at MYADDR.  */
 
 int
-linux_process_target::read_auxv (CORE_ADDR offset, unsigned char *myaddr,
-				 unsigned int len)
+linux_process_target::read_auxv (int pid, CORE_ADDR offset,
+				 unsigned char *myaddr, unsigned int len)
 {
   char filename[PATH_MAX];
   int fd, n;
-  int pid = lwpid_of (current_thread);
 
   xsnprintf (filename, sizeof filename, "/proc/%d/auxv", pid);
 
@@ -6979,14 +6978,15 @@ linux_get_pc_64bit (struct regcache *regcache)
 /* See linux-low.h.  */
 
 int
-linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
+linux_get_auxv (int pid, int wordsize, CORE_ADDR match, CORE_ADDR *valp)
 {
   gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
   int offset = 0;
 
   gdb_assert (wordsize == 4 || wordsize == 8);
 
-  while (the_target->read_auxv (offset, data, 2 * wordsize) == 2 * wordsize)
+  while (the_target->read_auxv (pid, offset, data, 2 * wordsize)
+	 == 2 * wordsize)
     {
       if (wordsize == 4)
 	{
@@ -7016,20 +7016,20 @@ linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
 /* See linux-low.h.  */
 
 CORE_ADDR
-linux_get_hwcap (int wordsize)
+linux_get_hwcap (int pid, int wordsize)
 {
   CORE_ADDR hwcap = 0;
-  linux_get_auxv (wordsize, AT_HWCAP, &hwcap);
+  linux_get_auxv (pid, wordsize, AT_HWCAP, &hwcap);
   return hwcap;
 }
 
 /* See linux-low.h.  */
 
 CORE_ADDR
-linux_get_hwcap2 (int wordsize)
+linux_get_hwcap2 (int pid, int wordsize)
 {
   CORE_ADDR hwcap2 = 0;
-  linux_get_auxv (wordsize, AT_HWCAP2, &hwcap2);
+  linux_get_auxv (pid, wordsize, AT_HWCAP2, &hwcap2);
   return hwcap2;
 }
 
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 1594f063f47c..182a540f3bb3 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -178,7 +178,7 @@ class linux_process_target : public process_stratum_target
 
   bool supports_read_auxv () override;
 
-  int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
+  int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
 		 unsigned int len) override;
 
   int insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
@@ -946,17 +946,16 @@ extern int have_ptrace_getregset;
    *VALP and return 1.  If not found or if there is an error, return
    0.  */
 
-int linux_get_auxv (int wordsize, CORE_ADDR match,
-		    CORE_ADDR *valp);
+int linux_get_auxv (int pid, int wordsize, CORE_ADDR match, CORE_ADDR *valp);
 
 /* Fetch the AT_HWCAP entry from the auxv vector, where entries are length
    WORDSIZE.  If no entry was found, return zero.  */
 
-CORE_ADDR linux_get_hwcap (int wordsize);
+CORE_ADDR linux_get_hwcap (int pid, int wordsize);
 
 /* Fetch the AT_HWCAP2 entry from the auxv vector, where entries are length
    WORDSIZE.  If no entry was found, return zero.  */
 
-CORE_ADDR linux_get_hwcap2 (int wordsize);
+CORE_ADDR linux_get_hwcap2 (int pid, int wordsize);
 
 #endif /* GDBSERVER_LINUX_LOW_H */
diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc
index 08824887003b..1db20d998ffd 100644
--- a/gdbserver/linux-ppc-low.cc
+++ b/gdbserver/linux-ppc-low.cc
@@ -894,8 +894,8 @@ ppc_target::low_arch_setup ()
 
   /* The value of current_process ()->tdesc needs to be set for this
      call.  */
-  ppc_hwcap = linux_get_hwcap (features.wordsize);
-  ppc_hwcap2 = linux_get_hwcap2 (features.wordsize);
+  ppc_hwcap = linux_get_hwcap (pid_of (current_thread), features.wordsize);
+  ppc_hwcap2 = linux_get_hwcap2 (pid_of (current_thread), features.wordsize);
 
   features.isa205 = ppc_linux_has_isa205 (ppc_hwcap);
 
@@ -1097,7 +1097,7 @@ is_elfv2_inferior (void)
   const struct target_desc *tdesc = current_process ()->tdesc;
   int wordsize = register_size (tdesc, 0);
 
-  if (!linux_get_auxv (wordsize, AT_PHDR, &phdr))
+  if (!linux_get_auxv (pid_of (current_thread), wordsize, AT_PHDR, &phdr))
     return def_res;
 
   /* Assume ELF header is at the beginning of the page where program headers
diff --git a/gdbserver/linux-s390-low.cc b/gdbserver/linux-s390-low.cc
index 5adc28070574..2d030a806350 100644
--- a/gdbserver/linux-s390-low.cc
+++ b/gdbserver/linux-s390-low.cc
@@ -592,7 +592,7 @@ s390_target::low_arch_setup ()
   /* Determine word size and HWCAP.  */
   int pid = pid_of (current_thread);
   int wordsize = s390_get_wordsize (pid);
-  unsigned long hwcap = linux_get_hwcap (wordsize);
+  unsigned long hwcap = linux_get_hwcap (pid, wordsize);
 
   /* Check whether the kernel supports extra register sets.  */
   int have_regset_last_break
diff --git a/gdbserver/netbsd-low.cc b/gdbserver/netbsd-low.cc
index f05bcd4e173f..03af80049457 100644
--- a/gdbserver/netbsd-low.cc
+++ b/gdbserver/netbsd-low.cc
@@ -581,11 +581,9 @@ netbsd_read_auxv(pid_t pid, void *offs, void *addr, size_t len)
    to debugger memory starting at MYADDR.  */
 
 int
-netbsd_process_target::read_auxv (CORE_ADDR offset,
+netbsd_process_target::read_auxv (int pid, CORE_ADDR offset,
 				  unsigned char *myaddr, unsigned int len)
 {
-  pid_t pid = pid_of (current_thread);
-
   return netbsd_read_auxv (pid, (void *) (intptr_t) offset, myaddr, len);
 }
 
diff --git a/gdbserver/netbsd-low.h b/gdbserver/netbsd-low.h
index 06b4fd613571..eced7b3dd860 100644
--- a/gdbserver/netbsd-low.h
+++ b/gdbserver/netbsd-low.h
@@ -77,7 +77,7 @@ class netbsd_process_target : public process_stratum_target
 
   bool supports_read_auxv () override;
 
-  int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
+  int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
 		 unsigned int len) override;
 
   bool supports_hardware_single_step () override;
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index aaef38e00622..63c323071670 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1443,7 +1443,7 @@ handle_qxfer_auxv (const char *annex,
   if (annex[0] != '\0' || current_thread == NULL)
     return -1;
 
-  return the_target->read_auxv (offset, readbuf, len);
+  return the_target->read_auxv (pid_of (current_thread), offset, readbuf, len);
 }
 
 /* Handle qXfer:exec-file:read.  */
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index c06a67600b1f..3651dbb9e862 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -344,8 +344,8 @@ process_stratum_target::supports_read_auxv ()
 }
 
 int
-process_stratum_target::read_auxv (CORE_ADDR offset, unsigned char *myaddr,
-				   unsigned int len)
+process_stratum_target::read_auxv (int pid, CORE_ADDR offset,
+				   unsigned char *myaddr, unsigned int len)
 {
   gdb_assert_not_reached ("target op read_auxv not supported");
 }
diff --git a/gdbserver/target.h b/gdbserver/target.h
index 18ab969dda70..fe7a80b645bc 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -175,7 +175,7 @@ class process_stratum_target
   /* Read auxiliary vector data from the inferior process.
 
      Read LEN bytes at OFFSET into a buffer at MYADDR.  */
-  virtual int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
+  virtual int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
 			 unsigned int len);
 
   /* Returns true if GDB Z breakpoint type TYPE is supported, false

[PATCH v2 3/6] gdbserver/linux-aarch64: Factor out function to get aarch64_features

[Thiago Jung Bauermann]

It will be used in a subsequent commit.  There's no functional change.
---
 gdbserver/linux-aarch64-low.cc | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index a6ed68f93029..cab4fc0a4674 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -652,6 +652,25 @@ aarch64_target::low_delete_process (arch_process_info *info)
   xfree (info);
 }
 
+/* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
+#define AARCH64_HWCAP_PACA (1 << 30)
+
+static struct aarch64_features
+aarch64_get_arch_features (const thread_info *thread)
+{
+  struct aarch64_features features;
+  int pid = pid_of (thread);
+
+  features.vq = aarch64_sve_get_vq (lwpid_of (thread));
+  /* A-profile PAC is 64-bit only.  */
+  features.pauth = linux_get_hwcap (pid, 8) & AARCH64_HWCAP_PACA;
+  /* A-profile MTE is 64-bit only.  */
+  features.mte = linux_get_hwcap2 (pid, 8) & HWCAP2_MTE;
+  features.tls = true;
+
+  return features;
+}
+
 void
 aarch64_target::low_new_thread (lwp_info *lwp)
 {
@@ -804,9 +823,6 @@ aarch64_adjust_register_sets (const struct aarch64_features &features)
     }
 }
 
-/* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
-#define AARCH64_HWCAP_PACA (1 << 30)
-
 /* Implementation of linux target ops method "low_arch_setup".  */
 
 void
@@ -822,15 +838,8 @@ aarch64_target::low_arch_setup ()
 
   if (is_elf64)
     {
-      struct aarch64_features features;
-      int pid = pid_of (current_thread);
-
-      features.vq = aarch64_sve_get_vq (tid);
-      /* A-profile PAC is 64-bit only.  */
-      features.pauth = (linux_get_hwcap (pid, 8) & AARCH64_HWCAP_PACA);
-      /* A-profile MTE is 64-bit only.  */
-      features.mte = linux_get_hwcap2 (pid, 8) & HWCAP2_MTE;
-      features.tls = true;
+      struct aarch64_features features
+	  = aarch64_get_arch_features (current_thread);
 
       current_process ()->tdesc = aarch64_linux_read_description (features);
 

[PATCH v2 4/6] gdbserver/linux-aarch64: When thread stops, update its target description

[Thiago Jung Bauermann]

This change allows aarch64-linux to support debugging programs where
different threads have different SVE vector lengths.  It requires
gdbserver to support different inferior threads having different target
descriptions.

The arch_update_tdesc method is added to the linux_process_target class to
allow aarch64-linux to probe the inferior's vq register and provide an
updated thread target description reflecting the new vector length.

After this change, all targets except SVE-supporting aarch64-linux will
still use per-process target descriptions.
---
 gdbserver/gdbthread.h          |  2 ++
 gdbserver/linux-aarch64-low.cc | 37 +++++++++++++++++++++++++++++++++-
 gdbserver/linux-low.cc         | 18 +++++++++++++++++
 gdbserver/linux-low.h          |  5 +++++
 gdbserver/regcache.cc          | 11 +++++++---
 gdbserver/tdesc.cc             |  3 +++
 6 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
index 8b897e73d33b..47b44d03b8e0 100644
--- a/gdbserver/gdbthread.h
+++ b/gdbserver/gdbthread.h
@@ -80,6 +80,8 @@ struct thread_info
 
   /* Branch trace target information for this thread.  */
   struct btrace_target_info *btrace = nullptr;
+
+  const struct target_desc *tdesc = nullptr;
 };
 
 extern std::list<thread_info *> all_threads;
diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index cab4fc0a4674..786ce4071279 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -99,6 +99,9 @@ protected:
 
   void low_arch_setup () override;
 
+  gdb::optional<const struct target_desc *>
+    arch_update_tdesc (const thread_info *thread) override;
+
   bool low_cannot_fetch_register (int regno) override;
 
   bool low_cannot_store_register (int regno) override;
@@ -184,6 +187,8 @@ struct arch_process_info
      same for each thread, it is reasonable for the data to live here.
      */
   struct aarch64_debug_reg_state debug_reg_state;
+
+  bool has_sve;
 };
 
 /* Return true if the size of register 0 is 8 byte.  */
@@ -840,8 +845,16 @@ aarch64_target::low_arch_setup ()
     {
       struct aarch64_features features
 	  = aarch64_get_arch_features (current_thread);
+      const target_desc *tdesc = aarch64_linux_read_description (features);
 
-      current_process ()->tdesc = aarch64_linux_read_description (features);
+      /* Only SVE-enabled inferiors need per-thread target descriptions.  */
+      if (features.vq > 0)
+	{
+	  current_thread->tdesc = tdesc;
+	  current_process ()->priv->arch_private->has_sve = true;
+	}
+
+      current_process ()->tdesc = tdesc;
 
       /* Adjust the register sets we should use for this particular set of
 	 features.  */
@@ -853,6 +866,28 @@ aarch64_target::low_arch_setup ()
   aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
 }
 
+/* Implementation of linux target ops method "arch_update_tdesc".  */
+
+gdb::optional<const struct target_desc *>
+aarch64_target::arch_update_tdesc (const thread_info *thread)
+{
+  /* Only processes using SVE need to update the thread's target description.  */
+  if (!get_thread_process (thread)->priv->arch_private->has_sve)
+    return {};
+
+  const struct aarch64_features features = aarch64_get_arch_features (thread);
+  const struct target_desc *tdesc = aarch64_linux_read_description (features);
+
+  if (tdesc == thread->tdesc)
+    return {};
+
+  /* Adjust the register sets we should use for this particular set of
+     features.  */
+  aarch64_adjust_register_sets(features);
+
+  return tdesc;
+}
+
 /* Implementation of linux target ops method "get_regs_info".  */
 
 const regs_info *
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index fc496275d6a3..7c510e26f0f5 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -483,6 +483,12 @@ linux_process_target::arch_setup_thread (thread_info *thread)
   low_arch_setup ();
 }
 
+gdb::optional<const struct target_desc *>
+linux_process_target::arch_update_tdesc (const thread_info *thread)
+{
+  return {};
+}
+
 int
 linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
 					    int wstat)
@@ -2348,6 +2354,18 @@ linux_process_target::filter_event (int lwpid, int wstat)
 	      return;
 	    }
 	}
+      else
+	{
+	  /* Give the arch code an opportunity to update the thread's target
+	     description.  */
+	  gdb::optional<const struct target_desc *> new_tdesc
+	      = arch_update_tdesc (thread);
+	  if (new_tdesc.has_value ())
+	    {
+	      regcache_release ();
+	      thread->tdesc = *new_tdesc;
+	    }
+	}
     }
 
   if (WIFSTOPPED (wstat) && child->must_set_ptrace_flags)
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 182a540f3bb3..ff14423e9e07 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -604,6 +604,11 @@ class linux_process_target : public process_stratum_target
   /* Architecture-specific setup for the current thread.  */
   virtual void low_arch_setup () = 0;
 
+  /* Allows arch-specific code to update the thread's target description when
+     the inferior stops.  */
+  virtual gdb::optional<const struct target_desc *>
+    arch_update_tdesc (const thread_info *thread);
+
   /* Return false if we can fetch/store the register, true if we cannot
      fetch/store the register.  */
   virtual bool low_cannot_fetch_register (int regno) = 0;
diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 14236069f712..03ee88b3cfd1 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -39,11 +39,16 @@ get_thread_regcache (struct thread_info *thread, int fetch)
      have.  */
   if (regcache == NULL)
     {
-      struct process_info *proc = get_thread_process (thread);
+      /* First see if there's a thread-specific target description.  */
+      const target_desc *tdesc = thread->tdesc;
 
-      gdb_assert (proc->tdesc != NULL);
+      /* If not, get it from the process instead.  */
+      if (tdesc == nullptr)
+	tdesc = get_thread_process (thread)->tdesc;
 
-      regcache = new_register_cache (proc->tdesc);
+      gdb_assert (tdesc != nullptr);
+
+      regcache = new_register_cache (tdesc);
       set_thread_regcache_data (thread, regcache);
     }
 
diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
index 5693cc6626fb..3665ab0540d5 100644
--- a/gdbserver/tdesc.cc
+++ b/gdbserver/tdesc.cc
@@ -129,6 +129,9 @@ current_target_desc (void)
   if (current_thread == NULL)
     return &default_description;
 
+  if (current_thread->tdesc != nullptr)
+    return current_thread->tdesc;
+
   return current_process ()->tdesc;
 }
 

[PATCH v2 5/6] gdb/aarch64: Factor out most of the thread_architecture method

[Thiago Jung Bauermann]

The same logic will be used by a subsequent commit when remotely debugging
an aarch64-linux target.

The code isn't changed, just moved around.
---
 gdb/aarch64-linux-nat.c | 28 ++--------------------------
 gdb/aarch64-tdep.c      | 35 +++++++++++++++++++++++++++++++++++
 gdb/aarch64-tdep.h      |  2 ++
 3 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index caefcb364852..ca230ea4fdb0 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -884,33 +884,9 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
   /* Find the current gdbarch the same way as process_stratum_target.  */
   inferior *inf = find_inferior_ptid (this, ptid);
   gdb_assert (inf != NULL);
-
-  /* If this is a 32-bit architecture, then this is ARM, not AArch64.
-     There's no SVE vectors here, so just return the inferior
-     architecture.  */
-  if (gdbarch_bfd_arch_info (inf->gdbarch)->bits_per_word == 32)
-    return inf->gdbarch;
-
-  /* Only return it if the current vector length matches the one in the tdep.  */
-  aarch64_gdbarch_tdep *tdep
-    = gdbarch_tdep<aarch64_gdbarch_tdep> (inf->gdbarch);
   uint64_t vq = aarch64_sve_get_vq (ptid.lwp ());
-  if (vq == tdep->vq)
-    return inf->gdbarch;
-
-  /* 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) 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.target_desc = aarch64_read_description (features);
-  return gdbarch_find_by_info (info);
+
+  return aarch64_update_gdbarch (inf->gdbarch, vq);
 }
 
 /* Implement the "supports_memory_tagging" target_ops method.  */
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 07330356fdcb..ffc128d91f60 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3486,6 +3486,41 @@ aarch64_cannot_store_register (struct gdbarch *gdbarch, int regnum)
 	  || regnum == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base));
 }
 
+/* Helper function for the "thread_architecture" target_ops method.
+
+   Returns a new gdbarch that is equivalent to the given gdbarch, but with SVE
+   registers reflecting the given vq value.  */
+
+struct gdbarch *
+aarch64_update_gdbarch (struct gdbarch *gdbarch, uint64_t vq)
+{
+  /* If this is a 32-bit architecture, then this is ARM, not AArch64.
+     There's no SVE vectors here, so just return the inferior
+     architecture.  */
+  if (gdbarch_bfd_arch_info (gdbarch)->bits_per_word == 32)
+    return gdbarch;
+
+  /* Only return it if the current vector length matches the one in the
+     tdep.  */
+  aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
+  if (vq == tdep->vq)
+    return gdbarch;
+
+  /* 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) 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 (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.target_desc = aarch64_read_description (features);
+  return gdbarch_find_by_info (info);
+}
+
 /* Implement the stack_frame_destroyed_p gdbarch method.  */
 
 static int
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index 55ccf2e777d2..80b9b3281a2d 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -143,4 +143,6 @@ void aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
 
 bool aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch);
 
+struct gdbarch *aarch64_update_gdbarch (struct gdbarch *gdbarch, uint64_t vq);
+
 #endif /* aarch64-tdep.h */

[PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Thiago Jung Bauermann]

If the remote target provides an expedited VG register, use it to update
the thread's gdbarch. This allows debugging programs that change their SVE
vector length during runtime.

This is accomplished by implementing the thread_architecture method in
remote_target, which returns the gdbarch corresponding to the expedited
registers provided by the last stop reply.

To allow changing the architecture based on the expedited registers, add a
new gdbarch method to allow arch-specific code to make the adjustment.
---
 gdb/aarch64-tdep.c        | 26 +++++++++++++++++++++++-
 gdb/arch-utils.c          |  9 +++++++++
 gdb/arch-utils.h          |  5 +++++
 gdb/gdbarch-components.py | 16 +++++++++++++++
 gdb/gdbarch-gen.h         | 11 ++++++++++
 gdb/gdbarch.c             | 22 ++++++++++++++++++++
 gdb/remote.c              | 42 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 130 insertions(+), 1 deletion(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index ffc128d91f60..764f693aee8d 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3486,7 +3486,8 @@ aarch64_cannot_store_register (struct gdbarch *gdbarch, int regnum)
 	  || regnum == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base));
 }
 
-/* Helper function for the "thread_architecture" target_ops method.
+/* Helper function for both the "update_architecture" gdbarch method and the
+   "thread_architecture" target_ops method.
 
    Returns a new gdbarch that is equivalent to the given gdbarch, but with SVE
    registers reflecting the given vq value.  */
@@ -3521,6 +3522,28 @@ aarch64_update_gdbarch (struct gdbarch *gdbarch, uint64_t vq)
   return gdbarch_find_by_info (info);
 }
 
+/* Implement the "update_architecture" gdbarch method.  */
+
+static struct gdbarch *
+aarch64_update_architecture (struct gdbarch *gdbarch,
+			     const std::vector<cached_reg_t> &regcache)
+{
+  /* Look for the VG register.  */
+  auto it = find_if (regcache.cbegin (), regcache.cend (),
+		     [] (const cached_reg_t &reg) {
+		       return reg.num == AARCH64_SVE_VG_REGNUM;
+		     });
+
+  /* No VG register was provided.  Don't change gdbarch.  */
+  if (it == regcache.cend ())
+    return gdbarch;
+
+  ULONGEST vg = extract_unsigned_integer (it->data, 8,
+					  gdbarch_byte_order (gdbarch));
+
+  return aarch64_update_gdbarch (gdbarch, sve_vq_from_vg (vg));
+}
+
 /* Implement the stack_frame_destroyed_p gdbarch method.  */
 
 static int
@@ -3742,6 +3765,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_tdesc_pseudo_register_reggroup_p (gdbarch,
 					aarch64_pseudo_register_reggroup_p);
   set_gdbarch_cannot_store_register (gdbarch, aarch64_cannot_store_register);
+  set_gdbarch_update_architecture (gdbarch, aarch64_update_architecture);
 
   /* ABI */
   set_gdbarch_short_bit (gdbarch, 16);
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 7b84daf046e2..2315aacb4bbe 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1090,6 +1090,15 @@ default_get_return_buf_addr (struct type *val_type, frame_info_ptr cur_frame)
   return 0;
 }
 
+/* See arch-utils.h.  */
+
+struct gdbarch *
+default_update_architecture (struct gdbarch *gdbarch,
+			     const std::vector<cached_reg_t> &regcache)
+{
+  return gdbarch;
+}
+
 /* Non-zero if we want to trace architecture code.  */
 
 #ifndef GDBARCH_DEBUG
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 46018a6fbbb6..e68a91f6753d 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -304,4 +304,9 @@ extern void default_read_core_file_mappings
 /* Default implementation of gdbarch default_get_return_buf_addr method.  */
 extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch,
 					      frame_info_ptr cur_frame);
+
+/* Default implementation of gdbarch update_architecture method.  */
+extern struct gdbarch *
+default_update_architecture (struct gdbarch *gdbarch,
+			     const std::vector<cached_reg_t> &regcache);
 #endif /* ARCH_UTILS_H */
diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
index 9b688998a7bd..fbb32c5e5853 100644
--- a/gdb/gdbarch-components.py
+++ b/gdb/gdbarch-components.py
@@ -2698,3 +2698,19 @@ Read core file mappings
     predefault="default_read_core_file_mappings",
     invalid=False,
 )
+
+Method(
+    comment="""
+An architecture may change based on the current contents of its registers.
+For instance, the length of the vector registers in AArch64's Scalable Vector
+Extension is given by the contents of the VL pseudo-register.
+
+This method allows an architecture to provide a new gdbarch corresponding to
+the registers in the given regcache.
+""",
+    type="struct gdbarch *",
+    name="update_architecture",
+    params=[("const std::vector<cached_reg_t> &", "regcache")],
+    predefault="default_update_architecture",
+    invalid=False,
+)
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index a663316df166..a65699e7241f 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1649,3 +1649,14 @@ extern void set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, gdbarch_g
 typedef void (gdbarch_read_core_file_mappings_ftype) (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb);
 extern void gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb);
 extern void set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, gdbarch_read_core_file_mappings_ftype *read_core_file_mappings);
+
+/* An architecture may change based on the current contents of its registers.
+   For instance, the length of the vector registers in AArch64's Scalable Vector
+   Extension is given by the contents of the VL pseudo-register.
+
+   This method allows an architecture to provide a new gdbarch corresponding to
+   the registers in the given regcache. */
+
+typedef struct gdbarch * (gdbarch_update_architecture_ftype) (struct gdbarch *gdbarch, const std::vector<cached_reg_t> &regcache);
+extern struct gdbarch * gdbarch_update_architecture (struct gdbarch *gdbarch, const std::vector<cached_reg_t> &regcache);
+extern void set_gdbarch_update_architecture (struct gdbarch *gdbarch, gdbarch_update_architecture_ftype *update_architecture);
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 3227e9458801..1b0a2c70db4c 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -255,6 +255,7 @@ struct gdbarch
   gdbarch_type_align_ftype *type_align = default_type_align;
   gdbarch_get_pc_address_flags_ftype *get_pc_address_flags = default_get_pc_address_flags;
   gdbarch_read_core_file_mappings_ftype *read_core_file_mappings = default_read_core_file_mappings;
+  gdbarch_update_architecture_ftype *update_architecture = default_update_architecture;
 };
 
 /* Create a new ``struct gdbarch'' based on information provided by
@@ -514,6 +515,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of type_align, invalid_p == 0 */
   /* Skip verify of get_pc_address_flags, invalid_p == 0 */
   /* Skip verify of read_core_file_mappings, invalid_p == 0 */
+  /* Skip verify of update_architecture, invalid_p == 0 */
   if (!log.empty ())
     internal_error (_("verify_gdbarch: the following are invalid ...%s"),
 		    log.c_str ());
@@ -1352,6 +1354,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   gdb_printf (file,
                       "gdbarch_dump: read_core_file_mappings = <%s>\n",
                       host_address_to_string (gdbarch->read_core_file_mappings));
+  gdb_printf (file,
+                      "gdbarch_dump: update_architecture = <%s>\n",
+                      host_address_to_string (gdbarch->update_architecture));
   if (gdbarch->dump_tdep != NULL)
     gdbarch->dump_tdep (gdbarch, file);
 }
@@ -5314,3 +5319,20 @@ set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch,
 {
   gdbarch->read_core_file_mappings = read_core_file_mappings;
 }
+
+struct gdbarch *
+gdbarch_update_architecture (struct gdbarch *gdbarch, const std::vector<cached_reg_t> &regcache)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->update_architecture != NULL);
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_update_architecture called\n");
+  return gdbarch->update_architecture (gdbarch, regcache);
+}
+
+void
+set_gdbarch_update_architecture (struct gdbarch *gdbarch,
+                                 gdbarch_update_architecture_ftype update_architecture)
+{
+  gdbarch->update_architecture = update_architecture;
+}
diff --git a/gdb/remote.c b/gdb/remote.c
index 5118ecd0a312..eb60ed51585b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -491,6 +491,8 @@ class remote_target : public process_stratum_target
   gdb::byte_vector thread_info_to_thread_handle (struct thread_info *tp)
 						 override;
 
+  struct gdbarch *thread_architecture (ptid_t ptid) override;
+
   void stop (ptid_t) override;
 
   void interrupt () override;
@@ -1150,6 +1152,10 @@ struct remote_thread_info : public private_thread_info
      to stop for a watchpoint.  */
   CORE_ADDR watch_data_address = 0;
 
+  /* The architecture corresponding to the expedited registers returned
+     in the last stop reply.  */
+  struct gdbarch *expedited_arch = nullptr;
+
   /* Get the thread's resume state.  */
   enum resume_state get_resume_state () const
   {
@@ -1168,6 +1174,8 @@ struct remote_thread_info : public private_thread_info
     m_resume_state = resume_state::RESUMED_PENDING_VCONT;
     m_resumed_pending_vcont_info.step = step;
     m_resumed_pending_vcont_info.sig = sig;
+
+    expedited_arch = nullptr;
   }
 
   /* Get the information this thread's pending vCont-resumption.
@@ -1185,6 +1193,8 @@ struct remote_thread_info : public private_thread_info
   void set_resumed ()
   {
     m_resume_state = resume_state::RESUMED;
+
+    expedited_arch = nullptr;
   }
 
 private:
@@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
       /* Expedited registers.  */
       if (!stop_reply->regcache.empty ())
 	{
+	  /* If GDB already knows about this thread, we can give the
+	     architecture-specific code a chance to update the gdbarch based on
+	     the expedited registers.  */
+	  if (find_thread_ptid (this, ptid) != nullptr)
+	    {
+	      stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
+							      stop_reply->regcache);
+
+	      /* Save stop_reply->arch so that it can be returned by the
+		 thread_architecture method.  */
+	      remote_thread_info *remote_thr = get_remote_thread_info (this,
+								       ptid);
+	      remote_thr->expedited_arch = stop_reply->arch;
+	    }
+
 	  struct regcache *regcache
 	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);
 
@@ -14382,6 +14407,23 @@ remote_target::thread_info_to_thread_handle (struct thread_info *tp)
   return priv->thread_handle;
 }
 
+struct gdbarch *
+remote_target::thread_architecture (ptid_t ptid)
+{
+  thread_info *thr = find_thread_ptid (this, ptid);
+  remote_thread_info *remote_thr = nullptr;
+
+  if (thr != nullptr)
+    remote_thr = get_remote_thread_info (thr);
+
+  if (remote_thr == nullptr || remote_thr->expedited_arch == nullptr)
+    /* The default thread_architecture implementation is the one from
+       process_stratum_target.  */
+    return process_stratum_target::thread_architecture(ptid);
+
+  return remote_thr->expedited_arch;
+}
+
 bool
 remote_target::can_async_p ()
 {

Re: [PATCH v2 2/6] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap

[Luis Machado]

Hi,

On 11/26/22 02:04, Thiago Jung Bauermann wrote:
> This patch doesn't change gdbserver behaviour, but after later changes are
> made it avoids a null pointer dereference when HWCAP needs to be obtained
> for a specific process while current_thread is nullptr.
> 
> Fixing linux_read_auxv, linux_get_hwcap and linux_get_hwcap2 to take a PID
> parameter seems more correct than setting current_thread in one particular
> code path.

I'm wondering if passing the pid is needed at all in gdbserver for the purposes of fetching the hwcap entries from auxv.

While current thread can be nullptr, I suppose current_process will never be nullptr if we have a valid inferior. And the auxv entries are per-process rather than per-thread.

There is a bit of a corner case when we have extended-remote without a live process and we need to fetch the hwcap bits to determine a particular feature to report via qSupported. But I suppose this is not what this change is trying to address, is it?

If not, it may be the case that we don't need these changes, and we can just use current_process.

Does that make sense?

> 
> Changes are propagated to allow passing the new parameter through the call
> chain.
> ---
>   gdbserver/linux-aarch64-low.cc |  7 ++++---
>   gdbserver/linux-arm-low.cc     |  2 +-
>   gdbserver/linux-low.cc         | 18 +++++++++---------
>   gdbserver/linux-low.h          |  9 ++++-----
>   gdbserver/linux-ppc-low.cc     |  6 +++---
>   gdbserver/linux-s390-low.cc    |  2 +-
>   gdbserver/netbsd-low.cc        |  4 +---
>   gdbserver/netbsd-low.h         |  2 +-
>   gdbserver/server.cc            |  2 +-
>   gdbserver/target.cc            |  4 ++--
>   gdbserver/target.h             |  2 +-
>   11 files changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index db5086962612..a6ed68f93029 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -823,12 +823,13 @@ aarch64_target::low_arch_setup ()
>     if (is_elf64)
>       {
>         struct aarch64_features features;
> +      int pid = pid_of (current_thread);
>   
>         features.vq = aarch64_sve_get_vq (tid);
>         /* A-profile PAC is 64-bit only.  */
> -      features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
> +      features.pauth = (linux_get_hwcap (pid, 8) & AARCH64_HWCAP_PACA);
>         /* A-profile MTE is 64-bit only.  */
> -      features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
> +      features.mte = linux_get_hwcap2 (pid, 8) & HWCAP2_MTE;
>         features.tls = true;
>   
>         current_process ()->tdesc = aarch64_linux_read_description (features);
> @@ -3299,7 +3300,7 @@ aarch64_target::supports_memory_tagging ()
>   #endif
>       }
>   
> -  return (linux_get_hwcap2 (8) & HWCAP2_MTE) != 0;
> +  return (linux_get_hwcap2 (pid_of (current_thread), 8) & HWCAP2_MTE) != 0;
>   }
>   
>   bool
> diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
> index a458b0f14a68..1420909626e8 100644
> --- a/gdbserver/linux-arm-low.cc
> +++ b/gdbserver/linux-arm-low.cc
> @@ -958,7 +958,7 @@ get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
>   static const struct target_desc *
>   arm_read_description (void)
>   {
> -  unsigned long arm_hwcap = linux_get_hwcap (4);
> +  unsigned long arm_hwcap = linux_get_hwcap (pid_of (current_thread), 4);
>   
>     if (arm_hwcap & HWCAP_IWMMXT)
>       return arm_linux_read_description (ARM_FP_TYPE_IWMMXT);
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index a896b37528b6..fc496275d6a3 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -5480,12 +5480,11 @@ linux_process_target::supports_read_auxv ()
>      to debugger memory starting at MYADDR.  */
>   
>   int
> -linux_process_target::read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> -				 unsigned int len)
> +linux_process_target::read_auxv (int pid, CORE_ADDR offset,
> +				 unsigned char *myaddr, unsigned int len)
>   {
>     char filename[PATH_MAX];
>     int fd, n;
> -  int pid = lwpid_of (current_thread);
>   
>     xsnprintf (filename, sizeof filename, "/proc/%d/auxv", pid);
>   
> @@ -6979,14 +6978,15 @@ linux_get_pc_64bit (struct regcache *regcache)
>   /* See linux-low.h.  */
>   
>   int
> -linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
> +linux_get_auxv (int pid, int wordsize, CORE_ADDR match, CORE_ADDR *valp)
>   {
>     gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
>     int offset = 0;
>   
>     gdb_assert (wordsize == 4 || wordsize == 8);
>   
> -  while (the_target->read_auxv (offset, data, 2 * wordsize) == 2 * wordsize)
> +  while (the_target->read_auxv (pid, offset, data, 2 * wordsize)
> +	 == 2 * wordsize)
>       {
>         if (wordsize == 4)
>   	{
> @@ -7016,20 +7016,20 @@ linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
>   /* See linux-low.h.  */
>   
>   CORE_ADDR
> -linux_get_hwcap (int wordsize)
> +linux_get_hwcap (int pid, int wordsize)
>   {
>     CORE_ADDR hwcap = 0;
> -  linux_get_auxv (wordsize, AT_HWCAP, &hwcap);
> +  linux_get_auxv (pid, wordsize, AT_HWCAP, &hwcap);
>     return hwcap;
>   }
>   
>   /* See linux-low.h.  */
>   
>   CORE_ADDR
> -linux_get_hwcap2 (int wordsize)
> +linux_get_hwcap2 (int pid, int wordsize)
>   {
>     CORE_ADDR hwcap2 = 0;
> -  linux_get_auxv (wordsize, AT_HWCAP2, &hwcap2);
> +  linux_get_auxv (pid, wordsize, AT_HWCAP2, &hwcap2);
>     return hwcap2;
>   }
>   
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 1594f063f47c..182a540f3bb3 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -178,7 +178,7 @@ class linux_process_target : public process_stratum_target
>   
>     bool supports_read_auxv () override;
>   
> -  int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> +  int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
>   		 unsigned int len) override;
>   
>     int insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
> @@ -946,17 +946,16 @@ extern int have_ptrace_getregset;
>      *VALP and return 1.  If not found or if there is an error, return
>      0.  */
>   
> -int linux_get_auxv (int wordsize, CORE_ADDR match,
> -		    CORE_ADDR *valp);
> +int linux_get_auxv (int pid, int wordsize, CORE_ADDR match, CORE_ADDR *valp);
>   
>   /* Fetch the AT_HWCAP entry from the auxv vector, where entries are length
>      WORDSIZE.  If no entry was found, return zero.  */
>   
> -CORE_ADDR linux_get_hwcap (int wordsize);
> +CORE_ADDR linux_get_hwcap (int pid, int wordsize);
>   
>   /* Fetch the AT_HWCAP2 entry from the auxv vector, where entries are length
>      WORDSIZE.  If no entry was found, return zero.  */
>   
> -CORE_ADDR linux_get_hwcap2 (int wordsize);
> +CORE_ADDR linux_get_hwcap2 (int pid, int wordsize);
>   
>   #endif /* GDBSERVER_LINUX_LOW_H */
> diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc
> index 08824887003b..1db20d998ffd 100644
> --- a/gdbserver/linux-ppc-low.cc
> +++ b/gdbserver/linux-ppc-low.cc
> @@ -894,8 +894,8 @@ ppc_target::low_arch_setup ()
>   
>     /* The value of current_process ()->tdesc needs to be set for this
>        call.  */
> -  ppc_hwcap = linux_get_hwcap (features.wordsize);
> -  ppc_hwcap2 = linux_get_hwcap2 (features.wordsize);
> +  ppc_hwcap = linux_get_hwcap (pid_of (current_thread), features.wordsize);
> +  ppc_hwcap2 = linux_get_hwcap2 (pid_of (current_thread), features.wordsize);
>   
>     features.isa205 = ppc_linux_has_isa205 (ppc_hwcap);
>   
> @@ -1097,7 +1097,7 @@ is_elfv2_inferior (void)
>     const struct target_desc *tdesc = current_process ()->tdesc;
>     int wordsize = register_size (tdesc, 0);
>   
> -  if (!linux_get_auxv (wordsize, AT_PHDR, &phdr))
> +  if (!linux_get_auxv (pid_of (current_thread), wordsize, AT_PHDR, &phdr))
>       return def_res;
>   
>     /* Assume ELF header is at the beginning of the page where program headers
> diff --git a/gdbserver/linux-s390-low.cc b/gdbserver/linux-s390-low.cc
> index 5adc28070574..2d030a806350 100644
> --- a/gdbserver/linux-s390-low.cc
> +++ b/gdbserver/linux-s390-low.cc
> @@ -592,7 +592,7 @@ s390_target::low_arch_setup ()
>     /* Determine word size and HWCAP.  */
>     int pid = pid_of (current_thread);
>     int wordsize = s390_get_wordsize (pid);
> -  unsigned long hwcap = linux_get_hwcap (wordsize);
> +  unsigned long hwcap = linux_get_hwcap (pid, wordsize);
>   
>     /* Check whether the kernel supports extra register sets.  */
>     int have_regset_last_break
> diff --git a/gdbserver/netbsd-low.cc b/gdbserver/netbsd-low.cc
> index f05bcd4e173f..03af80049457 100644
> --- a/gdbserver/netbsd-low.cc
> +++ b/gdbserver/netbsd-low.cc
> @@ -581,11 +581,9 @@ netbsd_read_auxv(pid_t pid, void *offs, void *addr, size_t len)
>      to debugger memory starting at MYADDR.  */
>   
>   int
> -netbsd_process_target::read_auxv (CORE_ADDR offset,
> +netbsd_process_target::read_auxv (int pid, CORE_ADDR offset,
>   				  unsigned char *myaddr, unsigned int len)
>   {
> -  pid_t pid = pid_of (current_thread);
> -
>     return netbsd_read_auxv (pid, (void *) (intptr_t) offset, myaddr, len);
>   }
>   
> diff --git a/gdbserver/netbsd-low.h b/gdbserver/netbsd-low.h
> index 06b4fd613571..eced7b3dd860 100644
> --- a/gdbserver/netbsd-low.h
> +++ b/gdbserver/netbsd-low.h
> @@ -77,7 +77,7 @@ class netbsd_process_target : public process_stratum_target
>   
>     bool supports_read_auxv () override;
>   
> -  int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> +  int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
>   		 unsigned int len) override;
>   
>     bool supports_hardware_single_step () override;
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index aaef38e00622..63c323071670 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -1443,7 +1443,7 @@ handle_qxfer_auxv (const char *annex,
>     if (annex[0] != '\0' || current_thread == NULL)
>       return -1;
>   
> -  return the_target->read_auxv (offset, readbuf, len);
> +  return the_target->read_auxv (pid_of (current_thread), offset, readbuf, len);
>   }
>   
>   /* Handle qXfer:exec-file:read.  */
> diff --git a/gdbserver/target.cc b/gdbserver/target.cc
> index c06a67600b1f..3651dbb9e862 100644
> --- a/gdbserver/target.cc
> +++ b/gdbserver/target.cc
> @@ -344,8 +344,8 @@ process_stratum_target::supports_read_auxv ()
>   }
>   
>   int
> -process_stratum_target::read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> -				   unsigned int len)
> +process_stratum_target::read_auxv (int pid, CORE_ADDR offset,
> +				   unsigned char *myaddr, unsigned int len)
>   {
>     gdb_assert_not_reached ("target op read_auxv not supported");
>   }
> diff --git a/gdbserver/target.h b/gdbserver/target.h
> index 18ab969dda70..fe7a80b645bc 100644
> --- a/gdbserver/target.h
> +++ b/gdbserver/target.h
> @@ -175,7 +175,7 @@ class process_stratum_target
>     /* Read auxiliary vector data from the inferior process.
>   
>        Read LEN bytes at OFFSET into a buffer at MYADDR.  */
> -  virtual int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> +  virtual int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
>   			 unsigned int len);
>   
>     /* Returns true if GDB Z breakpoint type TYPE is supported, false

Re: [PATCH v2 1/6] gdbserver: Add asserts in register_size and register_data functions

[Luis Machado]

On 11/26/22 02:04, Thiago Jung Bauermann wrote:
> These helped me during development, catching bugs closer to when they
> actually happened.
> ---
>   gdbserver/regcache.cc | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 5cbcea978a05..14236069f712 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -286,6 +286,8 @@ register_cache_size (const struct target_desc *tdesc)
>   int
>   register_size (const struct target_desc *tdesc, int n)
>   {
> +  gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
> +
>     return find_register_by_number (tdesc, n).size / 8;
>   }
>   
> @@ -300,6 +302,8 @@ regcache_register_size (const struct regcache *regcache, int n)
>   static unsigned char *
>   register_data (const struct regcache *regcache, int n)
>   {
> +  gdb_assert(n >= 0 && n < regcache->tdesc->reg_defs.size());
> +
>     return (regcache->registers
>   	  + find_register_by_number (regcache->tdesc, n).offset / 8);
>   }

LGTM.

Reviewed-by: Luis Machado <luis.machado@arm.com>

Re: [PATCH v2 3/6] gdbserver/linux-aarch64: Factor out function to get aarch64_features

[Luis Machado]

On 11/26/22 02:04, Thiago Jung Bauermann wrote:
> It will be used in a subsequent commit.  There's no functional change.
> ---
>   gdbserver/linux-aarch64-low.cc | 33 +++++++++++++++++++++------------
>   1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index a6ed68f93029..cab4fc0a4674 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -652,6 +652,25 @@ aarch64_target::low_delete_process (arch_process_info *info)
>     xfree (info);
>   }
>   
> +/* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
> +#define AARCH64_HWCAP_PACA (1 << 30)
> +
> +static struct aarch64_features
> +aarch64_get_arch_features (const thread_info *thread)
> +{
> +  struct aarch64_features features;
> +  int pid = pid_of (thread);
> +
> +  features.vq = aarch64_sve_get_vq (lwpid_of (thread));
> +  /* A-profile PAC is 64-bit only.  */
> +  features.pauth = linux_get_hwcap (pid, 8) & AARCH64_HWCAP_PACA;
> +  /* A-profile MTE is 64-bit only.  */
> +  features.mte = linux_get_hwcap2 (pid, 8) & HWCAP2_MTE;
> +  features.tls = true;
> +
> +  return features;
> +}
> +
>   void
>   aarch64_target::low_new_thread (lwp_info *lwp)
>   {
> @@ -804,9 +823,6 @@ aarch64_adjust_register_sets (const struct aarch64_features &features)
>       }
>   }
>   
> -/* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
> -#define AARCH64_HWCAP_PACA (1 << 30)
> -
>   /* Implementation of linux target ops method "low_arch_setup".  */
>   
>   void
> @@ -822,15 +838,8 @@ aarch64_target::low_arch_setup ()
>   
>     if (is_elf64)
>       {
> -      struct aarch64_features features;
> -      int pid = pid_of (current_thread);
> -
> -      features.vq = aarch64_sve_get_vq (tid);
> -      /* A-profile PAC is 64-bit only.  */
> -      features.pauth = (linux_get_hwcap (pid, 8) & AARCH64_HWCAP_PACA);
> -      /* A-profile MTE is 64-bit only.  */
> -      features.mte = linux_get_hwcap2 (pid, 8) & HWCAP2_MTE;
> -      features.tls = true;
> +      struct aarch64_features features
> +	  = aarch64_get_arch_features (current_thread);
>   
>         current_process ()->tdesc = aarch64_linux_read_description (features);
>   

LGTM

Reviewed-by: Luis Machado <luis.machado@arm.com>

Re: [PATCH v2 4/6] gdbserver/linux-aarch64: When thread stops, update its target description

[Luis Machado]

Thanks for the patch.

On 11/26/22 02:04, Thiago Jung Bauermann wrote:
> This change allows aarch64-linux to support debugging programs where
> different threads have different SVE vector lengths.  It requires
> gdbserver to support different inferior threads having different target
> descriptions.
> 
> The arch_update_tdesc method is added to the linux_process_target class to
> allow aarch64-linux to probe the inferior's vq register and provide an
> updated thread target description reflecting the new vector length.
> 
> After this change, all targets except SVE-supporting aarch64-linux will
> still use per-process target descriptions.
> ---
>   gdbserver/gdbthread.h          |  2 ++
>   gdbserver/linux-aarch64-low.cc | 37 +++++++++++++++++++++++++++++++++-
>   gdbserver/linux-low.cc         | 18 +++++++++++++++++
>   gdbserver/linux-low.h          |  5 +++++
>   gdbserver/regcache.cc          | 11 +++++++---
>   gdbserver/tdesc.cc             |  3 +++
>   6 files changed, 72 insertions(+), 4 deletions(-)
> 
> diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> index 8b897e73d33b..47b44d03b8e0 100644
> --- a/gdbserver/gdbthread.h
> +++ b/gdbserver/gdbthread.h
> @@ -80,6 +80,8 @@ struct thread_info
>   
>     /* Branch trace target information for this thread.  */
>     struct btrace_target_info *btrace = nullptr;
> +
> +  const struct target_desc *tdesc = nullptr;

Should we add a bit of information on how this new field is used, through a comment?

>   };
>   
>   extern std::list<thread_info *> all_threads;
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index cab4fc0a4674..786ce4071279 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -99,6 +99,9 @@ protected:
>   
>     void low_arch_setup () override;
>   
> +  gdb::optional<const struct target_desc *>
> +    arch_update_tdesc (const thread_info *thread) override;
> +
>     bool low_cannot_fetch_register (int regno) override;
>   
>     bool low_cannot_store_register (int regno) override;
> @@ -184,6 +187,8 @@ struct arch_process_info
>        same for each thread, it is reasonable for the data to live here.
>        */
>     struct aarch64_debug_reg_state debug_reg_state;
> +
> +  bool has_sve;
>   };

Though obvious, adding a comment like "has_sve" in gdb/aarch64-tdep.h will clarify the use of this field.

>   
>   /* Return true if the size of register 0 is 8 byte.  */
> @@ -840,8 +845,16 @@ aarch64_target::low_arch_setup ()
>       {
>         struct aarch64_features features
>   	  = aarch64_get_arch_features (current_thread);
> +      const target_desc *tdesc = aarch64_linux_read_description (features);
>   
> -      current_process ()->tdesc = aarch64_linux_read_description (features);
> +      /* Only SVE-enabled inferiors need per-thread target descriptions.  */
> +      if (features.vq > 0)
> +	{
> +	  current_thread->tdesc = tdesc;
> +	  current_process ()->priv->arch_private->has_sve = true;
> +	}
> +
> +      current_process ()->tdesc = tdesc;
>   
>         /* Adjust the register sets we should use for this particular set of
>   	 features.  */
> @@ -853,6 +866,28 @@ aarch64_target::low_arch_setup ()
>     aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
>   }
>   
> +/* Implementation of linux target ops method "arch_update_tdesc".  */
> +
> +gdb::optional<const struct target_desc *>
> +aarch64_target::arch_update_tdesc (const thread_info *thread)
> +{
> +  /* Only processes using SVE need to update the thread's target description.  */
> +  if (!get_thread_process (thread)->priv->arch_private->has_sve)
> +    return {};
> +
> +  const struct aarch64_features features = aarch64_get_arch_features (thread);
> +  const struct target_desc *tdesc = aarch64_linux_read_description (features);
> +
> +  if (tdesc == thread->tdesc)
> +    return {};
> +
> +  /* Adjust the register sets we should use for this particular set of
> +     features.  */
> +  aarch64_adjust_register_sets(features);
> +
> +  return tdesc;
> +}
> +
>   /* Implementation of linux target ops method "get_regs_info".  */
>   
>   const regs_info *
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index fc496275d6a3..7c510e26f0f5 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -483,6 +483,12 @@ linux_process_target::arch_setup_thread (thread_info *thread)
>     low_arch_setup ();
>   }
>   
> +gdb::optional<const struct target_desc *>
> +linux_process_target::arch_update_tdesc (const thread_info *thread)
> +{
> +  return {};
> +}
> +
>   int
>   linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>   					    int wstat)
> @@ -2348,6 +2354,18 @@ linux_process_target::filter_event (int lwpid, int wstat)
>   	      return;
>   	    }
>   	}
> +      else
> +	{
> +	  /* Give the arch code an opportunity to update the thread's target
> +	     description.  */
> +	  gdb::optional<const struct target_desc *> new_tdesc
> +	      = arch_update_tdesc (thread);
> +	  if (new_tdesc.has_value ())
> +	    {
> +	      regcache_release ();
> +	      thread->tdesc = *new_tdesc;
> +	    }
> +	}
>       }
>   
>     if (WIFSTOPPED (wstat) && child->must_set_ptrace_flags)
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 182a540f3bb3..ff14423e9e07 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -604,6 +604,11 @@ class linux_process_target : public process_stratum_target
>     /* Architecture-specific setup for the current thread.  */
>     virtual void low_arch_setup () = 0;
>   
> +  /* Allows arch-specific code to update the thread's target description when
> +     the inferior stops.  */

I'd also mention sometimes we don't need to update the target description if nothing's
changed. So an empty return is expected.

> +  virtual gdb::optional<const struct target_desc *>
> +    arch_update_tdesc (const thread_info *thread);
> +
>     /* Return false if we can fetch/store the register, true if we cannot
>        fetch/store the register.  */
>     virtual bool low_cannot_fetch_register (int regno) = 0;
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 14236069f712..03ee88b3cfd1 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -39,11 +39,16 @@ get_thread_regcache (struct thread_info *thread, int fetch)
>        have.  */
>     if (regcache == NULL)
>       {
> -      struct process_info *proc = get_thread_process (thread);
> +      /* First see if there's a thread-specific target description.  */
> +      const target_desc *tdesc = thread->tdesc;
>   
> -      gdb_assert (proc->tdesc != NULL);
> +      /* If not, get it from the process instead.  */
> +      if (tdesc == nullptr)
> +	tdesc = get_thread_process (thread)->tdesc;

Just a suggestion. Your call.

We could abstract away trying to fetch a tdesc from a thread and then from a process by having a function "get_tdesc ()" and calling it here.

Possibly with a better name.

>   
> -      regcache = new_register_cache (proc->tdesc);
> +      gdb_assert (tdesc != nullptr);
> +
> +      regcache = new_register_cache (tdesc);
>         set_thread_regcache_data (thread, regcache);
>       }
>   
> diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
> index 5693cc6626fb..3665ab0540d5 100644
> --- a/gdbserver/tdesc.cc
> +++ b/gdbserver/tdesc.cc
> @@ -129,6 +129,9 @@ current_target_desc (void)
>     if (current_thread == NULL)
>       return &default_description;
>   
> +  if (current_thread->tdesc != nullptr)
> +    return current_thread->tdesc;
> +
>     return current_process ()->tdesc;

We'd use the above function in here as well.

>   }
>   

Otherwise LGTM.

Reviewed-by: Luis Machado <luis.machado@arm.com>

Re: [PATCH v2 5/6] gdb/aarch64: Factor out most of the thread_architecture method

[Luis Machado]

On 11/26/22 02:04, Thiago Jung Bauermann wrote:
> The same logic will be used by a subsequent commit when remotely debugging
> an aarch64-linux target.
> 
> The code isn't changed, just moved around.
> ---
>   gdb/aarch64-linux-nat.c | 28 ++--------------------------
>   gdb/aarch64-tdep.c      | 35 +++++++++++++++++++++++++++++++++++
>   gdb/aarch64-tdep.h      |  2 ++
>   3 files changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index caefcb364852..ca230ea4fdb0 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -884,33 +884,9 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>     /* Find the current gdbarch the same way as process_stratum_target.  */
>     inferior *inf = find_inferior_ptid (this, ptid);
>     gdb_assert (inf != NULL);
> -
> -  /* If this is a 32-bit architecture, then this is ARM, not AArch64.
> -     There's no SVE vectors here, so just return the inferior
> -     architecture.  */
> -  if (gdbarch_bfd_arch_info (inf->gdbarch)->bits_per_word == 32)
> -    return inf->gdbarch;
> -
> -  /* Only return it if the current vector length matches the one in the tdep.  */
> -  aarch64_gdbarch_tdep *tdep
> -    = gdbarch_tdep<aarch64_gdbarch_tdep> (inf->gdbarch);
>     uint64_t vq = aarch64_sve_get_vq (ptid.lwp ());
> -  if (vq == tdep->vq)
> -    return inf->gdbarch;
> -
> -  /* 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) 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.target_desc = aarch64_read_description (features);
> -  return gdbarch_find_by_info (info);
> +
> +  return aarch64_update_gdbarch (inf->gdbarch, vq);
>   }
>   
>   /* Implement the "supports_memory_tagging" target_ops method.  */
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 07330356fdcb..ffc128d91f60 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3486,6 +3486,41 @@ aarch64_cannot_store_register (struct gdbarch *gdbarch, int regnum)
>   	  || regnum == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base));
>   }
>   
> +/* Helper function for the "thread_architecture" target_ops method.
> +
> +   Returns a new gdbarch that is equivalent to the given gdbarch, but with SVE
> +   registers reflecting the given vq value.  */
> +
> +struct gdbarch *
> +aarch64_update_gdbarch (struct gdbarch *gdbarch, uint64_t vq)
> +{
> +  /* If this is a 32-bit architecture, then this is ARM, not AArch64.
> +     There's no SVE vectors here, so just return the inferior
> +     architecture.  */
> +  if (gdbarch_bfd_arch_info (gdbarch)->bits_per_word == 32)
> +    return gdbarch;
> +
> +  /* Only return it if the current vector length matches the one in the
> +     tdep.  */
> +  aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
> +  if (vq == tdep->vq)
> +    return gdbarch;
> +
> +  /* 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) 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 (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.target_desc = aarch64_read_description (features);
> +  return gdbarch_find_by_info (info);
> +}
> +
>   /* Implement the stack_frame_destroyed_p gdbarch method.  */
>   
>   static int
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index 55ccf2e777d2..80b9b3281a2d 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -143,4 +143,6 @@ void aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
>   
>   bool aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch);
>   
> +struct gdbarch *aarch64_update_gdbarch (struct gdbarch *gdbarch, uint64_t vq);
> +
>   #endif /* aarch64-tdep.h */

LGTM.

Approved-by: Luis Machado <luis.machado@arm.com>

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Luis Machado]

On 11/26/22 02:04, Thiago Jung Bauermann wrote:
> If the remote target provides an expedited VG register, use it to update
> the thread's gdbarch. This allows debugging programs that change their SVE
> vector length during runtime.

debugging programs -> debugging programs remotely
> 
> This is accomplished by implementing the thread_architecture method in
> remote_target, which returns the gdbarch corresponding to the expedited
> registers provided by the last stop reply.
> 
> To allow changing the architecture based on the expedited registers, add a
> new gdbarch method to allow arch-specific code to make the adjustment.
> ---
>   gdb/aarch64-tdep.c        | 26 +++++++++++++++++++++++-
>   gdb/arch-utils.c          |  9 +++++++++
>   gdb/arch-utils.h          |  5 +++++
>   gdb/gdbarch-components.py | 16 +++++++++++++++
>   gdb/gdbarch-gen.h         | 11 ++++++++++
>   gdb/gdbarch.c             | 22 ++++++++++++++++++++
>   gdb/remote.c              | 42 +++++++++++++++++++++++++++++++++++++++
>   7 files changed, 130 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index ffc128d91f60..764f693aee8d 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3486,7 +3486,8 @@ aarch64_cannot_store_register (struct gdbarch *gdbarch, int regnum)
>   	  || regnum == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base));
>   }
>   
> -/* Helper function for the "thread_architecture" target_ops method.
> +/* Helper function for both the "update_architecture" gdbarch method and the
> +   "thread_architecture" target_ops method.
>   
>      Returns a new gdbarch that is equivalent to the given gdbarch, but with SVE
>      registers reflecting the given vq value.  */
> @@ -3521,6 +3522,28 @@ aarch64_update_gdbarch (struct gdbarch *gdbarch, uint64_t vq)
>     return gdbarch_find_by_info (info);
>   }
>   
> +/* Implement the "update_architecture" gdbarch method.  */
> +
> +static struct gdbarch *
> +aarch64_update_architecture (struct gdbarch *gdbarch,
> +			     const std::vector<cached_reg_t> &regcache)
> +{
> +  /* Look for the VG register.  */
> +  auto it = find_if (regcache.cbegin (), regcache.cend (),
> +		     [] (const cached_reg_t &reg) {
> +		       return reg.num == AARCH64_SVE_VG_REGNUM;
> +		     });
> +
> +  /* No VG register was provided.  Don't change gdbarch.  */
> +  if (it == regcache.cend ())
> +    return gdbarch;
> +
> +  ULONGEST vg = extract_unsigned_integer (it->data, 8,
> +					  gdbarch_byte_order (gdbarch));
> +
> +  return aarch64_update_gdbarch (gdbarch, sve_vq_from_vg (vg));
> +}
> +
>   /* Implement the stack_frame_destroyed_p gdbarch method.  */
>   
>   static int
> @@ -3742,6 +3765,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>     set_tdesc_pseudo_register_reggroup_p (gdbarch,
>   					aarch64_pseudo_register_reggroup_p);
>     set_gdbarch_cannot_store_register (gdbarch, aarch64_cannot_store_register);
> +  set_gdbarch_update_architecture (gdbarch, aarch64_update_architecture);
>   
>     /* ABI */
>     set_gdbarch_short_bit (gdbarch, 16);
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 7b84daf046e2..2315aacb4bbe 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1090,6 +1090,15 @@ default_get_return_buf_addr (struct type *val_type, frame_info_ptr cur_frame)
>     return 0;
>   }
>   
> +/* See arch-utils.h.  */
> +
> +struct gdbarch *
> +default_update_architecture (struct gdbarch *gdbarch,
> +			     const std::vector<cached_reg_t> &regcache)
> +{
> +  return gdbarch;
> +}
> +
>   /* Non-zero if we want to trace architecture code.  */
>   
>   #ifndef GDBARCH_DEBUG
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 46018a6fbbb6..e68a91f6753d 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -304,4 +304,9 @@ extern void default_read_core_file_mappings
>   /* Default implementation of gdbarch default_get_return_buf_addr method.  */
>   extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch,
>   					      frame_info_ptr cur_frame);
> +
> +/* Default implementation of gdbarch update_architecture method.  */
> +extern struct gdbarch *
> +default_update_architecture (struct gdbarch *gdbarch,
> +			     const std::vector<cached_reg_t> &regcache);
>   #endif /* ARCH_UTILS_H */
> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
> index 9b688998a7bd..fbb32c5e5853 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -2698,3 +2698,19 @@ Read core file mappings
>       predefault="default_read_core_file_mappings",
>       invalid=False,
>   )
> +
> +Method(
> +    comment="""
> +An architecture may change based on the current contents of its registers.
> +For instance, the length of the vector registers in AArch64's Scalable Vector
> +Extension is given by the contents of the VL pseudo-register.

VL -> VG

> +
> +This method allows an architecture to provide a new gdbarch corresponding to
> +the registers in the given regcache.
> +""",
> +    type="struct gdbarch *",
> +    name="update_architecture",
> +    params=[("const std::vector<cached_reg_t> &", "regcache")],
> +    predefault="default_update_architecture",
> +    invalid=False,
> +)
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index a663316df166..a65699e7241f 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1649,3 +1649,14 @@ extern void set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, gdbarch_g
>   typedef void (gdbarch_read_core_file_mappings_ftype) (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb);
>   extern void gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb);
>   extern void set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, gdbarch_read_core_file_mappings_ftype *read_core_file_mappings);
> +
> +/* An architecture may change based on the current contents of its registers.
> +   For instance, the length of the vector registers in AArch64's Scalable Vector
> +   Extension is given by the contents of the VL pseudo-register.
> +
> +   This method allows an architecture to provide a new gdbarch corresponding to
> +   the registers in the given regcache. */
> +
> +typedef struct gdbarch * (gdbarch_update_architecture_ftype) (struct gdbarch *gdbarch, const std::vector<cached_reg_t> &regcache);
> +extern struct gdbarch * gdbarch_update_architecture (struct gdbarch *gdbarch, const std::vector<cached_reg_t> &regcache);
> +extern void set_gdbarch_update_architecture (struct gdbarch *gdbarch, gdbarch_update_architecture_ftype *update_architecture);
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 3227e9458801..1b0a2c70db4c 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -255,6 +255,7 @@ struct gdbarch
>     gdbarch_type_align_ftype *type_align = default_type_align;
>     gdbarch_get_pc_address_flags_ftype *get_pc_address_flags = default_get_pc_address_flags;
>     gdbarch_read_core_file_mappings_ftype *read_core_file_mappings = default_read_core_file_mappings;
> +  gdbarch_update_architecture_ftype *update_architecture = default_update_architecture;
>   };
>   
>   /* Create a new ``struct gdbarch'' based on information provided by
> @@ -514,6 +515,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>     /* Skip verify of type_align, invalid_p == 0 */
>     /* Skip verify of get_pc_address_flags, invalid_p == 0 */
>     /* Skip verify of read_core_file_mappings, invalid_p == 0 */
> +  /* Skip verify of update_architecture, invalid_p == 0 */
>     if (!log.empty ())
>       internal_error (_("verify_gdbarch: the following are invalid ...%s"),
>   		    log.c_str ());
> @@ -1352,6 +1354,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>     gdb_printf (file,
>                         "gdbarch_dump: read_core_file_mappings = <%s>\n",
>                         host_address_to_string (gdbarch->read_core_file_mappings));
> +  gdb_printf (file,
> +                      "gdbarch_dump: update_architecture = <%s>\n",
> +                      host_address_to_string (gdbarch->update_architecture));
>     if (gdbarch->dump_tdep != NULL)
>       gdbarch->dump_tdep (gdbarch, file);
>   }
> @@ -5314,3 +5319,20 @@ set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch,
>   {
>     gdbarch->read_core_file_mappings = read_core_file_mappings;
>   }
> +
> +struct gdbarch *
> +gdbarch_update_architecture (struct gdbarch *gdbarch, const std::vector<cached_reg_t> &regcache)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->update_architecture != NULL);
> +  if (gdbarch_debug >= 2)
> +    gdb_printf (gdb_stdlog, "gdbarch_update_architecture called\n");
> +  return gdbarch->update_architecture (gdbarch, regcache);
> +}
> +
> +void
> +set_gdbarch_update_architecture (struct gdbarch *gdbarch,
> +                                 gdbarch_update_architecture_ftype update_architecture)
> +{
> +  gdbarch->update_architecture = update_architecture;
> +}
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 5118ecd0a312..eb60ed51585b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -491,6 +491,8 @@ class remote_target : public process_stratum_target
>     gdb::byte_vector thread_info_to_thread_handle (struct thread_info *tp)
>   						 override;
>   
> +  struct gdbarch *thread_architecture (ptid_t ptid) override;
> +
>     void stop (ptid_t) override;
>   
>     void interrupt () override;
> @@ -1150,6 +1152,10 @@ struct remote_thread_info : public private_thread_info
>        to stop for a watchpoint.  */
>     CORE_ADDR watch_data_address = 0;
>   
> +  /* The architecture corresponding to the expedited registers returned
> +     in the last stop reply.  */
> +  struct gdbarch *expedited_arch = nullptr;
> +
>     /* Get the thread's resume state.  */
>     enum resume_state get_resume_state () const
>     {
> @@ -1168,6 +1174,8 @@ struct remote_thread_info : public private_thread_info
>       m_resume_state = resume_state::RESUMED_PENDING_VCONT;
>       m_resumed_pending_vcont_info.step = step;
>       m_resumed_pending_vcont_info.sig = sig;
> +
> +    expedited_arch = nullptr;
>     }
>   
>     /* Get the information this thread's pending vCont-resumption.
> @@ -1185,6 +1193,8 @@ struct remote_thread_info : public private_thread_info
>     void set_resumed ()
>     {
>       m_resume_state = resume_state::RESUMED;
> +
> +    expedited_arch = nullptr;
>     }
>   
>   private:
> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>         /* Expedited registers.  */
>         if (!stop_reply->regcache.empty ())
>   	{
> +	  /* If GDB already knows about this thread, we can give the
> +	     architecture-specific code a chance to update the gdbarch based on
> +	     the expedited registers.  */
> +	  if (find_thread_ptid (this, ptid) != nullptr)
> +	    {
> +	      stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
> +							      stop_reply->regcache);
> +
> +	      /* Save stop_reply->arch so that it can be returned by the
> +		 thread_architecture method.  */
> +	      remote_thread_info *remote_thr = get_remote_thread_info (this,
> +								       ptid);
> +	      remote_thr->expedited_arch = stop_reply->arch;
> +	    }
> +
>   	  struct regcache *regcache
>   	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);
>   
> @@ -14382,6 +14407,23 @@ remote_target::thread_info_to_thread_handle (struct thread_info *tp)
>     return priv->thread_handle;
>   }
>   
> +struct gdbarch *
> +remote_target::thread_architecture (ptid_t ptid)
> +{
> +  thread_info *thr = find_thread_ptid (this, ptid);
> +  remote_thread_info *remote_thr = nullptr;
> +
> +  if (thr != nullptr)
> +    remote_thr = get_remote_thread_info (thr);
> +
> +  if (remote_thr == nullptr || remote_thr->expedited_arch == nullptr)
> +    /* The default thread_architecture implementation is the one from
> +       process_stratum_target.  */
> +    return process_stratum_target::thread_architecture(ptid);
> +
> +  return remote_thr->expedited_arch;
> +}
> +
>   bool
>   remote_target::can_async_p ()
>   {

Just to confirm, as I don't exactly remember how this went. Is the case where we switch threads during remote debugging covered in case we have threads with different vector lengths?

I seem to recall we'd call thread_architecture in those cases, but I don't know how the expedited register comes into play for this case. Do we get expedited registers for each thread that has stopped?

Otherwise LGTM.

Reviewed-by: Luis Machado <luis.machado@arm.com>

Re: [PATCH v2 1/6] gdbserver: Add asserts in register_size and register_data functions

[Simon Marchi]

On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
> These helped me during development, catching bugs closer to when they
> actually happened.
> ---
>  gdbserver/regcache.cc | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 5cbcea978a05..14236069f712 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -286,6 +286,8 @@ register_cache_size (const struct target_desc *tdesc)
>  int
>  register_size (const struct target_desc *tdesc, int n)
>  {
> +  gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
> +
>    return find_register_by_number (tdesc, n).size / 8;
>  }
>  
> @@ -300,6 +302,8 @@ regcache_register_size (const struct regcache *regcache, int n)
>  static unsigned char *
>  register_data (const struct regcache *regcache, int n)
>  {
> +  gdb_assert(n >= 0 && n < regcache->tdesc->reg_defs.size());

Missing space before parenthesis.

I don't know if that would have helped you, but given that
find_register_by_number is implemented as an std::vector lookup, it
would probably have been caught if building with -D_GLIBCXX_DEBUG.  I
recommend using that for development, it's really handy.

https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH v2 1/6] gdbserver: Add asserts in register_size and register_data functions

[Simon Marchi]

On 11/28/22 09:48, Simon Marchi wrote:
> 
> 
> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
>> These helped me during development, catching bugs closer to when they
>> actually happened.
>> ---
>>  gdbserver/regcache.cc | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
>> index 5cbcea978a05..14236069f712 100644
>> --- a/gdbserver/regcache.cc
>> +++ b/gdbserver/regcache.cc
>> @@ -286,6 +286,8 @@ register_cache_size (const struct target_desc *tdesc)
>>  int
>>  register_size (const struct target_desc *tdesc, int n)
>>  {
>> +  gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
>> +
>>    return find_register_by_number (tdesc, n).size / 8;
>>  }
>>  
>> @@ -300,6 +302,8 @@ regcache_register_size (const struct regcache *regcache, int n)
>>  static unsigned char *
>>  register_data (const struct regcache *regcache, int n)
>>  {
>> +  gdb_assert(n >= 0 && n < regcache->tdesc->reg_defs.size());
> 
> Missing space before parenthesis.
> 
> I don't know if that would have helped you, but given that
> find_register_by_number is implemented as an std::vector lookup, it
> would probably have been caught if building with -D_GLIBCXX_DEBUG.  I
> recommend using that for development, it's really handy.
> 
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

Actually, I would perhaps suggest moving the assertion checks to
find_register_by_number, the place that actually accesses reg_defs.

And we could perhaps remove the equivalent gdb_assert in
regcache_raw_read_unsigned, since it's checking the same a few frames
above.

Simon

Re: [PATCH v2 2/6] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap

[Simon Marchi]

On 11/28/22 06:50, Luis Machado via Gdb-patches wrote:
> Hi,
> 
> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>> This patch doesn't change gdbserver behaviour, but after later changes are
>> made it avoids a null pointer dereference when HWCAP needs to be obtained
>> for a specific process while current_thread is nullptr.
>>
>> Fixing linux_read_auxv, linux_get_hwcap and linux_get_hwcap2 to take a PID
>> parameter seems more correct than setting current_thread in one particular
>> code path.
> 
> I'm wondering if passing the pid is needed at all in gdbserver for the purposes of fetching the hwcap entries from auxv.
> 
> While current thread can be nullptr, I suppose current_process will never be nullptr if we have a valid inferior. And the auxv entries are per-process rather than per-thread.
> 
> There is a bit of a corner case when we have extended-remote without a live process and we need to fetch the hwcap bits to determine a particular feature to report via qSupported. But I suppose this is not what this change is trying to address, is it?
> 
> If not, it may be the case that we don't need these changes, and we can just use current_process.

Even if we could just use current_process, this patch is going in the
general direction I like, which is to reduce referring to the global
state everywhere, try to fetch it only at the entry points and then pass
the necessary context down by parameters.

Simon

Re: [PATCH v2 2/6] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap

[Simon Marchi]

On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
> This patch doesn't change gdbserver behaviour, but after later changes are
> made it avoids a null pointer dereference when HWCAP needs to be obtained
> for a specific process while current_thread is nullptr.
> 
> Fixing linux_read_auxv, linux_get_hwcap and linux_get_hwcap2 to take a PID
> parameter seems more correct than setting current_thread in one particular
> code path.
> 
> Changes are propagated to allow passing the new parameter through the call
> chain.

Thanks, this is ok, with the following nits fixed:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

... unless Luis is strongly against it.

> ---
>  gdbserver/linux-aarch64-low.cc |  7 ++++---
>  gdbserver/linux-arm-low.cc     |  2 +-
>  gdbserver/linux-low.cc         | 18 +++++++++---------
>  gdbserver/linux-low.h          |  9 ++++-----
>  gdbserver/linux-ppc-low.cc     |  6 +++---
>  gdbserver/linux-s390-low.cc    |  2 +-
>  gdbserver/netbsd-low.cc        |  4 +---
>  gdbserver/netbsd-low.h         |  2 +-
>  gdbserver/server.cc            |  2 +-
>  gdbserver/target.cc            |  4 ++--
>  gdbserver/target.h             |  2 +-
>  11 files changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index db5086962612..a6ed68f93029 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -823,12 +823,13 @@ aarch64_target::low_arch_setup ()
>    if (is_elf64)
>      {
>        struct aarch64_features features;
> +      int pid = pid_of (current_thread);
>  
>        features.vq = aarch64_sve_get_vq (tid);
>        /* A-profile PAC is 64-bit only.  */
> -      features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
> +      features.pauth = (linux_get_hwcap (pid, 8) & AARCH64_HWCAP_PACA);

Avoid adding the outer parenthesis.

> diff --git a/gdbserver/target.h b/gdbserver/target.h
> index 18ab969dda70..fe7a80b645bc 100644
> --- a/gdbserver/target.h
> +++ b/gdbserver/target.h
> @@ -175,7 +175,7 @@ class process_stratum_target
>    /* Read auxiliary vector data from the inferior process.
>  
>       Read LEN bytes at OFFSET into a buffer at MYADDR.  */
> -  virtual int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> +  virtual int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,

Please update the doc of the method to replace "from the inferior
process" to from "process with pid PID".

Simon

Re: [PATCH v2 3/6] gdbserver/linux-aarch64: Factor out function to get aarch64_features

[Simon Marchi]

On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
> It will be used in a subsequent commit.  There's no functional change.
> ---
>  gdbserver/linux-aarch64-low.cc | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index a6ed68f93029..cab4fc0a4674 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -652,6 +652,25 @@ aarch64_target::low_delete_process (arch_process_info *info)
>    xfree (info);
>  }
>  
> +/* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
> +#define AARCH64_HWCAP_PACA (1 << 30)
> +
> +static struct aarch64_features
> +aarch64_get_arch_features (const thread_info *thread)

Please document the new function.

> +  struct aarch64_features features;
> +  int pid = pid_of (thread);
> +
> +  features.vq = aarch64_sve_get_vq (lwpid_of (thread));

Just a note, I feel like the pid_of / lwpid_of functions (they used to
be macros) are a vestige of when GDB was in C.  Today, I would have no
problem doing:

  thread->id.pid ()
  thread->id.lwp ()

With the doc change done,

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH v2 2/6] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap

[Luis Machado]

On 11/28/22 15:07, Simon Marchi wrote:
> 
> 
> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
>> This patch doesn't change gdbserver behaviour, but after later changes are
>> made it avoids a null pointer dereference when HWCAP needs to be obtained
>> for a specific process while current_thread is nullptr.
>>
>> Fixing linux_read_auxv, linux_get_hwcap and linux_get_hwcap2 to take a PID
>> parameter seems more correct than setting current_thread in one particular
>> code path.
>>
>> Changes are propagated to allow passing the new parameter through the call
>> chain.
> 
> Thanks, this is ok, with the following nits fixed:
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> 
> ... unless Luis is strongly against it.

Not at all. Given gdbserver's knowledge of the current ptid is implicit (set by remote packets), I thought this would simplify the series a bit.

> 
>> ---
>>   gdbserver/linux-aarch64-low.cc |  7 ++++---
>>   gdbserver/linux-arm-low.cc     |  2 +-
>>   gdbserver/linux-low.cc         | 18 +++++++++---------
>>   gdbserver/linux-low.h          |  9 ++++-----
>>   gdbserver/linux-ppc-low.cc     |  6 +++---
>>   gdbserver/linux-s390-low.cc    |  2 +-
>>   gdbserver/netbsd-low.cc        |  4 +---
>>   gdbserver/netbsd-low.h         |  2 +-
>>   gdbserver/server.cc            |  2 +-
>>   gdbserver/target.cc            |  4 ++--
>>   gdbserver/target.h             |  2 +-
>>   11 files changed, 28 insertions(+), 30 deletions(-)
>>
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index db5086962612..a6ed68f93029 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -823,12 +823,13 @@ aarch64_target::low_arch_setup ()
>>     if (is_elf64)
>>       {
>>         struct aarch64_features features;
>> +      int pid = pid_of (current_thread);
>>   
>>         features.vq = aarch64_sve_get_vq (tid);
>>         /* A-profile PAC is 64-bit only.  */
>> -      features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
>> +      features.pauth = (linux_get_hwcap (pid, 8) & AARCH64_HWCAP_PACA);
> 
> Avoid adding the outer parenthesis.
> 
>> diff --git a/gdbserver/target.h b/gdbserver/target.h
>> index 18ab969dda70..fe7a80b645bc 100644
>> --- a/gdbserver/target.h
>> +++ b/gdbserver/target.h
>> @@ -175,7 +175,7 @@ class process_stratum_target
>>     /* Read auxiliary vector data from the inferior process.
>>   
>>        Read LEN bytes at OFFSET into a buffer at MYADDR.  */
>> -  virtual int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
>> +  virtual int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
> 
> Please update the doc of the method to replace "from the inferior
> process" to from "process with pid PID".
> 
> Simon

Re: [PATCH v2 4/6] gdbserver/linux-aarch64: When thread stops, update its target description

[Simon Marchi]

> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index cab4fc0a4674..786ce4071279 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -99,6 +99,9 @@ protected:
>  
>    void low_arch_setup () override;
>  
> +  gdb::optional<const struct target_desc *>
> +    arch_update_tdesc (const thread_info *thread) override;

I'm all for using optional to be clear and explicit in general, but
unless an empty optional and an optional wrapping nullptr are both valid
and have different meanings (which doesn't seem, to be the case here), I
wouldn't recommend wrapping a pointer in an optional.

Pointers have a dedicated value for "no value", that is well understood
by everyone.  And then, it does create the possibility of returning an
optional wrapping nullptr, which typically won't be a legitimate value.
So it's just one more thing to worry about.

> @@ -840,8 +845,16 @@ aarch64_target::low_arch_setup ()
>      {
>        struct aarch64_features features
>  	  = aarch64_get_arch_features (current_thread);
> +      const target_desc *tdesc = aarch64_linux_read_description (features);
>  
> -      current_process ()->tdesc = aarch64_linux_read_description (features);
> +      /* Only SVE-enabled inferiors need per-thread target descriptions.  */
> +      if (features.vq > 0)
> +	{
> +	  current_thread->tdesc = tdesc;
> +	  current_process ()->priv->arch_private->has_sve = true;
> +	}

Is it not possible for a process to start with SVE disabled (vq == 0) and
have some threads enable it later?

> @@ -853,6 +866,28 @@ aarch64_target::low_arch_setup ()
>    aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
>  }
>  
> +/* Implementation of linux target ops method "arch_update_tdesc".  */
> +
> +gdb::optional<const struct target_desc *>
> +aarch64_target::arch_update_tdesc (const thread_info *thread)
> +{
> +  /* Only processes using SVE need to update the thread's target description.  */
> +  if (!get_thread_process (thread)->priv->arch_private->has_sve)
> +    return {};
> +
> +  const struct aarch64_features features = aarch64_get_arch_features (thread);
> +  const struct target_desc *tdesc = aarch64_linux_read_description (features);
> +
> +  if (tdesc == thread->tdesc)
> +    return {};
> +
> +  /* Adjust the register sets we should use for this particular set of
> +     features.  */
> +  aarch64_adjust_register_sets(features);
> +
> +  return tdesc;

Naming nit: I don't think we need "update" in the method name, there's
no "updating component" to it AFAICT.  It's basically "get this thread's
tdesc if for some reason it differents from the process-wide we have".
So, I don't know, "get_thread_tdesc" or just "thread_tdesc".

> @@ -2348,6 +2354,18 @@ linux_process_target::filter_event (int lwpid, int wstat)
>  	      return;
>  	    }
>  	}
> +      else
> +	{
> +	  /* Give the arch code an opportunity to update the thread's target
> +	     description.  */
> +	  gdb::optional<const struct target_desc *> new_tdesc
> +	      = arch_update_tdesc (thread);
> +	  if (new_tdesc.has_value ())
> +	    {
> +	      regcache_release ();

Hmm, regcache_release frees the regcache for all threads.  Can we free
the regcache only for this thread?

Simon

Re: [PATCH v2 4/6] gdbserver/linux-aarch64: When thread stops, update its target description

[Luis Machado]

On 11/28/22 15:47, Simon Marchi wrote:
> 
> 
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index cab4fc0a4674..786ce4071279 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -99,6 +99,9 @@ protected:
>>   
>>     void low_arch_setup () override;
>>   
>> +  gdb::optional<const struct target_desc *>
>> +    arch_update_tdesc (const thread_info *thread) override;
> 
> I'm all for using optional to be clear and explicit in general, but
> unless an empty optional and an optional wrapping nullptr are both valid
> and have different meanings (which doesn't seem, to be the case here), I
> wouldn't recommend wrapping a pointer in an optional.
> 
> Pointers have a dedicated value for "no value", that is well understood
> by everyone.  And then, it does create the possibility of returning an
> optional wrapping nullptr, which typically won't be a legitimate value.
> So it's just one more thing to worry about.
> 
>> @@ -840,8 +845,16 @@ aarch64_target::low_arch_setup ()
>>       {
>>         struct aarch64_features features
>>   	  = aarch64_get_arch_features (current_thread);
>> +      const target_desc *tdesc = aarch64_linux_read_description (features);
>>   
>> -      current_process ()->tdesc = aarch64_linux_read_description (features);
>> +      /* Only SVE-enabled inferiors need per-thread target descriptions.  */
>> +      if (features.vq > 0)
>> +	{
>> +	  current_thread->tdesc = tdesc;
>> +	  current_process ()->priv->arch_private->has_sve = true;
>> +	}
> 
> Is it not possible for a process to start with SVE disabled (vq == 0) and
> have some threads enable it later?

No. When supported, SVE is never disabled. It will always have a valid VL value, which is > 0 and restricted to a certain set of values depending on the hardware.

What does happen is the SVE state starts up as inactive, so we have neon vector data instead. But the SVE registers will always be there and the VL will always be valid.

> 
>> @@ -853,6 +866,28 @@ aarch64_target::low_arch_setup ()
>>     aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
>>   }
>>   
>> +/* Implementation of linux target ops method "arch_update_tdesc".  */
>> +
>> +gdb::optional<const struct target_desc *>
>> +aarch64_target::arch_update_tdesc (const thread_info *thread)
>> +{
>> +  /* Only processes using SVE need to update the thread's target description.  */
>> +  if (!get_thread_process (thread)->priv->arch_private->has_sve)
>> +    return {};
>> +
>> +  const struct aarch64_features features = aarch64_get_arch_features (thread);
>> +  const struct target_desc *tdesc = aarch64_linux_read_description (features);
>> +
>> +  if (tdesc == thread->tdesc)
>> +    return {};
>> +
>> +  /* Adjust the register sets we should use for this particular set of
>> +     features.  */
>> +  aarch64_adjust_register_sets(features);
>> +
>> +  return tdesc;
> 
> Naming nit: I don't think we need "update" in the method name, there's
> no "updating component" to it AFAICT.  It's basically "get this thread's
> tdesc if for some reason it differents from the process-wide we have".
> So, I don't know, "get_thread_tdesc" or just "thread_tdesc".
> 
>> @@ -2348,6 +2354,18 @@ linux_process_target::filter_event (int lwpid, int wstat)
>>   	      return;
>>   	    }
>>   	}
>> +      else
>> +	{
>> +	  /* Give the arch code an opportunity to update the thread's target
>> +	     description.  */
>> +	  gdb::optional<const struct target_desc *> new_tdesc
>> +	      = arch_update_tdesc (thread);
>> +	  if (new_tdesc.has_value ())
>> +	    {
>> +	      regcache_release ();
> 
> Hmm, regcache_release frees the regcache for all threads.  Can we free
> the regcache only for this thread?
> 
> Simon

Re: [PATCH v2 4/6] gdbserver/linux-aarch64: When thread stops, update its target description

[Simon Marchi]

>> Is it not possible for a process to start with SVE disabled (vq == 0) and
>> have some threads enable it later?
> 
> No. When supported, SVE is never disabled. It will always have a valid VL value, which is > 0 and restricted to a certain set of values depending on the hardware.
> 
> What does happen is the SVE state starts up as inactive, so we have neon vector data instead. But the SVE registers will always be there and the VL will always be valid.

Thanks for clarifying.

Simon

Re: [PATCH v2 5/6] gdb/aarch64: Factor out most of the thread_architecture method

[Simon Marchi]

>  /* Implement the "supports_memory_tagging" target_ops method.  */
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 07330356fdcb..ffc128d91f60 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3486,6 +3486,41 @@ aarch64_cannot_store_register (struct gdbarch *gdbarch, int regnum)
>  	  || regnum == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base));
>  }
>  
> +/* Helper function for the "thread_architecture" target_ops method.
> +
> +   Returns a new gdbarch that is equivalent to the given gdbarch, but with SVE
> +   registers reflecting the given vq value.  */
> +
> +struct gdbarch *
> +aarch64_update_gdbarch (struct gdbarch *gdbarch, uint64_t vq)

Document this using:

/* See aarch64-tdep.h.  */

... and move the doc to the header file.

Otherwise:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Simon Marchi]

On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
> If the remote target provides an expedited VG register, use it to update
> the thread's gdbarch. This allows debugging programs that change their SVE
> vector length during runtime.
> 
> This is accomplished by implementing the thread_architecture method in
> remote_target, which returns the gdbarch corresponding to the expedited
> registers provided by the last stop reply.
> 
> To allow changing the architecture based on the expedited registers, add a
> new gdbarch method to allow arch-specific code to make the adjustment.

I get this when applying, can you address that?

Applying: gdb/aarch64: Detect vector length changes when debugging remotely
.git/rebase-apply/patch:164: indent with spaces.
                       "gdbarch_dump: update_architecture = <%s>\n",
.git/rebase-apply/patch:165: indent with spaces.
                       host_address_to_string (gdbarch->update_architecture));
.git/rebase-apply/patch:186: indent with spaces.
                                  gdbarch_update_architecture_ftype update_architecture)
warning: 3 lines add whitespace errors.

I think the idea makes sense.  Short of adding a packet for "get thread
architecture" or extending the stop reply format to add a note about a
thread having a special tdesc, I don't see another way to do this.  We
can't read the vq register using 'g', because to interpret the 'g'
result, we need to know the full architecture.  We wouldn't know for
sure the offset of vq in the reply.  By using expedited registers, we
just need to make sure vq's register number is stable.  I guess that is
the case?


> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>        /* Expedited registers.  */
>        if (!stop_reply->regcache.empty ())
>  	{
> +	  /* If GDB already knows about this thread, we can give the
> +	     architecture-specific code a chance to update the gdbarch based on
> +	     the expedited registers.  */
> +	  if (find_thread_ptid (this, ptid) != nullptr)
> +	    {
> +	      stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
> +							      stop_reply->regcache);
> +
> +	      /* Save stop_reply->arch so that it can be returned by the
> +		 thread_architecture method.  */
> +	      remote_thread_info *remote_thr = get_remote_thread_info (this,
> +								       ptid);
> +	      remote_thr->expedited_arch = stop_reply->arch;
> +	    }
> +
>  	  struct regcache *regcache
>  	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);


What happens with threads that are not yet known?  Imagine the process
starts with SVE == X, the main threads spawns a worker thread, the
worker thread updates its SVE to Y, and the worker thread hits a
breakpoint.  This is the first time we hear about that worker thread.
If we don't do a "gdbarch_update_architecture" for it and save it
somewhere, we'll never set the gdbarch with SVE == Y, will we?

Simon

Re: [PATCH v2 1/6] gdbserver: Add asserts in register_size and register_data functions

[Thiago Jung Bauermann]

Hello Simon,

Thank you for your review!

Simon Marchi <simark@simark.ca> writes:

> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
>> These helped me during development, catching bugs closer to when they
>> actually happened.
>> ---
>>  gdbserver/regcache.cc | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
>> index 5cbcea978a05..14236069f712 100644
>> --- a/gdbserver/regcache.cc
>> +++ b/gdbserver/regcache.cc
>> @@ -286,6 +286,8 @@ register_cache_size (const struct target_desc *tdesc)
>>  int
>>  register_size (const struct target_desc *tdesc, int n)
>>  {
>> +  gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
>> +
>>    return find_register_by_number (tdesc, n).size / 8;
>>  }
>>  
>> @@ -300,6 +302,8 @@ regcache_register_size (const struct regcache *regcache, int n)
>>  static unsigned char *
>>  register_data (const struct regcache *regcache, int n)
>>  {
>> +  gdb_assert(n >= 0 && n < regcache->tdesc->reg_defs.size());
>
> Missing space before parenthesis.
>
> I don't know if that would have helped you, but given that
> find_register_by_number is implemented as an std::vector lookup, it
> would probably have been caught if building with -D_GLIBCXX_DEBUG.  I
> recommend using that for development, it's really handy.
>
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html

Nice, I will start using that flag on my development builds. Thank you
for the hint!

-- 
Thiago

Re: [PATCH v2 1/6] gdbserver: Add asserts in register_size and register_data functions

[Thiago Jung Bauermann]

Simon Marchi <simark@simark.ca> writes:

> On 11/28/22 09:48, Simon Marchi wrote:
>> 
>> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>
> Actually, I would perhaps suggest moving the assertion checks to
> find_register_by_number, the place that actually accesses reg_defs.

Good idea. I'll do that in v3.

> And we could perhaps remove the equivalent gdb_assert in
> regcache_raw_read_unsigned, since it's checking the same a few frames
> above.

Good point. Will do.

-- 
Thiago

Re: [PATCH v2 1/6] gdbserver: Add asserts in register_size and register_data functions

[Thiago Jung Bauermann]

Hello Luis,

Thank you for the quick review!

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

> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>> These helped me during development, catching bugs closer to when they
>> actually happened.
>> ---
>>   gdbserver/regcache.cc | 4 ++++
>>   1 file changed, 4 insertions(+)
>> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
>> index 5cbcea978a05..14236069f712 100644
>> --- a/gdbserver/regcache.cc
>> +++ b/gdbserver/regcache.cc
>> @@ -286,6 +286,8 @@ register_cache_size (const struct target_desc *tdesc)
>>   int
>>   register_size (const struct target_desc *tdesc, int n)
>>   {
>> +  gdb_assert (n >= 0 && n < tdesc->reg_defs.size ());
>> +
>>     return find_register_by_number (tdesc, n).size / 8;
>>   }
>>   @@ -300,6 +302,8 @@ regcache_register_size (const struct regcache *regcache, int n)
>>   static unsigned char *
>>   register_data (const struct regcache *regcache, int n)
>>   {
>> +  gdb_assert(n >= 0 && n < regcache->tdesc->reg_defs.size());
>> +
>>     return (regcache->registers
>>   	  + find_register_by_number (regcache->tdesc, n).offset / 8);
>>   }
>
> LGTM.
>
> Reviewed-by: Luis Machado <luis.machado@arm.com>

With Simon's suggestion, this patch will be different in v3 so
unfortunately I won't be able to add this Reviewed-by.


-- 
Thiago

Re: [PATCH v2 2/6] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap

[Thiago Jung Bauermann]

Simon Marchi <simark@simark.ca> writes:

> On 11/28/22 06:50, Luis Machado via Gdb-patches wrote:
>> Hi,
>> 
>> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>>> This patch doesn't change gdbserver behaviour, but after later changes are
>>> made it avoids a null pointer dereference when HWCAP needs to be obtained
>>> for a specific process while current_thread is nullptr.
>>>
>>> Fixing linux_read_auxv, linux_get_hwcap and linux_get_hwcap2 to take a PID
>>> parameter seems more correct than setting current_thread in one particular
>>> code path.
>> 
>> I'm wondering if passing the pid is needed at all in gdbserver for the purposes of
>> fetching the hwcap entries from auxv.
>> 
>> While current thread can be nullptr, I suppose current_process will never be nullptr if
>> we have a valid inferior. And the auxv entries are per-process rather than per-thread.
>> 
>> There is a bit of a corner case when we have extended-remote without a live process and
>> we need to fetch the hwcap bits to determine a particular feature to report via
>> qSupported. But I suppose this is not what this change is trying to address, is it?
>> 
>> If not, it may be the case that we don't need these changes, and we can just use
>> current_process.
>
> Even if we could just use current_process, this patch is going in the
> general direction I like, which is to reduce referring to the global
> state everywhere, try to fetch it only at the entry points and then pass
> the necessary context down by parameters.

Thanks. As I looked into how to fix the problem I ran into, I thought
this was a nice cleanup that would solve my problem and couldn't resist
doing it. :-)

-- 
Thiago

Re: [PATCH v2 2/6] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap

[Thiago Jung Bauermann]

Simon Marchi <simark@simark.ca> writes:

> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
>> This patch doesn't change gdbserver behaviour, but after later changes are
>> made it avoids a null pointer dereference when HWCAP needs to be obtained
>> for a specific process while current_thread is nullptr.
>> 
>> Fixing linux_read_auxv, linux_get_hwcap and linux_get_hwcap2 to take a PID
>> parameter seems more correct than setting current_thread in one particular
>> code path.
>> 
>> Changes are propagated to allow passing the new parameter through the call
>> chain.
>
> Thanks, this is ok, with the following nits fixed:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>
> ... unless Luis is strongly against it.

Thanks!

v3 will have the nits fixed and also change uses of “pid_of (thread)” to
“thread->id.pid ()”.

>> ---
>>  gdbserver/linux-aarch64-low.cc |  7 ++++---
>>  gdbserver/linux-arm-low.cc     |  2 +-
>>  gdbserver/linux-low.cc         | 18 +++++++++---------
>>  gdbserver/linux-low.h          |  9 ++++-----
>>  gdbserver/linux-ppc-low.cc     |  6 +++---
>>  gdbserver/linux-s390-low.cc    |  2 +-
>>  gdbserver/netbsd-low.cc        |  4 +---
>>  gdbserver/netbsd-low.h         |  2 +-
>>  gdbserver/server.cc            |  2 +-
>>  gdbserver/target.cc            |  4 ++--
>>  gdbserver/target.h             |  2 +-
>>  11 files changed, 28 insertions(+), 30 deletions(-)
>> 
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index db5086962612..a6ed68f93029 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -823,12 +823,13 @@ aarch64_target::low_arch_setup ()
>>    if (is_elf64)
>>      {
>>        struct aarch64_features features;
>> +      int pid = pid_of (current_thread);
>>  
>>        features.vq = aarch64_sve_get_vq (tid);
>>        /* A-profile PAC is 64-bit only.  */
>> -      features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
>> +      features.pauth = (linux_get_hwcap (pid, 8) & AARCH64_HWCAP_PACA);
>
> Avoid adding the outer parenthesis.

Ah, that's a remnant from an older version of the code where this line
was longer and I added the parentheses to fix indentation. Fixed for v3.

>> diff --git a/gdbserver/target.h b/gdbserver/target.h
>> index 18ab969dda70..fe7a80b645bc 100644
>> --- a/gdbserver/target.h
>> +++ b/gdbserver/target.h
>> @@ -175,7 +175,7 @@ class process_stratum_target
>>    /* Read auxiliary vector data from the inferior process.
>>  
>>       Read LEN bytes at OFFSET into a buffer at MYADDR.  */
>> -  virtual int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
>> +  virtual int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
>
> Please update the doc of the method to replace "from the inferior
> process" to from "process with pid PID".

Indeed, thanks for spotting it. Fixed for v3.

-- 
Thiago

Re: [PATCH v2 3/6] gdbserver/linux-aarch64: Factor out function to get aarch64_features

[Thiago Jung Bauermann]

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

> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>> It will be used in a subsequent commit.  There's no functional change.
>> ---
>>   gdbserver/linux-aarch64-low.cc | 33 +++++++++++++++++++++------------
>>   1 file changed, 21 insertions(+), 12 deletions(-)
>
> LGTM
>
> Reviewed-by: Luis Machado <luis.machado@arm.com>

Thanks! Added the Reviewed-by tag to the patch description for v3.

-- 
Thiago

Re: [PATCH v2 3/6] gdbserver/linux-aarch64: Factor out function to get aarch64_features

[Thiago Jung Bauermann]

Simon Marchi <simark@simark.ca> writes:

> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
>> It will be used in a subsequent commit.  There's no functional change.
>> ---
>>  gdbserver/linux-aarch64-low.cc | 33 +++++++++++++++++++++------------
>>  1 file changed, 21 insertions(+), 12 deletions(-)
>> 
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index a6ed68f93029..cab4fc0a4674 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -652,6 +652,25 @@ aarch64_target::low_delete_process (arch_process_info *info)
>>    xfree (info);
>>  }
>>  
>> +/* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
>> +#define AARCH64_HWCAP_PACA (1 << 30)
>> +
>> +static struct aarch64_features
>> +aarch64_get_arch_features (const thread_info *thread)
>
> Please document the new function.

Done for v3.

>> +  struct aarch64_features features;
>> +  int pid = pid_of (thread);
>> +
>> +  features.vq = aarch64_sve_get_vq (lwpid_of (thread));
>
> Just a note, I feel like the pid_of / lwpid_of functions (they used to
> be macros) are a vestige of when GDB was in C.  Today, I would have no
> problem doing:
>
>   thread->id.pid ()
>   thread->id.lwp ()

Ah, thanks for the note. I changed occurrences of pid_of and lwpid_of in
this patch and the previous one (which are the only patches in this
series where the functions were added) to the above.

> With the doc change done,
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

Thanks! Added the Approved-by tag to the patch description for v3.

-- 
Thiago

Re: [PATCH v2 4/6] gdbserver/linux-aarch64: When thread stops, update its target description

[Thiago Jung Bauermann]

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

> Thanks for the patch.
>
> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>> This change allows aarch64-linux to support debugging programs where
>> different threads have different SVE vector lengths.  It requires
>> gdbserver to support different inferior threads having different target
>> descriptions.
>> The arch_update_tdesc method is added to the linux_process_target class to
>> allow aarch64-linux to probe the inferior's vq register and provide an
>> updated thread target description reflecting the new vector length.
>> After this change, all targets except SVE-supporting aarch64-linux will
>> still use per-process target descriptions.
>> ---
>>   gdbserver/gdbthread.h          |  2 ++
>>   gdbserver/linux-aarch64-low.cc | 37 +++++++++++++++++++++++++++++++++-
>>   gdbserver/linux-low.cc         | 18 +++++++++++++++++
>>   gdbserver/linux-low.h          |  5 +++++
>>   gdbserver/regcache.cc          | 11 +++++++---
>>   gdbserver/tdesc.cc             |  3 +++
>>   6 files changed, 72 insertions(+), 4 deletions(-)
>> diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
>> index 8b897e73d33b..47b44d03b8e0 100644
>> --- a/gdbserver/gdbthread.h
>> +++ b/gdbserver/gdbthread.h
>> @@ -80,6 +80,8 @@ struct thread_info
>>       /* Branch trace target information for this thread.  */
>>     struct btrace_target_info *btrace = nullptr;
>> +
>> +  const struct target_desc *tdesc = nullptr;
>
> Should we add a bit of information on how this new field is used, through a comment?

Good idea. For v3 I added:

  /* Target description for this thread.  Only present if it's different
     from the one in process_info.  */
  const struct target_desc *tdesc = nullptr;

>>   };
>>     extern std::list<thread_info *> all_threads;
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index cab4fc0a4674..786ce4071279 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -99,6 +99,9 @@ protected:
>>       void low_arch_setup () override;
>>   +  gdb::optional<const struct target_desc *>
>> +    arch_update_tdesc (const thread_info *thread) override;
>> +
>>     bool low_cannot_fetch_register (int regno) override;
>>       bool low_cannot_store_register (int regno) override;
>> @@ -184,6 +187,8 @@ struct arch_process_info
>>        same for each thread, it is reasonable for the data to live here.
>>        */
>>     struct aarch64_debug_reg_state debug_reg_state;
>> +
>> +  bool has_sve;
>>   };
>
> Though obvious, adding a comment like "has_sve" in gdb/aarch64-tdep.h will clarify the use
> of this field.

Indeed. For v3 I added:

  /* Whether this process has the Scalable Vector Extension available.  */
  bool has_sve;

>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>> index 182a540f3bb3..ff14423e9e07 100644
>> --- a/gdbserver/linux-low.h
>> +++ b/gdbserver/linux-low.h
>> @@ -604,6 +604,11 @@ class linux_process_target : public process_stratum_target
>>     /* Architecture-specific setup for the current thread.  */
>>     virtual void low_arch_setup () = 0;
>>   +  /* Allows arch-specific code to update the thread's target description when
>> +     the inferior stops.  */
>
> I'd also mention sometimes we don't need to update the target description if nothing's
> changed. So an empty return is expected.

Good point. Including Simon's suggestions this is what I have for v3:

  /* Allows arch-specific code to set the thread's target description when the
     inferior stops.  Returns nullptr if no thread-specific target description
     is necessary.  */
  virtual const struct target_desc *
    get_thread_tdesc (const thread_info *thread);

>> +  virtual gdb::optional<const struct target_desc *>
>> +    arch_update_tdesc (const thread_info *thread);
>> +
>>     /* Return false if we can fetch/store the register, true if we cannot
>>        fetch/store the register.  */
>>     virtual bool low_cannot_fetch_register (int regno) = 0;
>> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
>> index 14236069f712..03ee88b3cfd1 100644
>> --- a/gdbserver/regcache.cc
>> +++ b/gdbserver/regcache.cc
>> @@ -39,11 +39,16 @@ get_thread_regcache (struct thread_info *thread, int fetch)
>>        have.  */
>>     if (regcache == NULL)
>>       {
>> -      struct process_info *proc = get_thread_process (thread);
>> +      /* First see if there's a thread-specific target description.  */
>> +      const target_desc *tdesc = thread->tdesc;
>>   -      gdb_assert (proc->tdesc != NULL);
>> +      /* If not, get it from the process instead.  */
>> +      if (tdesc == nullptr)
>> +	tdesc = get_thread_process (thread)->tdesc;
>
> Just a suggestion. Your call.
>
> We could abstract away trying to fetch a tdesc from a thread and then from a process by
> having a function "get_tdesc ()" and calling it here.
>
> Possibly with a better name.

I like the idea. I implemented it and named the function
“get_thread_target_desc”.

>
>>   -      regcache = new_register_cache (proc->tdesc);
>> +      gdb_assert (tdesc != nullptr);
>> +
>> +      regcache = new_register_cache (tdesc);
>>         set_thread_regcache_data (thread, regcache);
>>       }
>>   diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
>> index 5693cc6626fb..3665ab0540d5 100644
>> --- a/gdbserver/tdesc.cc
>> +++ b/gdbserver/tdesc.cc
>> @@ -129,6 +129,9 @@ current_target_desc (void)
>>     if (current_thread == NULL)
>>       return &default_description;
>>   +  if (current_thread->tdesc != nullptr)
>> +    return current_thread->tdesc;
>> +
>>     return current_process ()->tdesc;
>
> We'd use the above function in here as well.

I did that for v3. There's a small difference in the code with the new
version: instead of using current_process (), it ends up doing
“get_thread_process (current_thread)”. I expect it to be equivalent
though, and I'm not seeing any regression in the testsuite.

>>   }
>>   
>
> Otherwise LGTM.
>
> Reviewed-by: Luis Machado <luis.machado@arm.com>

Thanks! Added the Reviewed-by tag to the patch description for v3.

-- 
Thiago

Re: [PATCH v2 4/6] gdbserver/linux-aarch64: When thread stops, update its target description

[Thiago Jung Bauermann]

Simon Marchi <simark@simark.ca> writes:

>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index cab4fc0a4674..786ce4071279 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -99,6 +99,9 @@ protected:
>>  
>>    void low_arch_setup () override;
>>  
>> +  gdb::optional<const struct target_desc *>
>> +    arch_update_tdesc (const thread_info *thread) override;
>
> I'm all for using optional to be clear and explicit in general, but
> unless an empty optional and an optional wrapping nullptr are both valid
> and have different meanings (which doesn't seem, to be the case here), I
> wouldn't recommend wrapping a pointer in an optional.
>
> Pointers have a dedicated value for "no value", that is well understood
> by everyone.  And then, it does create the possibility of returning an
> optional wrapping nullptr, which typically won't be a legitimate value.
> So it's just one more thing to worry about.

I like optional because it forces the caller to check that the result is
valid before using it, whereas with an unwrapped pointer it's easy to
accidentally do a null pointer dereference.

I hadn't considered the situation of an optional wrapping a nullptr
though. Interesting point. For v3 I changed the function to return an
unwrapped pointer and check for nullptr in the caller.

>> @@ -840,8 +845,16 @@ aarch64_target::low_arch_setup ()
>>      {
>>        struct aarch64_features features
>>  	  = aarch64_get_arch_features (current_thread);
>> +      const target_desc *tdesc = aarch64_linux_read_description (features);
>>  
>> -      current_process ()->tdesc = aarch64_linux_read_description (features);
>> +      /* Only SVE-enabled inferiors need per-thread target descriptions.  */
>> +      if (features.vq > 0)
>> +	{
>> +	  current_thread->tdesc = tdesc;
>> +	  current_process ()->priv->arch_private->has_sve = true;
>> +	}
>
> Is it not possible for a process to start with SVE disabled (vq == 0) and
> have some threads enable it later?

Thank you Luis for clarifying this point.

>> @@ -853,6 +866,28 @@ aarch64_target::low_arch_setup ()
>>    aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
>>  }
>>  
>> +/* Implementation of linux target ops method "arch_update_tdesc".  */
>> +
>> +gdb::optional<const struct target_desc *>
>> +aarch64_target::arch_update_tdesc (const thread_info *thread)
>> +{
>> +  /* Only processes using SVE need to update the thread's target description.  */
>> +  if (!get_thread_process (thread)->priv->arch_private->has_sve)
>> +    return {};
>> +
>> +  const struct aarch64_features features = aarch64_get_arch_features (thread);
>> +  const struct target_desc *tdesc = aarch64_linux_read_description (features);
>> +
>> +  if (tdesc == thread->tdesc)
>> +    return {};
>> +
>> +  /* Adjust the register sets we should use for this particular set of
>> +     features.  */
>> +  aarch64_adjust_register_sets(features);
>> +
>> +  return tdesc;
>
> Naming nit: I don't think we need "update" in the method name, there's
> no "updating component" to it AFAICT.  It's basically "get this thread's
> tdesc if for some reason it differents from the process-wide we have".
> So, I don't know, "get_thread_tdesc" or just "thread_tdesc".

That's true. I renamed the method to “get_thread_tdesc”. Thanks for the
suggestion.

>> @@ -2348,6 +2354,18 @@ linux_process_target::filter_event (int lwpid, int wstat)
>>  	      return;
>>  	    }
>>  	}
>> +      else
>> +	{
>> +	  /* Give the arch code an opportunity to update the thread's target
>> +	     description.  */
>> +	  gdb::optional<const struct target_desc *> new_tdesc
>> +	      = arch_update_tdesc (thread);
>> +	  if (new_tdesc.has_value ())
>> +	    {
>> +	      regcache_release ();
>
> Hmm, regcache_release frees the regcache for all threads.  Can we free
> the regcache only for this thread?

Indeed. regcache_release simply calls the static function
“free_register_cache_thread” on all threads, so for v3 I made it public
and called it just for this thread.

-- 
Thiago

Re: [PATCH v2 5/6] gdb/aarch64: Factor out most of the thread_architecture method

[Thiago Jung Bauermann]

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

> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>> The same logic will be used by a subsequent commit when remotely debugging
>> an aarch64-linux target.
>> The code isn't changed, just moved around.
>> ---
>>   gdb/aarch64-linux-nat.c | 28 ++--------------------------
>>   gdb/aarch64-tdep.c      | 35 +++++++++++++++++++++++++++++++++++
>>   gdb/aarch64-tdep.h      |  2 ++
>>   3 files changed, 39 insertions(+), 26 deletions(-)
>
> LGTM.
>
> Approved-by: Luis Machado <luis.machado@arm.com>

Thanks! Added the Approved-by tag to the patch description for v3.

-- 
Thiago

Re: [PATCH v2 5/6] gdb/aarch64: Factor out most of the thread_architecture method

[Thiago Jung Bauermann]

Simon Marchi <simark@simark.ca> writes:

>>  /* Implement the "supports_memory_tagging" target_ops method.  */
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 07330356fdcb..ffc128d91f60 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -3486,6 +3486,41 @@ aarch64_cannot_store_register (struct gdbarch *gdbarch, int regnum)
>>  	  || regnum == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base));
>>  }
>>  
>> +/* Helper function for the "thread_architecture" target_ops method.
>> +
>> +   Returns a new gdbarch that is equivalent to the given gdbarch, but with SVE
>> +   registers reflecting the given vq value.  */
>> +
>> +struct gdbarch *
>> +aarch64_update_gdbarch (struct gdbarch *gdbarch, uint64_t vq)
>
> Document this using:
>
> /* See aarch64-tdep.h.  */
>
> ... and move the doc to the header file.

I did that for v3.

> Otherwise:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

Thanks! Added the Approved-by tag to the patch description for v3.

-- 
Thiago

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Luis Machado]

On 11/26/22 02:04, Thiago Jung Bauermann wrote:
> If the remote target provides an expedited VG register, use it to update
> the thread's gdbarch. This allows debugging programs that change their SVE
> vector length during runtime.
> 
> This is accomplished by implementing the thread_architecture method in
> remote_target, which returns the gdbarch corresponding to the expedited
> registers provided by the last stop reply.
> 
> To allow changing the architecture based on the expedited registers, add a
> new gdbarch method to allow arch-specific code to make the adjustment.
> ---
>   gdb/aarch64-tdep.c        | 26 +++++++++++++++++++++++-
>   gdb/arch-utils.c          |  9 +++++++++
>   gdb/arch-utils.h          |  5 +++++
>   gdb/gdbarch-components.py | 16 +++++++++++++++
>   gdb/gdbarch-gen.h         | 11 ++++++++++
>   gdb/gdbarch.c             | 22 ++++++++++++++++++++
>   gdb/remote.c              | 42 +++++++++++++++++++++++++++++++++++++++
>   7 files changed, 130 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index ffc128d91f60..764f693aee8d 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3486,7 +3486,8 @@ aarch64_cannot_store_register (struct gdbarch *gdbarch, int regnum)
>   	  || regnum == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base));
>   }
>   
> -/* Helper function for the "thread_architecture" target_ops method.
> +/* Helper function for both the "update_architecture" gdbarch method and the
> +   "thread_architecture" target_ops method.
>   
>      Returns a new gdbarch that is equivalent to the given gdbarch, but with SVE
>      registers reflecting the given vq value.  */
> @@ -3521,6 +3522,28 @@ aarch64_update_gdbarch (struct gdbarch *gdbarch, uint64_t vq)
>     return gdbarch_find_by_info (info);
>   }
>   
> +/* Implement the "update_architecture" gdbarch method.  */
> +
> +static struct gdbarch *
> +aarch64_update_architecture (struct gdbarch *gdbarch,
> +			     const std::vector<cached_reg_t> &regcache)
> +{
> +  /* Look for the VG register.  */
> +  auto it = find_if (regcache.cbegin (), regcache.cend (),
> +		     [] (const cached_reg_t &reg) {
> +		       return reg.num == AARCH64_SVE_VG_REGNUM;
> +		     });
> +
> +  /* No VG register was provided.  Don't change gdbarch.  */
> +  if (it == regcache.cend ())
> +    return gdbarch;
> +
> +  ULONGEST vg = extract_unsigned_integer (it->data, 8,
> +					  gdbarch_byte_order (gdbarch));
> +
> +  return aarch64_update_gdbarch (gdbarch, sve_vq_from_vg (vg));
> +}
> +
>   /* Implement the stack_frame_destroyed_p gdbarch method.  */
>   
>   static int
> @@ -3742,6 +3765,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>     set_tdesc_pseudo_register_reggroup_p (gdbarch,
>   					aarch64_pseudo_register_reggroup_p);
>     set_gdbarch_cannot_store_register (gdbarch, aarch64_cannot_store_register);
> +  set_gdbarch_update_architecture (gdbarch, aarch64_update_architecture);
>   
>     /* ABI */
>     set_gdbarch_short_bit (gdbarch, 16);
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 7b84daf046e2..2315aacb4bbe 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1090,6 +1090,15 @@ default_get_return_buf_addr (struct type *val_type, frame_info_ptr cur_frame)
>     return 0;
>   }
>   
> +/* See arch-utils.h.  */
> +
> +struct gdbarch *
> +default_update_architecture (struct gdbarch *gdbarch,
> +			     const std::vector<cached_reg_t> &regcache)
> +{
> +  return gdbarch;
> +}
> +
>   /* Non-zero if we want to trace architecture code.  */
>   
>   #ifndef GDBARCH_DEBUG
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 46018a6fbbb6..e68a91f6753d 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -304,4 +304,9 @@ extern void default_read_core_file_mappings
>   /* Default implementation of gdbarch default_get_return_buf_addr method.  */
>   extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch,
>   					      frame_info_ptr cur_frame);
> +
> +/* Default implementation of gdbarch update_architecture method.  */
> +extern struct gdbarch *
> +default_update_architecture (struct gdbarch *gdbarch,
> +			     const std::vector<cached_reg_t> &regcache);
>   #endif /* ARCH_UTILS_H */
> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
> index 9b688998a7bd..fbb32c5e5853 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -2698,3 +2698,19 @@ Read core file mappings
>       predefault="default_read_core_file_mappings",
>       invalid=False,
>   )
> +
> +Method(
> +    comment="""
> +An architecture may change based on the current contents of its registers.
> +For instance, the length of the vector registers in AArch64's Scalable Vector
> +Extension is given by the contents of the VL pseudo-register.
> +
> +This method allows an architecture to provide a new gdbarch corresponding to
> +the registers in the given regcache.
> +""",
> +    type="struct gdbarch *",
> +    name="update_architecture",
> +    params=[("const std::vector<cached_reg_t> &", "regcache")],
> +    predefault="default_update_architecture",
> +    invalid=False,
> +)
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index a663316df166..a65699e7241f 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1649,3 +1649,14 @@ extern void set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, gdbarch_g
>   typedef void (gdbarch_read_core_file_mappings_ftype) (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb);
>   extern void gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb);
>   extern void set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch, gdbarch_read_core_file_mappings_ftype *read_core_file_mappings);
> +
> +/* An architecture may change based on the current contents of its registers.
> +   For instance, the length of the vector registers in AArch64's Scalable Vector
> +   Extension is given by the contents of the VL pseudo-register.
> +
> +   This method allows an architecture to provide a new gdbarch corresponding to
> +   the registers in the given regcache. */
> +
> +typedef struct gdbarch * (gdbarch_update_architecture_ftype) (struct gdbarch *gdbarch, const std::vector<cached_reg_t> &regcache);
> +extern struct gdbarch * gdbarch_update_architecture (struct gdbarch *gdbarch, const std::vector<cached_reg_t> &regcache);
> +extern void set_gdbarch_update_architecture (struct gdbarch *gdbarch, gdbarch_update_architecture_ftype *update_architecture);
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 3227e9458801..1b0a2c70db4c 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -255,6 +255,7 @@ struct gdbarch
>     gdbarch_type_align_ftype *type_align = default_type_align;
>     gdbarch_get_pc_address_flags_ftype *get_pc_address_flags = default_get_pc_address_flags;
>     gdbarch_read_core_file_mappings_ftype *read_core_file_mappings = default_read_core_file_mappings;
> +  gdbarch_update_architecture_ftype *update_architecture = default_update_architecture;
>   };
>   
>   /* Create a new ``struct gdbarch'' based on information provided by
> @@ -514,6 +515,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>     /* Skip verify of type_align, invalid_p == 0 */
>     /* Skip verify of get_pc_address_flags, invalid_p == 0 */
>     /* Skip verify of read_core_file_mappings, invalid_p == 0 */
> +  /* Skip verify of update_architecture, invalid_p == 0 */
>     if (!log.empty ())
>       internal_error (_("verify_gdbarch: the following are invalid ...%s"),
>   		    log.c_str ());
> @@ -1352,6 +1354,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>     gdb_printf (file,
>                         "gdbarch_dump: read_core_file_mappings = <%s>\n",
>                         host_address_to_string (gdbarch->read_core_file_mappings));
> +  gdb_printf (file,
> +                      "gdbarch_dump: update_architecture = <%s>\n",
> +                      host_address_to_string (gdbarch->update_architecture));
>     if (gdbarch->dump_tdep != NULL)
>       gdbarch->dump_tdep (gdbarch, file);
>   }
> @@ -5314,3 +5319,20 @@ set_gdbarch_read_core_file_mappings (struct gdbarch *gdbarch,
>   {
>     gdbarch->read_core_file_mappings = read_core_file_mappings;
>   }
> +
> +struct gdbarch *
> +gdbarch_update_architecture (struct gdbarch *gdbarch, const std::vector<cached_reg_t> &regcache)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->update_architecture != NULL);
> +  if (gdbarch_debug >= 2)
> +    gdb_printf (gdb_stdlog, "gdbarch_update_architecture called\n");
> +  return gdbarch->update_architecture (gdbarch, regcache);
> +}
> +
> +void
> +set_gdbarch_update_architecture (struct gdbarch *gdbarch,
> +                                 gdbarch_update_architecture_ftype update_architecture)
> +{
> +  gdbarch->update_architecture = update_architecture;
> +}
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 5118ecd0a312..eb60ed51585b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -491,6 +491,8 @@ class remote_target : public process_stratum_target
>     gdb::byte_vector thread_info_to_thread_handle (struct thread_info *tp)
>   						 override;
>   
> +  struct gdbarch *thread_architecture (ptid_t ptid) override;
> +
>     void stop (ptid_t) override;
>   
>     void interrupt () override;
> @@ -1150,6 +1152,10 @@ struct remote_thread_info : public private_thread_info
>        to stop for a watchpoint.  */
>     CORE_ADDR watch_data_address = 0;
>   
> +  /* The architecture corresponding to the expedited registers returned
> +     in the last stop reply.  */
> +  struct gdbarch *expedited_arch = nullptr;
> +
>     /* Get the thread's resume state.  */
>     enum resume_state get_resume_state () const
>     {
> @@ -1168,6 +1174,8 @@ struct remote_thread_info : public private_thread_info
>       m_resume_state = resume_state::RESUMED_PENDING_VCONT;
>       m_resumed_pending_vcont_info.step = step;
>       m_resumed_pending_vcont_info.sig = sig;
> +
> +    expedited_arch = nullptr;
>     }
>   
>     /* Get the information this thread's pending vCont-resumption.
> @@ -1185,6 +1193,8 @@ struct remote_thread_info : public private_thread_info
>     void set_resumed ()
>     {
>       m_resume_state = resume_state::RESUMED;
> +
> +    expedited_arch = nullptr;
>     }
>   
>   private:
> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>         /* Expedited registers.  */
>         if (!stop_reply->regcache.empty ())
>   	{
> +	  /* If GDB already knows about this thread, we can give the
> +	     architecture-specific code a chance to update the gdbarch based on
> +	     the expedited registers.  */
> +	  if (find_thread_ptid (this, ptid) != nullptr)
> +	    {
> +	      stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
> +							      stop_reply->regcache);
> +
> +	      /* Save stop_reply->arch so that it can be returned by the
> +		 thread_architecture method.  */
> +	      remote_thread_info *remote_thr = get_remote_thread_info (this,
> +								       ptid);
> +	      remote_thr->expedited_arch = stop_reply->arch;
> +	    }
> +
>   	  struct regcache *regcache
>   	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);
>   
> @@ -14382,6 +14407,23 @@ remote_target::thread_info_to_thread_handle (struct thread_info *tp)
>     return priv->thread_handle;
>   }
>   
> +struct gdbarch *
> +remote_target::thread_architecture (ptid_t ptid)
> +{
> +  thread_info *thr = find_thread_ptid (this, ptid);
> +  remote_thread_info *remote_thr = nullptr;
> +
> +  if (thr != nullptr)
> +    remote_thr = get_remote_thread_info (thr);
> +
> +  if (remote_thr == nullptr || remote_thr->expedited_arch == nullptr)
> +    /* The default thread_architecture implementation is the one from
> +       process_stratum_target.  */
> +    return process_stratum_target::thread_architecture(ptid);
> +
> +  return remote_thr->expedited_arch;
> +}
> +
>   bool
>   remote_target::can_async_p ()
>   {

Just recalled this. One deficiency of the current SVE implementation is that it doesn't have a proper way to hide the Z register when they don't have any meaningful state (SVE not active, so we have fpsimd state).

Given the VL will always be reported as > 0, even if there is no active SVE state, gdb will always attempt to show Z registers. And those are quite verbose.

In this sense, the implementations of the the thread_architecture methods for both native aarch64 and remote differ. While native aarch64's implementation is capable of detecting the lack of active SVE state, the
remote implementation can't. If gdbserver decided to drop the Z registers mid-execution (before the SVE state is not active), there wouldn't be vg for gdb to make a decision.

For example, there isn't a way to return a value of 0 for vg, as vg == 0 would mean no SVE feature. If there isn't a SVE feature, vg wouldn't exist.

Looking ahead, it would be the same for SME with its SVL. If we have to show both Z and ZA contexts, it will be quite a verbose exchange if we're listing all the possible registers.

I wonder if there is a better way to handle this that would still allow us to hide these scalable registers when there is no active state.

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Thiago Jung Bauermann]

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

> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>> If the remote target provides an expedited VG register, use it to update
>> the thread's gdbarch. This allows debugging programs that change their SVE
>> vector length during runtime.
>
> debugging programs -> debugging programs remotely

Indeed. Fixed.

>> This is accomplished by implementing the thread_architecture method in
>> remote_target, which returns the gdbarch corresponding to the expedited
>> registers provided by the last stop reply.
>> To allow changing the architecture based on the expedited registers, add a
>> new gdbarch method to allow arch-specific code to make the adjustment.

<snip>

>> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
>> index 9b688998a7bd..fbb32c5e5853 100644
>> --- a/gdb/gdbarch-components.py
>> +++ b/gdb/gdbarch-components.py
>> @@ -2698,3 +2698,19 @@ Read core file mappings
>>       predefault="default_read_core_file_mappings",
>>       invalid=False,
>>   )
>> +
>> +Method(
>> +    comment="""
>> +An architecture may change based on the current contents of its registers.
>> +For instance, the length of the vector registers in AArch64's Scalable Vector
>> +Extension is given by the contents of the VL pseudo-register.
>
> VL -> VG

Ok, changed.

>> +This method allows an architecture to provide a new gdbarch corresponding to
>> +the registers in the given regcache.
>> +""",
>> +    type="struct gdbarch *",
>> +    name="update_architecture",
>> +    params=[("const std::vector<cached_reg_t> &", "regcache")],
>> +    predefault="default_update_architecture",
>> +    invalid=False,
>> +)

<snip>

>>   @@ -14382,6 +14407,23 @@ remote_target::thread_info_to_thread_handle (struct
>> thread_info *tp)
>>     return priv->thread_handle;
>>   }
>>   +struct gdbarch *
>> +remote_target::thread_architecture (ptid_t ptid)
>> +{
>> +  thread_info *thr = find_thread_ptid (this, ptid);
>> +  remote_thread_info *remote_thr = nullptr;
>> +
>> +  if (thr != nullptr)
>> +    remote_thr = get_remote_thread_info (thr);
>> +
>> +  if (remote_thr == nullptr || remote_thr->expedited_arch == nullptr)
>> +    /* The default thread_architecture implementation is the one from
>> +       process_stratum_target.  */
>> +    return process_stratum_target::thread_architecture(ptid);
>> +
>> +  return remote_thr->expedited_arch;
>> +}
>> +
>>   bool
>>   remote_target::can_async_p ()
>>   {
>
> Just to confirm, as I don't exactly remember how this went. Is the case where we switch
> threads during remote debugging covered in case we have threads with different vector
> lengths?
>
> I seem to recall we'd call thread_architecture in those cases, but I don't know how the
> expedited register comes into play for this case. Do we get expedited registers for each
> thread that has stopped?

I thought this was fine because my limited understanding of the remote
protocol was that whenever a thread stops, a T stop reply is sent with
the expedited registers.

However, when I tried testing this scenario it didn't work: I get a
“Truncated register 51 in remote 'g' packet” error when the main thread
hits a breakpoint and I try to switch to another thread with a different
vector length. It turns out that gdbserver only sends the T stop reply
for the thread that hit the breakpoint.

Thanks for spotting this problem. I will add a testcase to exercise this
scenario and see if I can come up with a fix.

> Otherwise LGTM.
>
> Reviewed-by: Luis Machado <luis.machado@arm.com>

Thanks!

-- 
Thiago

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Thiago Jung Bauermann]

Simon Marchi <simark@simark.ca> writes:

> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
>> If the remote target provides an expedited VG register, use it to update
>> the thread's gdbarch. This allows debugging programs that change their SVE
>> vector length during runtime.
>> 
>> This is accomplished by implementing the thread_architecture method in
>> remote_target, which returns the gdbarch corresponding to the expedited
>> registers provided by the last stop reply.
>> 
>> To allow changing the architecture based on the expedited registers, add a
>> new gdbarch method to allow arch-specific code to make the adjustment.
>
> I get this when applying, can you address that?
>
> Applying: gdb/aarch64: Detect vector length changes when debugging remotely
> .git/rebase-apply/patch:164: indent with spaces.
>                        "gdbarch_dump: update_architecture = <%s>\n",
> .git/rebase-apply/patch:165: indent with spaces.
>                        host_address_to_string (gdbarch->update_architecture));
> .git/rebase-apply/patch:186: indent with spaces.
>                                   gdbarch_update_architecture_ftype update_architecture)
> warning: 3 lines add whitespace errors.

This is because gdbarch.py generates these lines in gdbarch.c with
spaces-only indentation. I will send a separate patch to fix it.

> I think the idea makes sense.  Short of adding a packet for "get thread
> architecture" or extending the stop reply format to add a note about a
> thread having a special tdesc, I don't see another way to do this.  We

I guess we could define such a packet, but this approach is indeed
simpler.

> can't read the vq register using 'g', because to interpret the 'g'
> result, we need to know the full architecture.  We wouldn't know for
> sure the offset of vq in the reply.  By using expedited registers, we
> just need to make sure vq's register number is stable.  I guess that is
> the case?

The register number is determined by the function
aarch64_create_target_description. If SVE is supported, VG will always
have the register number 85. If not, then AFAIU the register number will
be unused (since the PAuth, MTE and TLS features only have a few
registers). But it could potentially be used by another feature in the
future...

Do we have to reserve the number 85 for the VG pseudo-register?

>> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>>        /* Expedited registers.  */
>>        if (!stop_reply->regcache.empty ())
>>  	{
>> +	  /* If GDB already knows about this thread, we can give the
>> +	     architecture-specific code a chance to update the gdbarch based on
>> +	     the expedited registers.  */
>> +	  if (find_thread_ptid (this, ptid) != nullptr)
>> +	    {
>> +	      stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
>> +							      stop_reply->regcache);
>> +
>> +	      /* Save stop_reply->arch so that it can be returned by the
>> +		 thread_architecture method.  */
>> +	      remote_thread_info *remote_thr = get_remote_thread_info (this,
>> +								       ptid);
>> +	      remote_thr->expedited_arch = stop_reply->arch;
>> +	    }
>> +
>>  	  struct regcache *regcache
>>  	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);
>
>
> What happens with threads that are not yet known?  Imagine the process
> starts with SVE == X, the main threads spawns a worker thread, the
> worker thread updates its SVE to Y, and the worker thread hits a
> breakpoint.  This is the first time we hear about that worker thread.

I tested this scenario and... it sort of works, but not quite. When the
unknown thread hits the breakpoint, gdbserver sends a T stop reply
packet with the new VG value in the expedited registers. It will be the
first time we hear about that worker thread, but at the same time we
will also learn about the value of its VG pseudo-register. So GDB
reports the breakpoint hit event and gets to the prompt as expected. You
can even print the new value of the VG pseudo-register, or any other
expedited register such as PC or SP.

It's only when you try to print the value of a non-expedited register
that you get an error indicating that GDB and gdbserver don't agree on
the size of the SVE registers (“Truncated register 51 in remote 'g'
packet”).

> If we don't do a "gdbarch_update_architecture" for it and save it
> somewhere, we'll never set the gdbarch with SVE == Y, will we?

We do that in remote_target::process_stop_reply, and I was under the
impression that it happened early enough for GDB to have the correct
gdbarch available when it needs it, but unfortunately that's not the
case.

I will add a testcase exercising this issue and see if I can find a
solution. Thank you for spotting it.

-- 
Thiago

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Luis Machado]

On 12/1/22 03:16, Thiago Jung Bauermann wrote:
> 
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
>>> If the remote target provides an expedited VG register, use it to update
>>> the thread's gdbarch. This allows debugging programs that change their SVE
>>> vector length during runtime.
>>>
>>> This is accomplished by implementing the thread_architecture method in
>>> remote_target, which returns the gdbarch corresponding to the expedited
>>> registers provided by the last stop reply.
>>>
>>> To allow changing the architecture based on the expedited registers, add a
>>> new gdbarch method to allow arch-specific code to make the adjustment.
>>
>> I get this when applying, can you address that?
>>
>> Applying: gdb/aarch64: Detect vector length changes when debugging remotely
>> .git/rebase-apply/patch:164: indent with spaces.
>>                         "gdbarch_dump: update_architecture = <%s>\n",
>> .git/rebase-apply/patch:165: indent with spaces.
>>                         host_address_to_string (gdbarch->update_architecture));
>> .git/rebase-apply/patch:186: indent with spaces.
>>                                    gdbarch_update_architecture_ftype update_architecture)
>> warning: 3 lines add whitespace errors.
> 
> This is because gdbarch.py generates these lines in gdbarch.c with
> spaces-only indentation. I will send a separate patch to fix it.
> 
>> I think the idea makes sense.  Short of adding a packet for "get thread
>> architecture" or extending the stop reply format to add a note about a
>> thread having a special tdesc, I don't see another way to do this.  We
> 
> I guess we could define such a packet, but this approach is indeed
> simpler.
> 

Just a FYI, we now have a feature hash for determining the target features for aarch64. It is a 64-bit
number and doesn't rely on positioning of registers.

This situation with vg/SVE will also apply to svg/SME in the future.

>> can't read the vq register using 'g', because to interpret the 'g'
>> result, we need to know the full architecture.  We wouldn't know for
>> sure the offset of vq in the reply.  By using expedited registers, we
>> just need to make sure vq's register number is stable.  I guess that is
>> the case?
> 
> The register number is determined by the function
> aarch64_create_target_description. If SVE is supported, VG will always
> have the register number 85. If not, then AFAIU the register number will
> be unused (since the PAuth, MTE and TLS features only have a few
> registers). But it could potentially be used by another feature in the
> future...
> 
> Do we have to reserve the number 85 for the VG pseudo-register?
> 
>>> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>>>         /* Expedited registers.  */
>>>         if (!stop_reply->regcache.empty ())
>>>   	{
>>> +	  /* If GDB already knows about this thread, we can give the
>>> +	     architecture-specific code a chance to update the gdbarch based on
>>> +	     the expedited registers.  */
>>> +	  if (find_thread_ptid (this, ptid) != nullptr)
>>> +	    {
>>> +	      stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
>>> +							      stop_reply->regcache);
>>> +
>>> +	      /* Save stop_reply->arch so that it can be returned by the
>>> +		 thread_architecture method.  */
>>> +	      remote_thread_info *remote_thr = get_remote_thread_info (this,
>>> +								       ptid);
>>> +	      remote_thr->expedited_arch = stop_reply->arch;
>>> +	    }
>>> +
>>>   	  struct regcache *regcache
>>>   	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);
>>
>>
>> What happens with threads that are not yet known?  Imagine the process
>> starts with SVE == X, the main threads spawns a worker thread, the
>> worker thread updates its SVE to Y, and the worker thread hits a
>> breakpoint.  This is the first time we hear about that worker thread.
> 
> I tested this scenario and... it sort of works, but not quite. When the
> unknown thread hits the breakpoint, gdbserver sends a T stop reply
> packet with the new VG value in the expedited registers. It will be the
> first time we hear about that worker thread, but at the same time we
> will also learn about the value of its VG pseudo-register. So GDB
> reports the breakpoint hit event and gets to the prompt as expected. You
> can even print the new value of the VG pseudo-register, or any other
> expedited register such as PC or SP.
> 
> It's only when you try to print the value of a non-expedited register
> that you get an error indicating that GDB and gdbserver don't agree on
> the size of the SVE registers (“Truncated register 51 in remote 'g'
> packet”).
> 
>> If we don't do a "gdbarch_update_architecture" for it and save it
>> somewhere, we'll never set the gdbarch with SVE == Y, will we?
> 
> We do that in remote_target::process_stop_reply, and I was under the
> impression that it happened early enough for GDB to have the correct
> gdbarch available when it needs it, but unfortunately that's not the
> case.
> 
> I will add a testcase exercising this issue and see if I can find a
> solution. Thank you for spotting it.
> 

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Simon Marchi]

On 11/30/22 22:16, Thiago Jung Bauermann via Gdb-patches wrote:
> 
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
>>> If the remote target provides an expedited VG register, use it to update
>>> the thread's gdbarch. This allows debugging programs that change their SVE
>>> vector length during runtime.
>>>
>>> This is accomplished by implementing the thread_architecture method in
>>> remote_target, which returns the gdbarch corresponding to the expedited
>>> registers provided by the last stop reply.
>>>
>>> To allow changing the architecture based on the expedited registers, add a
>>> new gdbarch method to allow arch-specific code to make the adjustment.
>>
>> I get this when applying, can you address that?
>>
>> Applying: gdb/aarch64: Detect vector length changes when debugging remotely
>> .git/rebase-apply/patch:164: indent with spaces.
>>                        "gdbarch_dump: update_architecture = <%s>\n",
>> .git/rebase-apply/patch:165: indent with spaces.
>>                        host_address_to_string (gdbarch->update_architecture));
>> .git/rebase-apply/patch:186: indent with spaces.
>>                                   gdbarch_update_architecture_ftype update_architecture)
>> warning: 3 lines add whitespace errors.
> 
> This is because gdbarch.py generates these lines in gdbarch.c with
> spaces-only indentation. I will send a separate patch to fix it.

Ah, I did not realize it was in a generated file.  Well, if you can fix
the generator, that's nice, but otherwise not a big deal.

>> can't read the vq register using 'g', because to interpret the 'g'
>> result, we need to know the full architecture.  We wouldn't know for
>> sure the offset of vq in the reply.  By using expedited registers, we
>> just need to make sure vq's register number is stable.  I guess that is
>> the case?
> 
> The register number is determined by the function
> aarch64_create_target_description. If SVE is supported, VG will always
> have the register number 85. If not, then AFAIU the register number will
> be unused (since the PAuth, MTE and TLS features only have a few
> registers). But it could potentially be used by another feature in the
> future...
> 
> Do we have to reserve the number 85 for the VG pseudo-register?

Given that aarch64_update_architecture looks for register 85
(AARCH64_SVE_VG_REGNUM) in the expedited registers without prior
knowledge of whether there is really a VG register, I can imagine bad
things happening if another unrelated register is using number 85.

At this point, do we have access to the inferior's target
description?  Can we maybe check if register 85 is indeed VG, and bail
out otherwise?

> 
>>> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>>>        /* Expedited registers.  */
>>>        if (!stop_reply->regcache.empty ())
>>>  	{
>>> +	  /* If GDB already knows about this thread, we can give the
>>> +	     architecture-specific code a chance to update the gdbarch based on
>>> +	     the expedited registers.  */
>>> +	  if (find_thread_ptid (this, ptid) != nullptr)
>>> +	    {
>>> +	      stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
>>> +							      stop_reply->regcache);
>>> +
>>> +	      /* Save stop_reply->arch so that it can be returned by the
>>> +		 thread_architecture method.  */
>>> +	      remote_thread_info *remote_thr = get_remote_thread_info (this,
>>> +								       ptid);
>>> +	      remote_thr->expedited_arch = stop_reply->arch;
>>> +	    }
>>> +
>>>  	  struct regcache *regcache
>>>  	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);
>>
>>
>> What happens with threads that are not yet known?  Imagine the process
>> starts with SVE == X, the main threads spawns a worker thread, the
>> worker thread updates its SVE to Y, and the worker thread hits a
>> breakpoint.  This is the first time we hear about that worker thread.
> 
> I tested this scenario and... it sort of works, but not quite. When the
> unknown thread hits the breakpoint, gdbserver sends a T stop reply
> packet with the new VG value in the expedited registers. It will be the
> first time we hear about that worker thread, but at the same time we
> will also learn about the value of its VG pseudo-register. So GDB
> reports the breakpoint hit event and gets to the prompt as expected. You
> can even print the new value of the VG pseudo-register, or any other
> expedited register such as PC or SP.
> 
> It's only when you try to print the value of a non-expedited register
> that you get an error indicating that GDB and gdbserver don't agree on
> the size of the SVE registers (“Truncated register 51 in remote 'g'
> packet”).
> 
>> If we don't do a "gdbarch_update_architecture" for it and save it
>> somewhere, we'll never set the gdbarch with SVE == Y, will we?
> 
> We do that in remote_target::process_stop_reply, and I was under the
> impression that it happened early enough for GDB to have the correct
> gdbarch available when it needs it, but unfortunately that's not the
> case.
> 
> I will add a testcase exercising this issue and see if I can find a
> solution. Thank you for spotting it.

I think that the threads (if new) are added just after that, in the
remote_notice_new_inferior call.  Maybe you can reorder things to do
remote_notice_new_inferior first and your code + the
get_thread_arch_regcache call after.

Simon

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Thiago Jung Bauermann]

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

> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>>         /* Expedited registers.  */
>>         if (!stop_reply->regcache.empty ())
>>   	{
>> +	  /* If GDB already knows about this thread, we can give the
>> +	     architecture-specific code a chance to update the gdbarch based on
>> +	     the expedited registers.  */
>> +	  if (find_thread_ptid (this, ptid) != nullptr)
>> +	    {
>> +	      stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
>> +							      stop_reply->regcache);
>> +
>> +	      /* Save stop_reply->arch so that it can be returned by the
>> +		 thread_architecture method.  */
>> +	      remote_thread_info *remote_thr = get_remote_thread_info (this,
>> +								       ptid);
>> +	      remote_thr->expedited_arch = stop_reply->arch;
>> +	    }
>> +
>>   	  struct regcache *regcache
>>   	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);
>>   @@ -14382,6 +14407,23 @@ remote_target::thread_info_to_thread_handle (struct
>> thread_info *tp)
>>     return priv->thread_handle;
>>   }
>>   +struct gdbarch *
>> +remote_target::thread_architecture (ptid_t ptid)
>> +{
>> +  thread_info *thr = find_thread_ptid (this, ptid);
>> +  remote_thread_info *remote_thr = nullptr;
>> +
>> +  if (thr != nullptr)
>> +    remote_thr = get_remote_thread_info (thr);
>> +
>> +  if (remote_thr == nullptr || remote_thr->expedited_arch == nullptr)
>> +    /* The default thread_architecture implementation is the one from
>> +       process_stratum_target.  */
>> +    return process_stratum_target::thread_architecture(ptid);
>> +
>> +  return remote_thr->expedited_arch;
>> +}
>> +
>>   bool
>>   remote_target::can_async_p ()
>>   {
>
> Just recalled this. One deficiency of the current SVE implementation
> is that it doesn't have a proper way to hide the Z register when they
> don't have any meaningful state (SVE not active, so we have fpsimd
> state).

I see the SVE_HEADER_FLAG_SVE being checked/set in
aarch64_linux_supply_sve_regset and aarch64_linux_collect_sve_regset
(though in the latter it is set unconditionally), and also HAS_SVE_STATE
being used in aarch64_sve_regs_copy_to_reg_buf and
aarch64_sve_regs_copy_from_reg_buf.

But I wasn't able to find similar logic regarding hiding the Z
registers. Do you have any pointers?

> Given the VL will always be reported as > 0, even if there is no
> active SVE state, gdb will always attempt to show Z registers. And
> those are quite verbose.

The VG pseudo-register is defined with an int type, so we could return a
negative value to indicate that the Z registers are unused (and the
module of the value would still be the vector granule). Would that
be ok?

> In this sense, the implementations of the the thread_architecture
> methods for both native aarch64 and remote differ. While native
> aarch64's implementation is capable of detecting the lack of active
> SVE state, the remote implementation can't. If gdbserver decided to
> drop the Z registers mid-execution (before the SVE state is not
> active), there wouldn't be vg for gdb to make a decision.

Looking at aarch64_linux_nat_target::thread_architecture my impression
is that its result depends only on aarch64_sve_get_vq, so I don't see
how it's different from the remote implementation.

> I wonder if there is a better way to handle this that would still
> allow us to hide these scalable registers when there is no active
> state.

I guess it depends on whether you would consider using negative VG
values as better. :-)

-- 
Thiago

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Luis Machado]

On 12/5/22 22:37, Thiago Jung Bauermann wrote:
> 
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>>> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>>>          /* Expedited registers.  */
>>>          if (!stop_reply->regcache.empty ())
>>>    	{
>>> +	  /* If GDB already knows about this thread, we can give the
>>> +	     architecture-specific code a chance to update the gdbarch based on
>>> +	     the expedited registers.  */
>>> +	  if (find_thread_ptid (this, ptid) != nullptr)
>>> +	    {
>>> +	      stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
>>> +							      stop_reply->regcache);
>>> +
>>> +	      /* Save stop_reply->arch so that it can be returned by the
>>> +		 thread_architecture method.  */
>>> +	      remote_thread_info *remote_thr = get_remote_thread_info (this,
>>> +								       ptid);
>>> +	      remote_thr->expedited_arch = stop_reply->arch;
>>> +	    }
>>> +
>>>    	  struct regcache *regcache
>>>    	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);
>>>    @@ -14382,6 +14407,23 @@ remote_target::thread_info_to_thread_handle (struct
>>> thread_info *tp)
>>>      return priv->thread_handle;
>>>    }
>>>    +struct gdbarch *
>>> +remote_target::thread_architecture (ptid_t ptid)
>>> +{
>>> +  thread_info *thr = find_thread_ptid (this, ptid);
>>> +  remote_thread_info *remote_thr = nullptr;
>>> +
>>> +  if (thr != nullptr)
>>> +    remote_thr = get_remote_thread_info (thr);
>>> +
>>> +  if (remote_thr == nullptr || remote_thr->expedited_arch == nullptr)
>>> +    /* The default thread_architecture implementation is the one from
>>> +       process_stratum_target.  */
>>> +    return process_stratum_target::thread_architecture(ptid);
>>> +
>>> +  return remote_thr->expedited_arch;
>>> +}
>>> +
>>>    bool
>>>    remote_target::can_async_p ()
>>>    {
>>
>> Just recalled this. One deficiency of the current SVE implementation
>> is that it doesn't have a proper way to hide the Z register when they
>> don't have any meaningful state (SVE not active, so we have fpsimd
>> state).
> 
> I see the SVE_HEADER_FLAG_SVE being checked/set in
> aarch64_linux_supply_sve_regset and aarch64_linux_collect_sve_regset
> (though in the latter it is set unconditionally), and also HAS_SVE_STATE
> being used in aarch64_sve_regs_copy_to_reg_buf and
> aarch64_sve_regs_copy_from_reg_buf.
> 
> But I wasn't able to find similar logic regarding hiding the Z
> registers. Do you have any pointers?
> 

There isn't logic to do that at present. This is something I'm pondering for SME, and would possibly apply to
SVE as well. But it may be more complicated than useful.

>> Given the VL will always be reported as > 0, even if there is no
>> active SVE state, gdb will always attempt to show Z registers. And
>> those are quite verbose.
> 
> The VG pseudo-register is defined with an int type, so we could return a
> negative value to indicate that the Z registers are unused (and the
> module of the value would still be the vector granule). Would that
> be ok?

I think a value of 0 would be fine for this purpose but...

> 
>> In this sense, the implementations of the the thread_architecture
>> methods for both native aarch64 and remote differ. While native
>> aarch64's implementation is capable of detecting the lack of active
>> SVE state, the remote implementation can't. If gdbserver decided to
>> drop the Z registers mid-execution (before the SVE state is not
>> active), there wouldn't be vg for gdb to make a decision.
> 
> Looking at aarch64_linux_nat_target::thread_architecture my impression
> is that its result depends only on aarch64_sve_get_vq, so I don't see
> how it's different from the remote implementation.

... Right, but for native debugging we actually call ptrace to extract the vector length. That is completely independent from
the XML target description or the available register sets.

In the case of remote debugging, the expedited registers are returned based on an existing register description.

Take, for example, the following:

- SVE state is enabled and we have the vg register.
- Program updates the SVE vector length and, as a result, we no longer have an active SVE state (reverted to FPSIMD state)
- GDB drops the sve feature from its target description.
- Program executes a sve instruction and SVE state is made active.
- gdbserver tries to tell gdb what the value of vg is, but gdb doesn't know anything about vg at this point, because it has dropped the sve feature previously.

I think this is the downside of using expedited registers to communicate the architecture changes. For this case, returning a full XML would make it easy for gdb to
figure things out.

But, going back to hiding sve/sme registers when they're not in use, I'm not yet sure that is something we should pursue.

I'd say if you manage to get things working with the expedited registers (for the problematic cases Simon and I pointed out), that's fine.

> 
>> I wonder if there is a better way to handle this that would still
>> allow us to hide these scalable registers when there is no active
>> state.
> 
> I guess it depends on whether you would consider using negative VG
> values as better. :-)
> 

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Simon Marchi]

On 12/7/22 12:05, Luis Machado via Gdb-patches wrote:
> On 12/5/22 22:37, Thiago Jung Bauermann wrote:
>>
>> Luis Machado <luis.machado@arm.com> writes:
>>
>>> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>>>> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>>>>          /* Expedited registers.  */
>>>>          if (!stop_reply->regcache.empty ())
>>>>        {
>>>> +      /* If GDB already knows about this thread, we can give the
>>>> +         architecture-specific code a chance to update the gdbarch based on
>>>> +         the expedited registers.  */
>>>> +      if (find_thread_ptid (this, ptid) != nullptr)
>>>> +        {
>>>> +          stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
>>>> +                                  stop_reply->regcache);
>>>> +
>>>> +          /* Save stop_reply->arch so that it can be returned by the
>>>> +         thread_architecture method.  */
>>>> +          remote_thread_info *remote_thr = get_remote_thread_info (this,
>>>> +                                       ptid);
>>>> +          remote_thr->expedited_arch = stop_reply->arch;
>>>> +        }
>>>> +
>>>>          struct regcache *regcache
>>>>            = get_thread_arch_regcache (this, ptid, stop_reply->arch);
>>>>    @@ -14382,6 +14407,23 @@ remote_target::thread_info_to_thread_handle (struct
>>>> thread_info *tp)
>>>>      return priv->thread_handle;
>>>>    }
>>>>    +struct gdbarch *
>>>> +remote_target::thread_architecture (ptid_t ptid)
>>>> +{
>>>> +  thread_info *thr = find_thread_ptid (this, ptid);
>>>> +  remote_thread_info *remote_thr = nullptr;
>>>> +
>>>> +  if (thr != nullptr)
>>>> +    remote_thr = get_remote_thread_info (thr);
>>>> +
>>>> +  if (remote_thr == nullptr || remote_thr->expedited_arch == nullptr)
>>>> +    /* The default thread_architecture implementation is the one from
>>>> +       process_stratum_target.  */
>>>> +    return process_stratum_target::thread_architecture(ptid);
>>>> +
>>>> +  return remote_thr->expedited_arch;
>>>> +}
>>>> +
>>>>    bool
>>>>    remote_target::can_async_p ()
>>>>    {
>>>
>>> Just recalled this. One deficiency of the current SVE implementation
>>> is that it doesn't have a proper way to hide the Z register when they
>>> don't have any meaningful state (SVE not active, so we have fpsimd
>>> state).
>>
>> I see the SVE_HEADER_FLAG_SVE being checked/set in
>> aarch64_linux_supply_sve_regset and aarch64_linux_collect_sve_regset
>> (though in the latter it is set unconditionally), and also HAS_SVE_STATE
>> being used in aarch64_sve_regs_copy_to_reg_buf and
>> aarch64_sve_regs_copy_from_reg_buf.
>>
>> But I wasn't able to find similar logic regarding hiding the Z
>> registers. Do you have any pointers?
>>
> 
> There isn't logic to do that at present. This is something I'm pondering for SME, and would possibly apply to
> SVE as well. But it may be more complicated than useful.
> 
>>> Given the VL will always be reported as > 0, even if there is no
>>> active SVE state, gdb will always attempt to show Z registers. And
>>> those are quite verbose.
>>
>> The VG pseudo-register is defined with an int type, so we could return a
>> negative value to indicate that the Z registers are unused (and the
>> module of the value would still be the vector granule). Would that
>> be ok?
> 
> I think a value of 0 would be fine for this purpose but...
> 
>>
>>> In this sense, the implementations of the the thread_architecture
>>> methods for both native aarch64 and remote differ. While native
>>> aarch64's implementation is capable of detecting the lack of active
>>> SVE state, the remote implementation can't. If gdbserver decided to
>>> drop the Z registers mid-execution (before the SVE state is not
>>> active), there wouldn't be vg for gdb to make a decision.
>>
>> Looking at aarch64_linux_nat_target::thread_architecture my impression
>> is that its result depends only on aarch64_sve_get_vq, so I don't see
>> how it's different from the remote implementation.
> 
> ... Right, but for native debugging we actually call ptrace to extract the vector length. That is completely independent from
> the XML target description or the available register sets.
> 
> In the case of remote debugging, the expedited registers are returned based on an existing register description.
> 
> Take, for example, the following:
> 
> - SVE state is enabled and we have the vg register.
> - Program updates the SVE vector length and, as a result, we no longer have an active SVE state (reverted to FPSIMD state)
> - GDB drops the sve feature from its target description.
> - Program executes a sve instruction and SVE state is made active.
> - gdbserver tries to tell gdb what the value of vg is, but gdb doesn't know anything about vg at this point, because it has dropped the sve feature previously.
> 
> I think this is the downside of using expedited registers to communicate the architecture changes. For this case, returning a full XML would make it easy for gdb to
> figure things out.
> 
> But, going back to hiding sve/sme registers when they're not in use, I'm not yet sure that is something we should pursue.
> 
> I'd say if you manage to get things working with the expedited registers (for the problematic cases Simon and I pointed out), that's fine.

Reading this, I thought of another potential problem with using
expedited registers to communicate the architectures change.  When using
the non-stop version of the remote protocol, we will have a stop reply
for each thread that stops, so that's fine, we can infer each thread's
tdesc.  But with the all-stop variant of the protocol, you only get a
stop reply for one thread, for a given stop.

For instance, imagine you have two threads, thread 1 hits a breakpoint.
GDB gets a stop reply for thread 1, and thread 2 is just considered stop
because we're using the all-stop remote protocol.  If we need to read
thread 2's registers, we would need to know its tdesc.  Its vg may have
changed since last time, or it might be the first time we learned about
it.  In that case, I don't think we have a way out of adding some mean
for gdbserver to tell gdb which tdesc applies to that thread.

If it's the case, that we need to extend the RSP to allow fetching
per-thread tdesc, here's the idea I had in mind for a while, to avoid
adding too much overhead.  Stop replies and the qXfer:threads:read reply
could include a per-thread tdesc identifier.  That tdesc identifier
would be something short, either an incrementing number, or some kind of
hash of the tdesc.  It would be something opaque chosen by the stub.  A
new packet would be introduced to allow GDB to request the XML given
that ID.  On the GDB side, GDB would request the XML when encountering a
tdesc ID it doesn't know about, and then cache the result.  The
additional overhead would be:

 - fetching each XML tdesc once, that seems inevitable (we just don't
   want to fetch the same XML tdesc over and over)
 - a few characters more in stop replies and qXfer:threads:read replies

I believe this could be implemented in a backwards compatible way
without even adding a new qSupported feature.  XML is extensible by
nature.  And stop replies are too.  The T stop reply doc says:

  Otherwise, GDB should ignore this ‘n:r’ pair and go on to the next;
  this allows us to extend the protocol in the future.

So in theory we could add a `tdesc-id:1234` field without breaking old
GDBs.  However, those old GDBs would break when trying to read registers
of threads with unexpected SVE lengths.

Simon

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Luis Machado]

On 12/7/22 19:25, Simon Marchi wrote:
> On 12/7/22 12:05, Luis Machado via Gdb-patches wrote:
>> On 12/5/22 22:37, Thiago Jung Bauermann wrote:
>>>
>>> Luis Machado <luis.machado@arm.com> writes:
>>>
>>>> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>>>>> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>>>>>           /* Expedited registers.  */
>>>>>           if (!stop_reply->regcache.empty ())
>>>>>         {
>>>>> +      /* If GDB already knows about this thread, we can give the
>>>>> +         architecture-specific code a chance to update the gdbarch based on
>>>>> +         the expedited registers.  */
>>>>> +      if (find_thread_ptid (this, ptid) != nullptr)
>>>>> +        {
>>>>> +          stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
>>>>> +                                  stop_reply->regcache);
>>>>> +
>>>>> +          /* Save stop_reply->arch so that it can be returned by the
>>>>> +         thread_architecture method.  */
>>>>> +          remote_thread_info *remote_thr = get_remote_thread_info (this,
>>>>> +                                       ptid);
>>>>> +          remote_thr->expedited_arch = stop_reply->arch;
>>>>> +        }
>>>>> +
>>>>>           struct regcache *regcache
>>>>>             = get_thread_arch_regcache (this, ptid, stop_reply->arch);
>>>>>     @@ -14382,6 +14407,23 @@ remote_target::thread_info_to_thread_handle (struct
>>>>> thread_info *tp)
>>>>>       return priv->thread_handle;
>>>>>     }
>>>>>     +struct gdbarch *
>>>>> +remote_target::thread_architecture (ptid_t ptid)
>>>>> +{
>>>>> +  thread_info *thr = find_thread_ptid (this, ptid);
>>>>> +  remote_thread_info *remote_thr = nullptr;
>>>>> +
>>>>> +  if (thr != nullptr)
>>>>> +    remote_thr = get_remote_thread_info (thr);
>>>>> +
>>>>> +  if (remote_thr == nullptr || remote_thr->expedited_arch == nullptr)
>>>>> +    /* The default thread_architecture implementation is the one from
>>>>> +       process_stratum_target.  */
>>>>> +    return process_stratum_target::thread_architecture(ptid);
>>>>> +
>>>>> +  return remote_thr->expedited_arch;
>>>>> +}
>>>>> +
>>>>>     bool
>>>>>     remote_target::can_async_p ()
>>>>>     {
>>>>
>>>> Just recalled this. One deficiency of the current SVE implementation
>>>> is that it doesn't have a proper way to hide the Z register when they
>>>> don't have any meaningful state (SVE not active, so we have fpsimd
>>>> state).
>>>
>>> I see the SVE_HEADER_FLAG_SVE being checked/set in
>>> aarch64_linux_supply_sve_regset and aarch64_linux_collect_sve_regset
>>> (though in the latter it is set unconditionally), and also HAS_SVE_STATE
>>> being used in aarch64_sve_regs_copy_to_reg_buf and
>>> aarch64_sve_regs_copy_from_reg_buf.
>>>
>>> But I wasn't able to find similar logic regarding hiding the Z
>>> registers. Do you have any pointers?
>>>
>>
>> There isn't logic to do that at present. This is something I'm pondering for SME, and would possibly apply to
>> SVE as well. But it may be more complicated than useful.
>>
>>>> Given the VL will always be reported as > 0, even if there is no
>>>> active SVE state, gdb will always attempt to show Z registers. And
>>>> those are quite verbose.
>>>
>>> The VG pseudo-register is defined with an int type, so we could return a
>>> negative value to indicate that the Z registers are unused (and the
>>> module of the value would still be the vector granule). Would that
>>> be ok?
>>
>> I think a value of 0 would be fine for this purpose but...
>>
>>>
>>>> In this sense, the implementations of the the thread_architecture
>>>> methods for both native aarch64 and remote differ. While native
>>>> aarch64's implementation is capable of detecting the lack of active
>>>> SVE state, the remote implementation can't. If gdbserver decided to
>>>> drop the Z registers mid-execution (before the SVE state is not
>>>> active), there wouldn't be vg for gdb to make a decision.
>>>
>>> Looking at aarch64_linux_nat_target::thread_architecture my impression
>>> is that its result depends only on aarch64_sve_get_vq, so I don't see
>>> how it's different from the remote implementation.
>>
>> ... Right, but for native debugging we actually call ptrace to extract the vector length. That is completely independent from
>> the XML target description or the available register sets.
>>
>> In the case of remote debugging, the expedited registers are returned based on an existing register description.
>>
>> Take, for example, the following:
>>
>> - SVE state is enabled and we have the vg register.
>> - Program updates the SVE vector length and, as a result, we no longer have an active SVE state (reverted to FPSIMD state)
>> - GDB drops the sve feature from its target description.
>> - Program executes a sve instruction and SVE state is made active.
>> - gdbserver tries to tell gdb what the value of vg is, but gdb doesn't know anything about vg at this point, because it has dropped the sve feature previously.
>>
>> I think this is the downside of using expedited registers to communicate the architecture changes. For this case, returning a full XML would make it easy for gdb to
>> figure things out.
>>
>> But, going back to hiding sve/sme registers when they're not in use, I'm not yet sure that is something we should pursue.
>>
>> I'd say if you manage to get things working with the expedited registers (for the problematic cases Simon and I pointed out), that's fine.
> 
> Reading this, I thought of another potential problem with using
> expedited registers to communicate the architectures change.  When using
> the non-stop version of the remote protocol, we will have a stop reply
> for each thread that stops, so that's fine, we can infer each thread's
> tdesc.  But with the all-stop variant of the protocol, you only get a
> stop reply for one thread, for a given stop.

I wasn't sure about this case, so I mentioned it in the first reply to patch 6/6. Isn't it possible to send back
information about all the threads that have stopped in all-stop mode?

> 
> For instance, imagine you have two threads, thread 1 hits a breakpoint.
> GDB gets a stop reply for thread 1, and thread 2 is just considered stop
> because we're using the all-stop remote protocol.  If we need to read
> thread 2's registers, we would need to know its tdesc.  Its vg may have
> changed since last time, or it might be the first time we learned about
> it.  In that case, I don't think we have a way out of adding some mean
> for gdbserver to tell gdb which tdesc applies to that thread.
> 
> If it's the case, that we need to extend the RSP to allow fetching
> per-thread tdesc, here's the idea I had in mind for a while, to avoid
> adding too much overhead.  Stop replies and the qXfer:threads:read reply
> could include a per-thread tdesc identifier.  That tdesc identifier
> would be something short, either an incrementing number, or some kind of
> hash of the tdesc.  It would be something opaque chosen by the stub.  A
> new packet would be introduced to allow GDB to request the XML given
> that ID.  On the GDB side, GDB would request the XML when encountering a
> tdesc ID it doesn't know about, and then cache the result.  The
> additional overhead would be:

The aarch64 port uses feature hashes for the target descriptions. We could leverage that, but we would need to
keep the hash stable so older gdb's that don't know about certain features don't break/get confused.

> 
>   - fetching each XML tdesc once, that seems inevitable (we just don't
>     want to fetch the same XML tdesc over and over)
>   - a few characters more in stop replies and qXfer:threads:read replies
> 
> I believe this could be implemented in a backwards compatible way
> without even adding a new qSupported feature.  XML is extensible by
> nature.  And stop replies are too.  The T stop reply doc says:
> 
>    Otherwise, GDB should ignore this ‘n:r’ pair and go on to the next;
>    this allows us to extend the protocol in the future.
> 
> So in theory we could add a `tdesc-id:1234` field without breaking old
> GDBs.  However, those old GDBs would break when trying to read registers
> of threads with unexpected SVE lengths.

It sounds like a good approach to me, and slightly better than using the expedited registers.

It would be nice if we could minimize the amount of tdesc-id entries for all the threads, if they're mostly the same.

The most obvious case is having hundreds of threads that all have the same tdesc-id. It wouldn't be nice to duplicate all that data.

Originally we discussed a packet to ask the remote if the target description had changed. If nothing's changed, it is a quick reply.

I suppose we could have something similar to that here as well, to optimize things, that is, don't return data if nothing's changed.
> 
> Simon

Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Thiago Jung Bauermann]

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

> On 12/7/22 19:25, Simon Marchi wrote:
>> Reading this, I thought of another potential problem with using
>> expedited registers to communicate the architectures change.  When using
>> the non-stop version of the remote protocol, we will have a stop reply
>> for each thread that stops, so that's fine, we can infer each thread's
>> tdesc.  But with the all-stop variant of the protocol, you only get a
>> stop reply for one thread, for a given stop.
>
> I wasn't sure about this case, so I mentioned it in the first reply to
> patch 6/6. Isn't it possible to send back information about all the
> threads that have stopped in all-stop mode?

I think it could be done by adding the extra information in the XML of
the qXfer:threads:read reply as Simon suggested here.

I thought about adding the expedited registers for each thread to fix
this case, but as long as we are making changes to the remote protocol,
Simon's idea is cleaner and more general.

>> For instance, imagine you have two threads, thread 1 hits a breakpoint.
>> GDB gets a stop reply for thread 1, and thread 2 is just considered stop
>> because we're using the all-stop remote protocol.  If we need to read
>> thread 2's registers, we would need to know its tdesc.  Its vg may have
>> changed since last time, or it might be the first time we learned about
>> it.  In that case, I don't think we have a way out of adding some mean
>> for gdbserver to tell gdb which tdesc applies to that thread.
>> If it's the case, that we need to extend the RSP to allow fetching
>> per-thread tdesc, here's the idea I had in mind for a while, to avoid
>> adding too much overhead.  Stop replies and the qXfer:threads:read reply
>> could include a per-thread tdesc identifier.  That tdesc identifier
>> would be something short, either an incrementing number, or some kind of
>> hash of the tdesc.  It would be something opaque chosen by the stub.  A
>> new packet would be introduced to allow GDB to request the XML given
>> that ID.  On the GDB side, GDB would request the XML when encountering a
>> tdesc ID it doesn't know about, and then cache the result.  The
>> additional overhead would be:
>
> The aarch64 port uses feature hashes for the target descriptions. We
> could leverage that, but we would need to keep the hash stable so
> older gdb's that don't know about certain features don't break/get
> confused.

IIUC the tdesc ids are defined by gdbserver and are opaque to GDB, and
also they are only meaningful within a single connection, so they don't
need to be stable.

Because of that, I think small integers are preferable to hashes because
they are shorter so it's less data to push through the wire.

>>   - fetching each XML tdesc once, that seems inevitable (we just don't
>>     want to fetch the same XML tdesc over and over)
>>   - a few characters more in stop replies and qXfer:threads:read replies
>> I believe this could be implemented in a backwards compatible way
>> without even adding a new qSupported feature.  XML is extensible by
>> nature.  And stop replies are too.  The T stop reply doc says:
>>    Otherwise, GDB should ignore this ‘n:r’ pair and go on to the next;
>>    this allows us to extend the protocol in the future.
>> So in theory we could add a `tdesc-id:1234` field without breaking old
>> GDBs.  However, those old GDBs would break when trying to read registers
>> of threads with unexpected SVE lengths.
>
> It sounds like a good approach to me, and slightly better than using
> the expedited registers.

I agree. Thanks Simon for this idea and for detailing it! I'll take a
stab at implementing it for v3.

> It would be nice if we could minimize the amount of tdesc-id entries
> for all the threads, if they're mostly the same.
>
> The most obvious case is having hundreds of threads that all have the
> same tdesc-id. It wouldn't be nice to duplicate all that data.

gdbserver can assign the same id for identical tdescs, so I think this
will happen naturally.

> Originally we discussed a packet to ask the remote if the target
> description had changed. If nothing's changed, it is a quick reply.
>
> I suppose we could have something similar to that here as well, to
> optimize things, that is, don't return data if nothing's changed.

Yes, if it happens that GDB needs to fetch registers without having
received a tdesc-updating T stop reply or qXfer:threads:read reply then
this is a good solution.

-- 
Thiago