public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/8] gdbserver improvements for AArch64 SVE support
@ 2022-09-08  6:41 Thiago Jung Bauermann
  2022-09-08  6:41 ` [PATCH 1/8] gdbserver: Add asserts in register_size and register_data functions Thiago Jung Bauermann
                   ` (7 more replies)
  0 siblings, 8 replies; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-09-08  6:41 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann

Hello,

The aarch64 architecture supports the Scalable Vector Extension, which
provides vector registers and operations that can be used by software in a
way that is agnostic to the size of the vector length actually provided by
the hardware. It also allows programs to change the vector length at
runtime. Also, it's worth noting that each thread in the program can have
a different vector length.

GDB supports debugging programs using SVE, and also supports debugging
programs which change their vector length at runtime. gdbserver, on the
other hand, supports the former but not the latter.

This patch series adds support to gdbserver to allow debugging programs
that change their vector length at runtime. This series makes the test
gdb.arch/aarch64-sve.exp pass on the native-gdbserver and
native-extended-gdbserver boards.

GDB supports different vector lengths by using one target description for
each vector length, and switches target descriptions when it notices that
the vector length of the inferior thread changed. This is done by
aarch64_linux_nat_target::thread_architecture, which returns a gdbarch
reflecting the current vector length.

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 arguably in patch 5, which changes gdbserver to use per-thread
target descriptions rather than per-process. In practice only the
aarch64-linux target will actually have different per-thread tdescs. All
other targets use the same tdesc in all threads so little is changed.

Since patch 5 touches so many targets, I'm trying to test this series on
some different systems but the only real or virtualized machines I can
test on are {aarch64,x86_64}-{linux,freebsd,netbsd}. I'll also try it out
on QEMU-emulated machines on some other architectures, assuming that's
helpful.

I'm still going through testing (and struggling a bit to get the testsuite
running properly on the BSDs) but these are my results so far:

These targets were regression tested and no regressions were found:

- aarch64-linux on the native-extended-gdbserver board
- x86_64-linux on unix¹ and native-extended-gdbserver boards

These targets build but were not regression tested:

- aarch64-freebsd13.1
- aarch64-netbsd9.3
- arm-linux-gnueabihf
- powerpc-linux
- powerpc64le-linux
- riscv64-linux

The targets corresponding to these files were edited with no testing so
far. I'll test some under QEMU emulation but most will be left untested:

- linux-arc-low.cc
- linux-ia64-low.cc
- linux-loongarch-low.cc
- linux-m68k-low.cc
- linux-mips-low.cc
- linux-nios2-low.cc
- linux-or1k-low.cc
- linux-s390-low.cc
- linux-sh-low.cc
- linux-sparc-low.cc
- linux-tic6x-low.cc
- linux-xtensa-low.cc
- netbsd-amd64-low.cc
- netbsd-i386-low.cc

¹ There's one additional unexpected core file reported on x86_linux
  native, even though the test results are unchanged. I'm looking into it
  and will report back.


Thiago Jung Bauermann (8):
  gdbserver: Add asserts in register_size and register_data functions
  gdbserver: Add thread parameter to linux_get_auxv and linux_get_hwcap
  gdb,gdbserver/aarch64-linux: Allow aarch64_sve_get_vq to return error
  gdbserver/linux-aarch64: Factor out function to get aarch64_features
  gdbserver: Move target description from being per-process to being
    per-thread
  gdbserver/linux-aarch64: When inferior stops, update its target
    description
  gdb/aarch64: Factor out most of the thread_architecture method
  gdb/aarch64: When remote debugging detect changes in the target
    description

 gdb/aarch64-linux-nat.c            |  37 ++++------
 gdb/aarch64-tdep.c                 |  60 ++++++++++++++++
 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                      |  23 ++++++
 gdb/nat/aarch64-sve-linux-ptrace.c |  18 +++--
 gdb/nat/aarch64-sve-linux-ptrace.h |   2 +-
 gdb/remote.c                       |  42 +++++++++++
 gdbserver/gdbthread.h              |   2 +
 gdbserver/inferiors.h              |   2 -
 gdbserver/linux-aarch64-low.cc     | 110 +++++++++++++++++++++++++----
 gdbserver/linux-arc-low.cc         |   6 +-
 gdbserver/linux-arm-low.cc         |   6 +-
 gdbserver/linux-ia64-low.cc        |   2 +-
 gdbserver/linux-loongarch-low.cc   |   2 +-
 gdbserver/linux-low.cc             |  62 +++++++++++-----
 gdbserver/linux-low.h              |  15 ++--
 gdbserver/linux-m68k-low.cc        |   2 +-
 gdbserver/linux-mips-low.cc        |   8 +--
 gdbserver/linux-nios2-low.cc       |   2 +-
 gdbserver/linux-or1k-low.cc        |   2 +-
 gdbserver/linux-ppc-low.cc         |  16 ++---
 gdbserver/linux-riscv-low.cc       |   2 +-
 gdbserver/linux-s390-low.cc        |   6 +-
 gdbserver/linux-sh-low.cc          |   2 +-
 gdbserver/linux-sparc-low.cc       |   2 +-
 gdbserver/linux-tic6x-low.cc       |   2 +-
 gdbserver/linux-x86-low.cc         |   2 +-
 gdbserver/linux-xtensa-low.cc      |   2 +-
 gdbserver/netbsd-aarch64-low.cc    |   2 +-
 gdbserver/netbsd-amd64-low.cc      |   2 +-
 gdbserver/netbsd-i386-low.cc       |   2 +-
 gdbserver/netbsd-low.cc            |   4 +-
 gdbserver/netbsd-low.h             |   4 +-
 gdbserver/regcache.cc              |  10 +--
 gdbserver/server.cc                |   2 +-
 gdbserver/target.cc                |   4 +-
 gdbserver/target.h                 |   4 +-
 gdbserver/tdesc.cc                 |   2 +-
 42 files changed, 395 insertions(+), 121 deletions(-)


base-commit: 5edf42b635a375f7bf79e2079529eeb869129cbe

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 1/8] gdbserver: Add asserts in register_size and register_data functions
  2022-09-08  6:41 [PATCH 0/8] gdbserver improvements for AArch64 SVE support Thiago Jung Bauermann
@ 2022-09-08  6:41 ` Thiago Jung Bauermann
  2022-09-20  7:36   ` Luis Machado
  2022-09-08  6:41 ` [PATCH 2/8] gdbserver: Add thread parameter to linux_get_auxv and linux_get_hwcap Thiago Jung Bauermann
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-09-08  6:41 UTC (permalink / raw)
  To: gdb-patches; +Cc: 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 27491efc52d5..ebaeb5e86895 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);
 }

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 2/8] gdbserver: Add thread parameter to linux_get_auxv and linux_get_hwcap
  2022-09-08  6:41 [PATCH 0/8] gdbserver improvements for AArch64 SVE support Thiago Jung Bauermann
  2022-09-08  6:41 ` [PATCH 1/8] gdbserver: Add asserts in register_size and register_data functions Thiago Jung Bauermann
@ 2022-09-08  6:41 ` Thiago Jung Bauermann
  2022-09-20  7:43   ` Luis Machado
  2022-09-08  6:41 ` [PATCH 3/8] gdb, gdbserver/aarch64-linux: Allow aarch64_sve_get_vq to return error Thiago Jung Bauermann
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-09-08  6:41 UTC (permalink / raw)
  To: gdb-patches; +Cc: 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
thread_info parameter seems more correct than setting current_thread in one
particular code path.

Changes are propagated to allow passing the thread parameter through the
call chain.
---
 gdbserver/linux-aarch64-low.cc |  6 +++---
 gdbserver/linux-arm-low.cc     |  2 +-
 gdbserver/linux-low.cc         | 20 +++++++++++---------
 gdbserver/linux-low.h          | 10 +++++-----
 gdbserver/linux-ppc-low.cc     |  6 +++---
 gdbserver/linux-s390-low.cc    |  2 +-
 gdbserver/netbsd-low.cc        |  4 ++--
 gdbserver/netbsd-low.h         |  4 ++--
 gdbserver/server.cc            |  2 +-
 gdbserver/target.cc            |  4 ++--
 gdbserver/target.h             |  4 ++--
 11 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index db5086962612..5d4d667dfa42 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -826,9 +826,9 @@ aarch64_target::low_arch_setup ()
 
       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 (current_thread, 8) & AARCH64_HWCAP_PACA;
       /* A-profile MTE is 64-bit only.  */
-      features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
+      features.mte = linux_get_hwcap2 (current_thread, 8) & HWCAP2_MTE;
       features.tls = true;
 
       current_process ()->tdesc = aarch64_linux_read_description (features);
@@ -3299,7 +3299,7 @@ aarch64_target::supports_memory_tagging ()
 #endif
     }
 
-  return (linux_get_hwcap2 (8) & HWCAP2_MTE) != 0;
+  return (linux_get_hwcap2 (current_thread, 8) & HWCAP2_MTE) != 0;
 }
 
 bool
diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
index ab6209a3abc8..d40ce412fe1c 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 (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 2f71360d3bde..9034a1d8fabe 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -5486,12 +5486,12 @@ 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 (const thread_info *thread, CORE_ADDR offset,
+				 unsigned char *myaddr, unsigned int len)
 {
   char filename[PATH_MAX];
   int fd, n;
-  int pid = lwpid_of (current_thread);
+  int pid = lwpid_of (thread);
 
   xsnprintf (filename, sizeof filename, "/proc/%d/auxv", pid);
 
@@ -6912,14 +6912,16 @@ 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 (const thread_info *thread, 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 (thread, offset, data, 2 * wordsize)
+	 == 2 * wordsize)
     {
       if (wordsize == 4)
 	{
@@ -6949,20 +6951,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 (const thread_info *thread, int wordsize)
 {
   CORE_ADDR hwcap = 0;
-  linux_get_auxv (wordsize, AT_HWCAP, &hwcap);
+  linux_get_auxv (thread, wordsize, AT_HWCAP, &hwcap);
   return hwcap;
 }
 
 /* See linux-low.h.  */
 
 CORE_ADDR
-linux_get_hwcap2 (int wordsize)
+linux_get_hwcap2 (const thread_info *thread, int wordsize)
 {
   CORE_ADDR hwcap2 = 0;
-  linux_get_auxv (wordsize, AT_HWCAP2, &hwcap2);
+  linux_get_auxv (thread, wordsize, AT_HWCAP2, &hwcap2);
   return hwcap2;
 }
 
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 79be31b8f729..bd1ec31dc5a4 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -178,8 +178,8 @@ class linux_process_target : public process_stratum_target
 
   bool supports_read_auxv () override;
 
-  int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
-		 unsigned int len) override;
+  int read_auxv (const thread_info *thread, CORE_ADDR offset,
+		 unsigned char *myaddr, unsigned int len) override;
 
   int insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
 		    int size, raw_breakpoint *bp) override;
@@ -944,17 +944,17 @@ 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,
+int linux_get_auxv (const thread_info *thread, 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 (const thread_info *thread, 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 (const thread_info *thread, int wordsize);
 
 #endif /* GDBSERVER_LINUX_LOW_H */
diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc
index 08824887003b..08608b2dcc38 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 (current_thread, features.wordsize);
+  ppc_hwcap2 = linux_get_hwcap2 (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 (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..0d01dffa64a4 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 (current_thread, 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..71ac61e9dfbd 100644
--- a/gdbserver/netbsd-low.cc
+++ b/gdbserver/netbsd-low.cc
@@ -581,10 +581,10 @@ 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 (const thread_info *thread, CORE_ADDR offset,
 				  unsigned char *myaddr, unsigned int len)
 {
-  pid_t pid = pid_of (current_thread);
+  pid_t pid = pid_of (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..506329251eaf 100644
--- a/gdbserver/netbsd-low.h
+++ b/gdbserver/netbsd-low.h
@@ -77,8 +77,8 @@ class netbsd_process_target : public process_stratum_target
 
   bool supports_read_auxv () override;
 
-  int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
-		 unsigned int len) override;
+  int read_auxv (const thread_info *thread, 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 c619206d5d2d..2d5cf6e8d8de 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 (current_thread, offset, readbuf, len);
 }
 
 /* Handle qXfer:exec-file:read.  */
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index adcfe6e7bcc7..7ee18bde8635 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 (const thread_info *thread, 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 6c536a307786..cd6b2193a228 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -175,8 +175,8 @@ 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,
-			 unsigned int len);
+  virtual int read_auxv (const thread_info *thread, CORE_ADDR offset,
+			 unsigned char *myaddr, unsigned int len);
 
   /* Returns true if GDB Z breakpoint type TYPE is supported, false
      otherwise.  The type is coded as follows:

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 3/8] gdb, gdbserver/aarch64-linux: Allow aarch64_sve_get_vq to return error
  2022-09-08  6:41 [PATCH 0/8] gdbserver improvements for AArch64 SVE support Thiago Jung Bauermann
  2022-09-08  6:41 ` [PATCH 1/8] gdbserver: Add asserts in register_size and register_data functions Thiago Jung Bauermann
  2022-09-08  6:41 ` [PATCH 2/8] gdbserver: Add thread parameter to linux_get_auxv and linux_get_hwcap Thiago Jung Bauermann
@ 2022-09-08  6:41 ` Thiago Jung Bauermann
  2022-09-20  8:07   ` Luis Machado
  2022-09-08  6:41 ` [PATCH 4/8] gdbserver/linux-aarch64: Factor out function to get aarch64_features Thiago Jung Bauermann
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-09-08  6:41 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann

If ptrace fails, aarch64_sve_get_vq assumes that SVE isn't supported.
Because in a subsequent change this function will need to be called from
low_new_thread (which can be called whe the inferior thread isn't stopped),
it will need to distinguish between ptrace errors due to SVE not being
supported from ptrace errors due to the inferior thread not being stopped.

This patch changes the function to return -1 in the latter case and adjusts
callers.  When a caller is allowed to propagate the error, it does so.  When
that isn't possible an assertion is added to ensure the error isn't missed.
---
 gdb/aarch64-linux-nat.c            | 14 ++++++++++++--
 gdb/nat/aarch64-sve-linux-ptrace.c | 18 ++++++++++++------
 gdb/nat/aarch64-sve-linux-ptrace.h |  2 +-
 gdbserver/linux-aarch64-low.cc     |  6 +++++-
 4 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index eda79ec6d35c..633cbc08796c 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -784,8 +784,14 @@ aarch64_linux_nat_target::read_description ()
   CORE_ADDR hwcap = linux_get_hwcap (this);
   CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
 
+  int vq = aarch64_sve_get_vq (tid);
+  if (vq < 0)
+    /* A ptrace error happened so we can't determine the vq value.
+       Give up trying to read a target description.  */
+    return nullptr;
+
   aarch64_features features;
-  features.vq = aarch64_sve_get_vq (tid);
+  features.vq = vq;
   features.pauth = hwcap & AARCH64_HWCAP_PACA;
   features.mte = hwcap2 & HWCAP2_MTE;
   features.tls = true;
@@ -894,7 +900,11 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
   /* 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 ());
+  int vq = aarch64_sve_get_vq (ptid.lwp ());
+
+  /* If ptrace fails we can't determine vq, but the thread_architecture method
+     always succeeds so all we can do here is assert that vq is valid.  */
+  gdb_assert (vq >= 0);
   if (vq == tdep->vq)
     return inf->gdbarch;
 
diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
index 019d2d65d89a..62f7fc5f56e1 100644
--- a/gdb/nat/aarch64-sve-linux-ptrace.c
+++ b/gdb/nat/aarch64-sve-linux-ptrace.c
@@ -30,7 +30,7 @@
 
 /* See nat/aarch64-sve-linux-ptrace.h.  */
 
-uint64_t
+int
 aarch64_sve_get_vq (int tid)
 {
   struct iovec iovec;
@@ -43,13 +43,19 @@ aarch64_sve_get_vq (int tid)
      128bit chunks in a Z register.  We use VQ because 128bits is the minimum
      a Z register can increase in size.  */
 
+  errno = 0;
   if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
     {
+      if (errno == ESRCH)
+	/* The process isn't stopped.  We can't determine SVE support or the
+	   value of vq.  */
+	return -1;
+
       /* SVE is not supported.  */
       return 0;
     }
 
-  uint64_t vq = sve_vq_from_vl (header.vl);
+  int vq = sve_vq_from_vl (header.vl);
 
   if (!sve_vl_valid (header.vl))
     {
@@ -103,10 +109,10 @@ aarch64_sve_set_vq (int tid, struct reg_buffer_common *reg_buf)
     {
       /* If vg is not available yet, fetch it from ptrace.  The VG value from
 	 ptrace is likely the correct one.  */
-      uint64_t vq = aarch64_sve_get_vq (tid);
+      int vq = aarch64_sve_get_vq (tid);
 
       /* If something went wrong, just bail out.  */
-      if (vq == 0)
+      if (vq <= 0)
 	return false;
 
       reg_vg = sve_vg_from_vq (vq);
@@ -123,9 +129,9 @@ std::unique_ptr<gdb_byte[]>
 aarch64_sve_get_sveregs (int tid)
 {
   struct iovec iovec;
-  uint64_t vq = aarch64_sve_get_vq (tid);
+  int vq = aarch64_sve_get_vq (tid);
 
-  if (vq == 0)
+  if (vq <= 0)
     perror_with_name (_("Unable to fetch SVE register header"));
 
   /* A ptrace call with NT_ARM_SVE will return a header followed by either a
diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
index 5c264b395313..90920bc48ef3 100644
--- a/gdb/nat/aarch64-sve-linux-ptrace.h
+++ b/gdb/nat/aarch64-sve-linux-ptrace.h
@@ -43,7 +43,7 @@
 /* Read VQ for the given tid using ptrace.  If SVE is not supported then zero
    is returned (on a system that supports SVE, then VQ cannot be zero).  */
 
-uint64_t aarch64_sve_get_vq (int tid);
+int aarch64_sve_get_vq (int tid);
 
 /* Set VQ in the kernel for the given tid, using either the value VQ or
    reading from the register VG in the register buffer.  */
diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index 5d4d667dfa42..576925838f49 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -823,8 +823,12 @@ aarch64_target::low_arch_setup ()
   if (is_elf64)
     {
       struct aarch64_features features;
+      int vq = aarch64_sve_get_vq (tid);
 
-      features.vq = aarch64_sve_get_vq (tid);
+      /* If ptrace fails we can't determine vq, but the low_arch_setup method
+	  always succeeds so all we can do here is assert that vq is valid.  */
+      gdb_assert (vq >= 0);
+      features.vq = vq;
       /* A-profile PAC is 64-bit only.  */
       features.pauth = linux_get_hwcap (current_thread, 8) & AARCH64_HWCAP_PACA;
       /* A-profile MTE is 64-bit only.  */

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 4/8] gdbserver/linux-aarch64: Factor out function to get aarch64_features
  2022-09-08  6:41 [PATCH 0/8] gdbserver improvements for AArch64 SVE support Thiago Jung Bauermann
                   ` (2 preceding siblings ...)
  2022-09-08  6:41 ` [PATCH 3/8] gdb, gdbserver/aarch64-linux: Allow aarch64_sve_get_vq to return error Thiago Jung Bauermann
@ 2022-09-08  6:41 ` Thiago Jung Bauermann
  2022-09-20  8:08   ` Luis Machado
  2022-09-08  6:41 ` [PATCH 5/8] gdbserver: Move target description from being per-process to being per-thread Thiago Jung Bauermann
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-09-08  6:41 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann

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

diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index 576925838f49..9b57be73818e 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -652,6 +652,28 @@ 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 gdb::optional<struct aarch64_features>
+aarch64_get_arch_features (const thread_info *thread)
+{
+  struct aarch64_features features;
+  int vq = aarch64_sve_get_vq (thread->id.lwp ());
+
+  if (vq < 0)
+    return {};
+
+  features.vq = vq;
+  /* A-profile PAC is 64-bit only.  */
+  features.pauth = linux_get_hwcap (thread, 8) & AARCH64_HWCAP_PACA;
+  /* A-profile MTE is 64-bit only.  */
+  features.mte = linux_get_hwcap2 (thread, 8) & HWCAP2_MTE;
+  features.tls = true;
+
+  return features;
+}
+
 void
 aarch64_target::low_new_thread (lwp_info *lwp)
 {
@@ -804,9 +826,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,24 +841,18 @@ aarch64_target::low_arch_setup ()
 
   if (is_elf64)
     {
-      struct aarch64_features features;
-      int vq = aarch64_sve_get_vq (tid);
+      gdb::optional<struct aarch64_features> features
+	  = aarch64_get_arch_features (current_thread);
 
-      /* If ptrace fails we can't determine vq, but the low_arch_setup method
-	  always succeeds so all we can do here is assert that vq is valid.  */
-      gdb_assert (vq >= 0);
-      features.vq = vq;
-      /* A-profile PAC is 64-bit only.  */
-      features.pauth = linux_get_hwcap (current_thread, 8) & AARCH64_HWCAP_PACA;
-      /* A-profile MTE is 64-bit only.  */
-      features.mte = linux_get_hwcap2 (current_thread, 8) & HWCAP2_MTE;
-      features.tls = true;
+      /* If ptrace fails we can't determine vq, but low_arch_setup always
+	 succeeds so all we can do here is assert that features is valid.  */
+      gdb_assert (features.has_value ());
 
-      current_process ()->tdesc = aarch64_linux_read_description (features);
+      current_process ()->tdesc = aarch64_linux_read_description (*features);
 
       /* Adjust the register sets we should use for this particular set of
 	 features.  */
-      aarch64_adjust_register_sets (features);
+      aarch64_adjust_register_sets (*features);
     }
   else
     current_process ()->tdesc = aarch32_linux_read_description ();

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 5/8] gdbserver: Move target description from being per-process to being per-thread
  2022-09-08  6:41 [PATCH 0/8] gdbserver improvements for AArch64 SVE support Thiago Jung Bauermann
                   ` (3 preceding siblings ...)
  2022-09-08  6:41 ` [PATCH 4/8] gdbserver/linux-aarch64: Factor out function to get aarch64_features Thiago Jung Bauermann
@ 2022-09-08  6:41 ` Thiago Jung Bauermann
  2022-09-20 11:21   ` Luis Machado
  2022-09-08  6:41 ` [PATCH 6/8] gdbserver/linux-aarch64: When inferior stops, update its target description Thiago Jung Bauermann
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-09-08  6:41 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann

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

After this change, all targets except aarch64-linux will still use one
target description for all threads in the inferior process (their tdesc
member in struct thread_info will point to the same target description).
This is done by linux_process_target::handle_extended_wait which copies the
target description from the original thread to the new thread if
low_new_thread doesn't provide one.

In the case of aarch64-linux, the low_new_thread method probes the thread's
architecture features and assigns a description to it.
---
 gdbserver/gdbthread.h            |  2 ++
 gdbserver/inferiors.h            |  2 --
 gdbserver/linux-aarch64-low.cc   | 48 ++++++++++++++++++++++++++++++--
 gdbserver/linux-arc-low.cc       |  6 ++--
 gdbserver/linux-arm-low.cc       |  4 +--
 gdbserver/linux-ia64-low.cc      |  2 +-
 gdbserver/linux-loongarch-low.cc |  2 +-
 gdbserver/linux-low.cc           | 24 ++++++++++------
 gdbserver/linux-m68k-low.cc      |  2 +-
 gdbserver/linux-mips-low.cc      |  8 +++---
 gdbserver/linux-nios2-low.cc     |  2 +-
 gdbserver/linux-or1k-low.cc      |  2 +-
 gdbserver/linux-ppc-low.cc       | 10 +++----
 gdbserver/linux-riscv-low.cc     |  2 +-
 gdbserver/linux-s390-low.cc      |  4 +--
 gdbserver/linux-sh-low.cc        |  2 +-
 gdbserver/linux-sparc-low.cc     |  2 +-
 gdbserver/linux-tic6x-low.cc     |  2 +-
 gdbserver/linux-x86-low.cc       |  2 +-
 gdbserver/linux-xtensa-low.cc    |  2 +-
 gdbserver/netbsd-aarch64-low.cc  |  2 +-
 gdbserver/netbsd-amd64-low.cc    |  2 +-
 gdbserver/netbsd-i386-low.cc     |  2 +-
 gdbserver/regcache.cc            |  6 ++--
 gdbserver/tdesc.cc               |  2 +-
 25 files changed, 95 insertions(+), 49 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/inferiors.h b/gdbserver/inferiors.h
index 6de746cb228f..cdb39dd90d54 100644
--- a/gdbserver/inferiors.h
+++ b/gdbserver/inferiors.h
@@ -65,8 +65,6 @@ struct process_info
      for unfiltered syscall reporting.  */
   std::vector<int> syscalls_to_catch;
 
-  const struct target_desc *tdesc = NULL;
-
   /* Private target data.  */
   struct process_info_private *priv = NULL;
 
diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index 9b57be73818e..8c4306ab8794 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -191,9 +191,26 @@ struct arch_process_info
 static int
 is_64bit_tdesc (void)
 {
+  const target_desc *tdesc;
+
+  if (current_thread && current_thread->tdesc)
+    tdesc = current_thread->tdesc;
+  else
+    {
+      /* Find some thread that has a target description.  */
+      const struct thread_info *thread
+	  = find_thread (current_process ()->pid, [] (thread_info *thr) {
+	      return thr->tdesc != nullptr;
+	    });
+
+      gdb_assert (thread != nullptr);
+
+      tdesc = thread->tdesc;
+    }
+
   /* We may not have a current thread at this point, so go straight to
      the process's target description.  */
-  return register_size (current_process ()->tdesc, 0) == 8;
+  return register_size (tdesc, 0) == 8;
 }
 
 static void
@@ -678,6 +695,31 @@ void
 aarch64_target::low_new_thread (lwp_info *lwp)
 {
   aarch64_linux_new_thread (lwp);
+
+  ptid_t ptid = ptid_of_lwp (lwp);
+  process_info *proc = find_process_pid (ptid.pid ());
+
+  /* If the inferior is still going through the shell or the thread isn't
+     stopped we can't read its registers in order to read the target
+     description.  */
+  if (proc->starting_up)
+    return;
+
+  thread_info *thread = get_lwp_thread (lwp);
+  unsigned int machine;
+  gdb_assert (ptid.lwp_p ());
+  bool is_elf64 = linux_pid_exe_is_elf_64_file (ptid.lwp (), &machine);
+
+  if (is_elf64)
+    {
+      gdb::optional<struct aarch64_features> features
+	  = aarch64_get_arch_features (thread);
+
+      if (features.has_value())
+	thread->tdesc = aarch64_linux_read_description (*features);
+    }
+  else
+    thread->tdesc = aarch32_linux_read_description ();
 }
 
 void
@@ -848,14 +890,14 @@ aarch64_target::low_arch_setup ()
 	 succeeds so all we can do here is assert that features is valid.  */
       gdb_assert (features.has_value ());
 
-      current_process ()->tdesc = aarch64_linux_read_description (*features);
+      current_thread->tdesc = aarch64_linux_read_description (*features);
 
       /* Adjust the register sets we should use for this particular set of
 	 features.  */
       aarch64_adjust_register_sets (*features);
     }
   else
-    current_process ()->tdesc = aarch32_linux_read_description ();
+    current_thread->tdesc = aarch32_linux_read_description ();
 
   aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
 }
diff --git a/gdbserver/linux-arc-low.cc b/gdbserver/linux-arc-low.cc
index fee1dd33eb20..1727bdede452 100644
--- a/gdbserver/linux-arc-low.cc
+++ b/gdbserver/linux-arc-low.cc
@@ -123,19 +123,19 @@ arc_linux_read_description (void)
 void
 arc_target::low_arch_setup ()
 {
-  current_process ()->tdesc = arc_linux_read_description ();
+  current_thread->tdesc = arc_linux_read_description ();
 }
 
 bool
 arc_target::low_cannot_fetch_register (int regno)
 {
-  return (regno >= current_process ()->tdesc->reg_defs.size ());
+  return (regno >= current_thread->tdesc->reg_defs.size ());
 }
 
 bool
 arc_target::low_cannot_store_register (int regno)
 {
-  return (regno >= current_process ()->tdesc->reg_defs.size ());
+  return (regno >= current_thread->tdesc->reg_defs.size ());
 }
 
 /* This works for both endianness.  Below you see an illustration of how
diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
index d40ce412fe1c..5f52e061bcf6 100644
--- a/gdbserver/linux-arm-low.cc
+++ b/gdbserver/linux-arm-low.cc
@@ -998,7 +998,7 @@ arm_target::low_arch_setup ()
   /* Query hardware watchpoint/breakpoint capabilities.  */
   arm_linux_init_hwbp_cap (tid);
 
-  current_process ()->tdesc = arm_read_description ();
+  current_thread->tdesc = arm_read_description ();
 
   iov.iov_base = gpregs;
   iov.iov_len = sizeof (gpregs);
@@ -1118,7 +1118,7 @@ static struct regs_info regs_info_arm =
 const regs_info *
 arm_target::get_regs_info ()
 {
-  const struct target_desc *tdesc = current_process ()->tdesc;
+  const struct target_desc *tdesc = current_thread->tdesc;
 
   if (have_ptrace_getregset == 1
       && (is_aarch32_linux_description (tdesc)
diff --git a/gdbserver/linux-ia64-low.cc b/gdbserver/linux-ia64-low.cc
index 4530a283b376..3095038fc50e 100644
--- a/gdbserver/linux-ia64-low.cc
+++ b/gdbserver/linux-ia64-low.cc
@@ -382,7 +382,7 @@ ia64_target::get_regs_info ()
 void
 ia64_target::low_arch_setup ()
 {
-  current_process ()->tdesc = tdesc_ia64;
+  current_thread->tdesc = tdesc_ia64;
 }
 
 /* The linux target ops object.  */
diff --git a/gdbserver/linux-loongarch-low.cc b/gdbserver/linux-loongarch-low.cc
index cccf1ba780b3..50cca5b2da10 100644
--- a/gdbserver/linux-loongarch-low.cc
+++ b/gdbserver/linux-loongarch-low.cc
@@ -86,7 +86,7 @@ loongarch_target::low_arch_setup ()
 
   if (!tdesc->expedite_regs)
     init_target_desc (tdesc.get (), expedite_regs);
-  current_process ()->tdesc = tdesc.release ();
+  current_thread->tdesc = tdesc.release ();
 }
 
 /* Collect GPRs from REGCACHE into BUF.  */
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 9034a1d8fabe..d37eb120e114 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -585,8 +585,8 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
 	  clone_all_breakpoints (child_thr, event_thr);
 
 	  target_desc_up tdesc = allocate_target_description ();
-	  copy_target_description (tdesc.get (), parent_proc->tdesc);
-	  child_proc->tdesc = tdesc.release ();
+	  copy_target_description (tdesc.get (), event_thr->tdesc);
+	  child_thr->tdesc = tdesc.release ();
 
 	  /* Clone arch-specific process data.  */
 	  low_new_fork (parent_proc, child_proc);
@@ -642,6 +642,11 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
 	 before calling resume_one_lwp.  */
       new_lwp->stopped = 1;
 
+      /* If low_new_thread () wasn't able to set the target description, copy
+	 it from the original thread.  */
+      if (new_lwp->thread->tdesc == nullptr)
+	new_lwp->thread->tdesc = event_thr->tdesc;
+
       /* If we're suspending all threads, leave this one suspended
 	 too.  If the fork/clone parent is stepping over a breakpoint,
 	 all other threads have been suspended already.  Leave the
@@ -1209,7 +1214,7 @@ linux_process_target::attach (unsigned long pid)
 
       async_file_mark ();
 
-      gdb_assert (proc->tdesc != NULL);
+      gdb_assert (initial_thread->tdesc != NULL);
     }
 
   return 0;
@@ -2330,7 +2335,7 @@ linux_process_target::filter_event (int lwpid, int wstat)
 
       /* Architecture-specific setup after inferior is running.  */
       proc = find_process_pid (pid_of (thread));
-      if (proc->tdesc == NULL)
+      if (thread->tdesc == NULL || proc->starting_up)
 	{
 	  if (proc->attached)
 	    {
@@ -3966,14 +3971,13 @@ linux_process_target::resume_one_lwp_throw (lwp_info *lwp, int step,
 {
   struct thread_info *thread = get_lwp_thread (lwp);
   int ptrace_request;
-  struct process_info *proc = get_thread_process (thread);
 
   /* Note that target description may not be initialised
-     (proc->tdesc == NULL) at this point because the program hasn't
+     (thread->tdesc == NULL) at this point because the program hasn't
      stopped at the first instruction yet.  It means GDBserver skips
      the extra traps from the wrapper program (see option --wrapper).
      Code in this function that requires register access should be
-     guarded by proc->tdesc == NULL or something else.  */
+     guarded by thread->tdesc == NULL or something else.  */
 
   if (lwp->stopped == 0)
     return;
@@ -4090,7 +4094,9 @@ linux_process_target::resume_one_lwp_throw (lwp_info *lwp, int step,
       step = single_step (lwp);
     }
 
-  if (proc->tdesc != NULL && low_supports_breakpoints ())
+  process_info *proc = find_process_pid(thread->id.pid());
+
+  if (!proc->starting_up && low_supports_breakpoints ())
     {
       struct regcache *regcache = get_thread_regcache (current_thread, 1);
 
@@ -4349,7 +4355,7 @@ linux_process_target::thread_needs_step_over (thread_info *thread)
 
   /* GDBserver is skipping the extra traps from the wrapper program,
      don't have to do step over.  */
-  if (proc->tdesc == NULL)
+  if (proc->starting_up)
     return false;
 
   /* LWPs which will not be resumed are not interesting, because we
diff --git a/gdbserver/linux-m68k-low.cc b/gdbserver/linux-m68k-low.cc
index 7a433ffab5ef..5b17e21c5258 100644
--- a/gdbserver/linux-m68k-low.cc
+++ b/gdbserver/linux-m68k-low.cc
@@ -253,7 +253,7 @@ m68k_target::get_regs_info ()
 void
 m68k_target::low_arch_setup ()
 {
-  current_process ()->tdesc = tdesc_m68k;
+  current_thread->tdesc = tdesc_m68k;
 }
 
 /* The linux target ops object.  */
diff --git a/gdbserver/linux-mips-low.cc b/gdbserver/linux-mips-low.cc
index e72403bd91cb..6d9d110e412c 100644
--- a/gdbserver/linux-mips-low.cc
+++ b/gdbserver/linux-mips-low.cc
@@ -213,7 +213,7 @@ mips_read_description (void)
 void
 mips_target::low_arch_setup ()
 {
-  current_process ()->tdesc = mips_read_description ();
+  current_thread->tdesc = mips_read_description ();
 }
 
 /* Per-process arch-specific data we want to keep.  */
@@ -264,7 +264,7 @@ mips_target::low_cannot_fetch_register (int regno)
   if (get_regs_info ()->usrregs->regmap[regno] == -1)
     return true;
 
-  tdesc = current_process ()->tdesc;
+  tdesc = current_thread->tdesc;
 
   /* On n32 we can't access 64-bit registers via PTRACE_PEEKUSR.  */
   if (register_size (tdesc, regno) > sizeof (PTRACE_XFER_TYPE))
@@ -284,7 +284,7 @@ mips_target::low_cannot_store_register (int regno)
   if (get_regs_info ()->usrregs->regmap[regno] == -1)
     return true;
 
-  tdesc = current_process ()->tdesc;
+  tdesc = current_thread->tdesc;
 
   /* On n32 we can't access 64-bit registers via PTRACE_POKEUSR.  */
   if (register_size (tdesc, regno) > sizeof (PTRACE_XFER_TYPE))
@@ -308,7 +308,7 @@ mips_target::low_cannot_store_register (int regno)
 bool
 mips_target::low_fetch_register (regcache *regcache, int regno)
 {
-  const struct target_desc *tdesc = current_process ()->tdesc;
+  const struct target_desc *tdesc = current_thread->tdesc;
 
   if (find_regno (tdesc, "r0") == regno)
     {
diff --git a/gdbserver/linux-nios2-low.cc b/gdbserver/linux-nios2-low.cc
index e850fcfc1efc..675305f0de51 100644
--- a/gdbserver/linux-nios2-low.cc
+++ b/gdbserver/linux-nios2-low.cc
@@ -119,7 +119,7 @@ static int nios2_regmap[] = {
 void
 nios2_target::low_arch_setup ()
 {
-  current_process ()->tdesc = tdesc_nios2_linux;
+  current_thread->tdesc = tdesc_nios2_linux;
 }
 
 /* Implement the low_cannot_fetch_register linux target ops method.  */
diff --git a/gdbserver/linux-or1k-low.cc b/gdbserver/linux-or1k-low.cc
index 4c6d69496fb2..0288d634367e 100644
--- a/gdbserver/linux-or1k-low.cc
+++ b/gdbserver/linux-or1k-low.cc
@@ -115,7 +115,7 @@ static int or1k_regmap[] = {
 void
 or1k_target::low_arch_setup ()
 {
-  current_process ()->tdesc = tdesc_or1k_linux;
+  current_thread->tdesc = tdesc_or1k_linux;
 }
 
 /* Implement the low_cannot_fetch_register linux target ops method.  */
diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc
index 08608b2dcc38..486574bd3dbb 100644
--- a/gdbserver/linux-ppc-low.cc
+++ b/gdbserver/linux-ppc-low.cc
@@ -212,7 +212,7 @@ ppc_check_regset (int tid, int regset_id, int regsetsize)
 bool
 ppc_target::low_cannot_store_register (int regno)
 {
-  const struct target_desc *tdesc = current_process ()->tdesc;
+  const struct target_desc *tdesc = current_thread->tdesc;
 
 #ifndef __powerpc64__
   /* Some kernels do not allow us to store fpscr.  */
@@ -890,9 +890,9 @@ ppc_target::low_arch_setup ()
   else
       tdesc = tdesc_powerpc_64l;
 
-  current_process ()->tdesc = tdesc;
+  current_thread->tdesc = tdesc;
 
-  /* The value of current_process ()->tdesc needs to be set for this
+  /* The value of current_thread->tdesc needs to be set for this
      call.  */
   ppc_hwcap = linux_get_hwcap (current_thread, features.wordsize);
   ppc_hwcap2 = linux_get_hwcap2 (current_thread, features.wordsize);
@@ -952,7 +952,7 @@ ppc_target::low_arch_setup ()
    }
 #endif
 
-  current_process ()->tdesc = tdesc;
+  current_thread->tdesc = tdesc;
 
   for (regset = ppc_regsets; regset->size >= 0; regset++)
     switch (regset->get_request)
@@ -1094,7 +1094,7 @@ is_elfv2_inferior (void)
   CORE_ADDR phdr;
   Elf64_Ehdr ehdr;
 
-  const struct target_desc *tdesc = current_process ()->tdesc;
+  const struct target_desc *tdesc = current_thread->tdesc;
   int wordsize = register_size (tdesc, 0);
 
   if (!linux_get_auxv (current_thread, wordsize, AT_PHDR, &phdr))
diff --git a/gdbserver/linux-riscv-low.cc b/gdbserver/linux-riscv-low.cc
index 6b2902e422d9..c1c3e5fa5cae 100644
--- a/gdbserver/linux-riscv-low.cc
+++ b/gdbserver/linux-riscv-low.cc
@@ -92,7 +92,7 @@ riscv_target::low_arch_setup ()
 
   if (!tdesc->expedite_regs)
     init_target_desc (tdesc.get (), expedite_regs);
-  current_process ()->tdesc = tdesc.release ();
+  current_thread->tdesc = tdesc.release ();
 }
 
 /* Collect GPRs from REGCACHE into BUF.  */
diff --git a/gdbserver/linux-s390-low.cc b/gdbserver/linux-s390-low.cc
index 0d01dffa64a4..4d3e81a05c76 100644
--- a/gdbserver/linux-s390-low.cc
+++ b/gdbserver/linux-s390-low.cc
@@ -697,7 +697,7 @@ s390_target::low_arch_setup ()
 	  break;
 	}
 
-  current_process ()->tdesc = tdesc;
+  current_thread->tdesc = tdesc;
 }
 
 
@@ -771,7 +771,7 @@ s390_target::get_regs_info ()
   if (have_hwcap_s390_high_gprs)
     {
 #ifdef __s390x__
-      const struct target_desc *tdesc = current_process ()->tdesc;
+      const struct target_desc *tdesc = current_thread->tdesc;
 
       if (register_size (tdesc, 0) == 4)
 	return &regs_info_3264;
diff --git a/gdbserver/linux-sh-low.cc b/gdbserver/linux-sh-low.cc
index 966bdeb8ba7a..bee46b8892a3 100644
--- a/gdbserver/linux-sh-low.cc
+++ b/gdbserver/linux-sh-low.cc
@@ -180,7 +180,7 @@ sh_target::get_regs_info ()
 void
 sh_target::low_arch_setup ()
 {
-  current_process ()->tdesc = tdesc_sh;
+  current_thread->tdesc = tdesc_sh;
 }
 
 /* The linux target ops object.  */
diff --git a/gdbserver/linux-sparc-low.cc b/gdbserver/linux-sparc-low.cc
index d4826d029a9d..7c9d4d456926 100644
--- a/gdbserver/linux-sparc-low.cc
+++ b/gdbserver/linux-sparc-low.cc
@@ -299,7 +299,7 @@ sparc_target::low_breakpoint_at (CORE_ADDR where)
 void
 sparc_target::low_arch_setup ()
 {
-  current_process ()->tdesc = tdesc_sparc64;
+  current_thread->tdesc = tdesc_sparc64;
 }
 
 static struct regset_info sparc_regsets[] = {
diff --git a/gdbserver/linux-tic6x-low.cc b/gdbserver/linux-tic6x-low.cc
index 893b5f795ac1..02032112a1be 100644
--- a/gdbserver/linux-tic6x-low.cc
+++ b/gdbserver/linux-tic6x-low.cc
@@ -389,7 +389,7 @@ tic6x_target::low_arch_setup ()
     }
   tic6x_usrregs_info.regmap = tic6x_regmap;
 
-  current_process ()->tdesc = tic6x_read_description (feature);
+  current_thread->tdesc = tic6x_read_description (feature);
 }
 
 static struct regsets_info tic6x_regsets_info =
diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
index d2b55f6f0d2b..14c42603a67e 100644
--- a/gdbserver/linux-x86-low.cc
+++ b/gdbserver/linux-x86-low.cc
@@ -1091,7 +1091,7 @@ x86_target::get_regs_info ()
 void
 x86_target::low_arch_setup ()
 {
-  current_process ()->tdesc = x86_linux_read_description ();
+  current_thread->tdesc = x86_linux_read_description ();
 }
 
 bool
diff --git a/gdbserver/linux-xtensa-low.cc b/gdbserver/linux-xtensa-low.cc
index 8e7c4c08b7bd..7035edff7bd2 100644
--- a/gdbserver/linux-xtensa-low.cc
+++ b/gdbserver/linux-xtensa-low.cc
@@ -311,7 +311,7 @@ static struct regs_info myregs_info =
 void
 xtensa_target::low_arch_setup ()
 {
-  current_process ()->tdesc = tdesc_xtensa;
+  current_thread->tdesc = tdesc_xtensa;
 }
 
 const regs_info *
diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
index f8447b0d1eec..211a0794b7fc 100644
--- a/gdbserver/netbsd-aarch64-low.cc
+++ b/gdbserver/netbsd-aarch64-low.cc
@@ -101,7 +101,7 @@ netbsd_aarch64_target::low_arch_setup ()
   static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
   init_target_desc (tdesc, expedite_regs_aarch64);
 
-  current_process ()->tdesc = tdesc;
+  current_thread->tdesc = tdesc;
 }
 
 /* The singleton target ops object.  */
diff --git a/gdbserver/netbsd-amd64-low.cc b/gdbserver/netbsd-amd64-low.cc
index d08e3489f580..024401a6418c 100644
--- a/gdbserver/netbsd-amd64-low.cc
+++ b/gdbserver/netbsd-amd64-low.cc
@@ -194,7 +194,7 @@ netbsd_amd64_target::low_arch_setup ()
 
   init_target_desc (tdesc, amd64_expedite_regs);
 
-  current_process ()->tdesc = tdesc;
+  current_thread->tdesc = tdesc;
 }
 
 /* The singleton target ops object.  */
diff --git a/gdbserver/netbsd-i386-low.cc b/gdbserver/netbsd-i386-low.cc
index f6fa40c144e3..eec149aa9f99 100644
--- a/gdbserver/netbsd-i386-low.cc
+++ b/gdbserver/netbsd-i386-low.cc
@@ -145,7 +145,7 @@ netbsd_i386_target::low_arch_setup ()
 
   init_target_desc (tdesc, i386_expedite_regs);
 
-  current_process ()->tdesc = tdesc;
+  current_thread->tdesc = tdesc;
 }
 
 /* The singleton target ops object.  */
diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index ebaeb5e86895..9e8572fbee50 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -39,11 +39,9 @@ get_thread_regcache (struct thread_info *thread, int fetch)
      have.  */
   if (regcache == NULL)
     {
-      struct process_info *proc = get_thread_process (thread);
+      gdb_assert (thread->tdesc != NULL);
 
-      gdb_assert (proc->tdesc != NULL);
-
-      regcache = new_register_cache (proc->tdesc);
+      regcache = new_register_cache (thread->tdesc);
       set_thread_regcache_data (thread, regcache);
     }
 
diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
index 5693cc6626fb..17666c33661f 100644
--- a/gdbserver/tdesc.cc
+++ b/gdbserver/tdesc.cc
@@ -129,7 +129,7 @@ current_target_desc (void)
   if (current_thread == NULL)
     return &default_description;
 
-  return current_process ()->tdesc;
+  return current_thread->tdesc;
 }
 
 /* An empty structure.  */

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 6/8] gdbserver/linux-aarch64: When inferior stops, update its target description
  2022-09-08  6:41 [PATCH 0/8] gdbserver improvements for AArch64 SVE support Thiago Jung Bauermann
                   ` (4 preceding siblings ...)
  2022-09-08  6:41 ` [PATCH 5/8] gdbserver: Move target description from being per-process to being per-thread Thiago Jung Bauermann
@ 2022-09-08  6:41 ` Thiago Jung Bauermann
  2022-09-20  8:48   ` Luis Machado
  2022-09-08  6:41 ` [PATCH 7/8] gdb/aarch64: Factor out most of the thread_architecture method Thiago Jung Bauermann
  2022-09-08  6:41 ` [PATCH 8/8] gdb/aarch64: When remote debugging detect changes in the target description Thiago Jung Bauermann
  7 siblings, 1 reply; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-09-08  6:41 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann

Introduce new arch_update_tdesc method to the linux_process_target class.

This allows aarch64-linux to probe the inferior's vq register and provide
an updated target description reflecting the new vector length.
---
 gdbserver/linux-aarch64-low.cc | 21 +++++++++++++++++++++
 gdbserver/linux-low.cc         | 18 ++++++++++++++++++
 gdbserver/linux-low.h          |  5 +++++
 3 files changed, 44 insertions(+)

diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index 8c4306ab8794..cc90eae1e36b 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;
@@ -902,6 +905,24 @@ 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)
+{
+  gdb::optional<struct aarch64_features> features
+      = aarch64_get_arch_features (thread);
+
+  if (!features.has_value())
+    return {};
+
+  /* Adjust the register sets we should use for this particular set of
+     features.  */
+  aarch64_adjust_register_sets(*features);
+
+  return aarch64_linux_read_description (*features);
+}
+
 /* 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 d37eb120e114..2a7c6f166a65 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)
@@ -2354,6 +2360,18 @@ linux_process_target::filter_event (int lwpid, int wstat)
 	      return;
 	    }
 	}
+      else
+	{
+	  /* Give the arch code an opportunity to update the target
+	     description.  */
+	  gdb::optional<const struct target_desc *> new_tdesc
+	      = arch_update_tdesc (thread);
+	  if (new_tdesc.has_value () && *new_tdesc != thread->tdesc)
+	    {
+	      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 bd1ec31dc5a4..fba462744953 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -602,6 +602,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 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;

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 7/8] gdb/aarch64: Factor out most of the thread_architecture method
  2022-09-08  6:41 [PATCH 0/8] gdbserver improvements for AArch64 SVE support Thiago Jung Bauermann
                   ` (5 preceding siblings ...)
  2022-09-08  6:41 ` [PATCH 6/8] gdbserver/linux-aarch64: When inferior stops, update its target description Thiago Jung Bauermann
@ 2022-09-08  6:41 ` Thiago Jung Bauermann
  2022-09-20  8:52   ` Luis Machado
  2022-09-08  6:41 ` [PATCH 8/8] gdb/aarch64: When remote debugging detect changes in the target description Thiago Jung Bauermann
  7 siblings, 1 reply; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-09-08  6:41 UTC (permalink / raw)
  To: gdb-patches; +Cc: 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 | 25 +------------------------
 gdb/aarch64-tdep.c      | 34 ++++++++++++++++++++++++++++++++++
 gdb/aarch64-tdep.h      |  2 ++
 3 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 633cbc08796c..ff4c69f6af7d 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -891,36 +891,13 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
   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);
   int vq = aarch64_sve_get_vq (ptid.lwp ());
 
   /* If ptrace fails we can't determine vq, but the thread_architecture method
      always succeeds so all we can do here is assert that vq is valid.  */
   gdb_assert (vq >= 0);
-  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 7229b53838e8..18c2b1ec1523 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3420,6 +3420,40 @@ 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_create_target_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 d8513023c376..3d78515e167f 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -139,4 +139,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 */

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 8/8] gdb/aarch64: When remote debugging detect changes in the target description
  2022-09-08  6:41 [PATCH 0/8] gdbserver improvements for AArch64 SVE support Thiago Jung Bauermann
                   ` (6 preceding siblings ...)
  2022-09-08  6:41 ` [PATCH 7/8] gdb/aarch64: Factor out most of the thread_architecture method Thiago Jung Bauermann
@ 2022-09-08  6:41 ` Thiago Jung Bauermann
  2022-09-20  8:59   ` Luis Machado
  7 siblings, 1 reply; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-09-08  6:41 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann

If the remote target provides an expedited VG register, use it to update
its target description.  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        | 28 +++++++++++++++++++++++++-
 gdb/arch-utils.c          |  9 +++++++++
 gdb/arch-utils.h          |  5 +++++
 gdb/gdbarch-components.py | 16 +++++++++++++++
 gdb/gdbarch-gen.h         | 11 ++++++++++
 gdb/gdbarch.c             | 23 +++++++++++++++++++++
 gdb/remote.c              | 42 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 18c2b1ec1523..acfd84ea3f33 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3420,7 +3420,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.  */
@@ -3454,6 +3455,30 @@ 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)
+{
+  gdb_assert (gdbarch != NULL);
+
+  /* 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
@@ -3667,6 +3692,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 9bd4f0ddae66..1c75d100b55b 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1092,6 +1092,15 @@ default_read_core_file_mappings
 {
 }
 
+/* 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 f850e5fd6e78..f81e0ad78fa3 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -300,4 +300,9 @@ extern void default_read_core_file_mappings
    struct bfd *cbfd,
    read_core_file_mappings_pre_loop_ftype pre_loop_cb,
    read_core_file_mappings_loop_ftype loop_cb);
+
+/* 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 71aa5991fbe0..5a23b2ac48b7 100644
--- a/gdb/gdbarch-components.py
+++ b/gdb/gdbarch-components.py
@@ -2652,3 +2652,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 0504962e50d2..b543a9878930 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1627,3 +1627,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 0edae7f6f0a2..4c9482f8fbf7 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -253,6 +253,7 @@ struct gdbarch
   gdbarch_type_align_ftype *type_align = nullptr;
   gdbarch_get_pc_address_flags_ftype *get_pc_address_flags = nullptr;
   gdbarch_read_core_file_mappings_ftype *read_core_file_mappings = nullptr;
+  gdbarch_update_architecture_ftype *update_architecture = nullptr;
 };
 
 /* Create a new ``struct gdbarch'' based on information provided by
@@ -368,6 +369,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->type_align = default_type_align;
   gdbarch->get_pc_address_flags = default_get_pc_address_flags;
   gdbarch->read_core_file_mappings = default_read_core_file_mappings;
+  gdbarch->update_architecture = default_update_architecture;
   /* gdbarch_alloc() */
 
   return gdbarch;
@@ -605,6 +607,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 (__FILE__, __LINE__,
 		    _("verify_gdbarch: the following are invalid ...%s"),
@@ -1438,6 +1441,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);
 }
@@ -5363,3 +5369,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 70f918a7362c..4e6a33dfb686 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:
@@ -8072,6 +8082,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);
 
@@ -14448,6 +14473,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 ()
 {

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/8] gdbserver: Add asserts in register_size and register_data functions
  2022-09-08  6:41 ` [PATCH 1/8] gdbserver: Add asserts in register_size and register_data functions Thiago Jung Bauermann
@ 2022-09-20  7:36   ` Luis Machado
  2022-11-26  1:36     ` Thiago Jung Bauermann
  0 siblings, 1 reply; 25+ messages in thread
From: Luis Machado @ 2022-09-20  7:36 UTC (permalink / raw)
  To: Thiago Jung Bauermann, gdb-patches

On 9/8/22 07:41, 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 27491efc52d5..ebaeb5e86895 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. Someone else must approve, but I suppose it could go in on its own.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/8] gdbserver: Add thread parameter to linux_get_auxv and linux_get_hwcap
  2022-09-08  6:41 ` [PATCH 2/8] gdbserver: Add thread parameter to linux_get_auxv and linux_get_hwcap Thiago Jung Bauermann
@ 2022-09-20  7:43   ` Luis Machado
  2022-11-26  1:37     ` Thiago Jung Bauermann
  0 siblings, 1 reply; 25+ messages in thread
From: Luis Machado @ 2022-09-20  7:43 UTC (permalink / raw)
  To: Thiago Jung Bauermann, gdb-patches

On 9/8/22 07:41, 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
> thread_info parameter seems more correct than setting current_thread in one
> particular code path.
> 
> Changes are propagated to allow passing the thread parameter through the
> call chain.

I suppose the effects will only be known when later patches are applied. But the auxv data is per-process, right?

It sounds a bit strange to pass a thread for the hwcap/auxv lookup. Is there some other reason to do that?

> ---
>   gdbserver/linux-aarch64-low.cc |  6 +++---
>   gdbserver/linux-arm-low.cc     |  2 +-
>   gdbserver/linux-low.cc         | 20 +++++++++++---------
>   gdbserver/linux-low.h          | 10 +++++-----
>   gdbserver/linux-ppc-low.cc     |  6 +++---
>   gdbserver/linux-s390-low.cc    |  2 +-
>   gdbserver/netbsd-low.cc        |  4 ++--
>   gdbserver/netbsd-low.h         |  4 ++--
>   gdbserver/server.cc            |  2 +-
>   gdbserver/target.cc            |  4 ++--
>   gdbserver/target.h             |  4 ++--
>   11 files changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index db5086962612..5d4d667dfa42 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -826,9 +826,9 @@ aarch64_target::low_arch_setup ()
>   
>         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 (current_thread, 8) & AARCH64_HWCAP_PACA;
>         /* A-profile MTE is 64-bit only.  */
> -      features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
> +      features.mte = linux_get_hwcap2 (current_thread, 8) & HWCAP2_MTE;
>         features.tls = true;
>   
>         current_process ()->tdesc = aarch64_linux_read_description (features);
> @@ -3299,7 +3299,7 @@ aarch64_target::supports_memory_tagging ()
>   #endif
>       }
>   
> -  return (linux_get_hwcap2 (8) & HWCAP2_MTE) != 0;
> +  return (linux_get_hwcap2 (current_thread, 8) & HWCAP2_MTE) != 0;
>   }
>   
>   bool
> diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
> index ab6209a3abc8..d40ce412fe1c 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 (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 2f71360d3bde..9034a1d8fabe 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -5486,12 +5486,12 @@ 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 (const thread_info *thread, CORE_ADDR offset,
> +				 unsigned char *myaddr, unsigned int len)
>   {
>     char filename[PATH_MAX];
>     int fd, n;
> -  int pid = lwpid_of (current_thread);
> +  int pid = lwpid_of (thread);
>   
>     xsnprintf (filename, sizeof filename, "/proc/%d/auxv", pid);
>   
> @@ -6912,14 +6912,16 @@ 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 (const thread_info *thread, 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 (thread, offset, data, 2 * wordsize)
> +	 == 2 * wordsize)
>       {
>         if (wordsize == 4)
>   	{
> @@ -6949,20 +6951,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 (const thread_info *thread, int wordsize)
>   {
>     CORE_ADDR hwcap = 0;
> -  linux_get_auxv (wordsize, AT_HWCAP, &hwcap);
> +  linux_get_auxv (thread, wordsize, AT_HWCAP, &hwcap);
>     return hwcap;
>   }
>   
>   /* See linux-low.h.  */
>   
>   CORE_ADDR
> -linux_get_hwcap2 (int wordsize)
> +linux_get_hwcap2 (const thread_info *thread, int wordsize)
>   {
>     CORE_ADDR hwcap2 = 0;
> -  linux_get_auxv (wordsize, AT_HWCAP2, &hwcap2);
> +  linux_get_auxv (thread, wordsize, AT_HWCAP2, &hwcap2);
>     return hwcap2;
>   }
>   
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 79be31b8f729..bd1ec31dc5a4 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -178,8 +178,8 @@ class linux_process_target : public process_stratum_target
>   
>     bool supports_read_auxv () override;
>   
> -  int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> -		 unsigned int len) override;
> +  int read_auxv (const thread_info *thread, CORE_ADDR offset,
> +		 unsigned char *myaddr, unsigned int len) override;
>   
>     int insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
>   		    int size, raw_breakpoint *bp) override;
> @@ -944,17 +944,17 @@ 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,
> +int linux_get_auxv (const thread_info *thread, 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 (const thread_info *thread, 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 (const thread_info *thread, int wordsize);
>   
>   #endif /* GDBSERVER_LINUX_LOW_H */
> diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc
> index 08824887003b..08608b2dcc38 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 (current_thread, features.wordsize);
> +  ppc_hwcap2 = linux_get_hwcap2 (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 (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..0d01dffa64a4 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 (current_thread, 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..71ac61e9dfbd 100644
> --- a/gdbserver/netbsd-low.cc
> +++ b/gdbserver/netbsd-low.cc
> @@ -581,10 +581,10 @@ 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 (const thread_info *thread, CORE_ADDR offset,
>   				  unsigned char *myaddr, unsigned int len)
>   {
> -  pid_t pid = pid_of (current_thread);
> +  pid_t pid = pid_of (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..506329251eaf 100644
> --- a/gdbserver/netbsd-low.h
> +++ b/gdbserver/netbsd-low.h
> @@ -77,8 +77,8 @@ class netbsd_process_target : public process_stratum_target
>   
>     bool supports_read_auxv () override;
>   
> -  int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> -		 unsigned int len) override;
> +  int read_auxv (const thread_info *thread, 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 c619206d5d2d..2d5cf6e8d8de 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 (current_thread, offset, readbuf, len);
>   }
>   
>   /* Handle qXfer:exec-file:read.  */
> diff --git a/gdbserver/target.cc b/gdbserver/target.cc
> index adcfe6e7bcc7..7ee18bde8635 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 (const thread_info *thread, 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 6c536a307786..cd6b2193a228 100644
> --- a/gdbserver/target.h
> +++ b/gdbserver/target.h
> @@ -175,8 +175,8 @@ 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,
> -			 unsigned int len);
> +  virtual int read_auxv (const thread_info *thread, CORE_ADDR offset,
> +			 unsigned char *myaddr, unsigned int len);
>   
>     /* Returns true if GDB Z breakpoint type TYPE is supported, false
>        otherwise.  The type is coded as follows:


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/8] gdb, gdbserver/aarch64-linux: Allow aarch64_sve_get_vq to return error
  2022-09-08  6:41 ` [PATCH 3/8] gdb, gdbserver/aarch64-linux: Allow aarch64_sve_get_vq to return error Thiago Jung Bauermann
@ 2022-09-20  8:07   ` Luis Machado
  2022-11-26  1:42     ` Thiago Jung Bauermann
  0 siblings, 1 reply; 25+ messages in thread
From: Luis Machado @ 2022-09-20  8:07 UTC (permalink / raw)
  To: Thiago Jung Bauermann, gdb-patches

On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote:
> If ptrace fails, aarch64_sve_get_vq assumes that SVE isn't supported.
> Because in a subsequent change this function will need to be called from
> low_new_thread (which can be called whe the inferior thread isn't stopped),
> it will need to distinguish between ptrace errors due to SVE not being
> supported from ptrace errors due to the inferior thread not being stopped.

My understanding is that we will always be able to validate supported features when we start/attach to a process. So if, say, we detect SVE
support through HWCAP's in the beginning of the debugging session, that will still hold for any thread that is spawned. What may change later
on is the VL (still, very unlikely).

When a thread is spawned, it will inherit SVE settings from its parent. From the Linux Kernel SVE documentation:

   In particular, on return from a fork() or clone(), the parent and new child
   process or thread share identical SVE configuration, matching that of the
   parent before the call.

Is that something we can use here? I suppose there may be corner cases where the parent spawned a thread and changed its VL size etc.

If we can't and a thread is running, we won't be able to tell the VL, but we have a state that is meant to indicate "unknown VL", don't we? Maybe I'm misremembering,
but -1 used to indicate that.

> 
> This patch changes the function to return -1 in the latter case and adjusts
> callers.  When a caller is allowed to propagate the error, it does so.  When
> that isn't possible an assertion is added to ensure the error isn't missed.
> ---
>   gdb/aarch64-linux-nat.c            | 14 ++++++++++++--
>   gdb/nat/aarch64-sve-linux-ptrace.c | 18 ++++++++++++------
>   gdb/nat/aarch64-sve-linux-ptrace.h |  2 +-
>   gdbserver/linux-aarch64-low.cc     |  6 +++++-
>   4 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index eda79ec6d35c..633cbc08796c 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -784,8 +784,14 @@ aarch64_linux_nat_target::read_description ()
>     CORE_ADDR hwcap = linux_get_hwcap (this);
>     CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
>   
> +  int vq = aarch64_sve_get_vq (tid);
> +  if (vq < 0)
> +    /* A ptrace error happened so we can't determine the vq value.
> +       Give up trying to read a target description.  */
> +    return nullptr;
> +
>     aarch64_features features;
> -  features.vq = aarch64_sve_get_vq (tid);
> +  features.vq = vq;
>     features.pauth = hwcap & AARCH64_HWCAP_PACA;
>     features.mte = hwcap2 & HWCAP2_MTE;
>     features.tls = true;
> @@ -894,7 +900,11 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>     /* 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 ());
> +  int vq = aarch64_sve_get_vq (ptid.lwp ());
> +
> +  /* If ptrace fails we can't determine vq, but the thread_architecture method
> +     always succeeds so all we can do here is assert that vq is valid.  */
> +  gdb_assert (vq >= 0);
>     if (vq == tdep->vq)
>       return inf->gdbarch;
>   
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
> index 019d2d65d89a..62f7fc5f56e1 100644
> --- a/gdb/nat/aarch64-sve-linux-ptrace.c
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
> @@ -30,7 +30,7 @@
>   
>   /* See nat/aarch64-sve-linux-ptrace.h.  */
>   
> -uint64_t
> +int
>   aarch64_sve_get_vq (int tid)
>   {
>     struct iovec iovec;
> @@ -43,13 +43,19 @@ aarch64_sve_get_vq (int tid)
>        128bit chunks in a Z register.  We use VQ because 128bits is the minimum
>        a Z register can increase in size.  */
>   
> +  errno = 0;
>     if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
>       {
> +      if (errno == ESRCH)
> +	/* The process isn't stopped.  We can't determine SVE support or the
> +	   value of vq.  */
> +	return -1;
> +
>         /* SVE is not supported.  */
>         return 0;
>       }
>   
> -  uint64_t vq = sve_vq_from_vl (header.vl);
> +  int vq = sve_vq_from_vl (header.vl);
>   
>     if (!sve_vl_valid (header.vl))
>       {
> @@ -103,10 +109,10 @@ aarch64_sve_set_vq (int tid, struct reg_buffer_common *reg_buf)
>       {
>         /* If vg is not available yet, fetch it from ptrace.  The VG value from
>   	 ptrace is likely the correct one.  */
> -      uint64_t vq = aarch64_sve_get_vq (tid);
> +      int vq = aarch64_sve_get_vq (tid);
>   
>         /* If something went wrong, just bail out.  */
> -      if (vq == 0)
> +      if (vq <= 0)
>   	return false;
>   
>         reg_vg = sve_vg_from_vq (vq);
> @@ -123,9 +129,9 @@ std::unique_ptr<gdb_byte[]>
>   aarch64_sve_get_sveregs (int tid)
>   {
>     struct iovec iovec;
> -  uint64_t vq = aarch64_sve_get_vq (tid);
> +  int vq = aarch64_sve_get_vq (tid);
>   
> -  if (vq == 0)
> +  if (vq <= 0)
>       perror_with_name (_("Unable to fetch SVE register header"));
>   
>     /* A ptrace call with NT_ARM_SVE will return a header followed by either a
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
> index 5c264b395313..90920bc48ef3 100644
> --- a/gdb/nat/aarch64-sve-linux-ptrace.h
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.h
> @@ -43,7 +43,7 @@
>   /* Read VQ for the given tid using ptrace.  If SVE is not supported then zero
>      is returned (on a system that supports SVE, then VQ cannot be zero).  */
>   
> -uint64_t aarch64_sve_get_vq (int tid);
> +int aarch64_sve_get_vq (int tid);
>   
>   /* Set VQ in the kernel for the given tid, using either the value VQ or
>      reading from the register VG in the register buffer.  */
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index 5d4d667dfa42..576925838f49 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -823,8 +823,12 @@ aarch64_target::low_arch_setup ()
>     if (is_elf64)
>       {
>         struct aarch64_features features;
> +      int vq = aarch64_sve_get_vq (tid);
>   
> -      features.vq = aarch64_sve_get_vq (tid);
> +      /* If ptrace fails we can't determine vq, but the low_arch_setup method
> +	  always succeeds so all we can do here is assert that vq is valid.  */
> +      gdb_assert (vq >= 0);
> +      features.vq = vq;
>         /* A-profile PAC is 64-bit only.  */
>         features.pauth = linux_get_hwcap (current_thread, 8) & AARCH64_HWCAP_PACA;
>         /* A-profile MTE is 64-bit only.  */


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 4/8] gdbserver/linux-aarch64: Factor out function to get aarch64_features
  2022-09-08  6:41 ` [PATCH 4/8] gdbserver/linux-aarch64: Factor out function to get aarch64_features Thiago Jung Bauermann
@ 2022-09-20  8:08   ` Luis Machado
  2022-11-26  1:44     ` Thiago Jung Bauermann
  0 siblings, 1 reply; 25+ messages in thread
From: Luis Machado @ 2022-09-20  8:08 UTC (permalink / raw)
  To: Thiago Jung Bauermann, gdb-patches

On 9/8/22 07:41, 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 | 45 ++++++++++++++++++++++------------
>   1 file changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index 576925838f49..9b57be73818e 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -652,6 +652,28 @@ 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 gdb::optional<struct aarch64_features>
> +aarch64_get_arch_features (const thread_info *thread)
> +{
> +  struct aarch64_features features;
> +  int vq = aarch64_sve_get_vq (thread->id.lwp ());
> +
> +  if (vq < 0)
> +    return {};

If we can't identify SVE properly, should we carry on and disable SVE support but enable all the other
features we can find?

> +
> +  features.vq = vq;
> +  /* A-profile PAC is 64-bit only.  */
> +  features.pauth = linux_get_hwcap (thread, 8) & AARCH64_HWCAP_PACA;
> +  /* A-profile MTE is 64-bit only.  */
> +  features.mte = linux_get_hwcap2 (thread, 8) & HWCAP2_MTE;
> +  features.tls = true;
> +
> +  return features;
> +}
> +
>   void
>   aarch64_target::low_new_thread (lwp_info *lwp)
>   {
> @@ -804,9 +826,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,24 +841,18 @@ aarch64_target::low_arch_setup ()
>   
>     if (is_elf64)
>       {
> -      struct aarch64_features features;
> -      int vq = aarch64_sve_get_vq (tid);
> +      gdb::optional<struct aarch64_features> features
> +	  = aarch64_get_arch_features (current_thread);
>   
> -      /* If ptrace fails we can't determine vq, but the low_arch_setup method
> -	  always succeeds so all we can do here is assert that vq is valid.  */
> -      gdb_assert (vq >= 0);
> -      features.vq = vq;
> -      /* A-profile PAC is 64-bit only.  */
> -      features.pauth = linux_get_hwcap (current_thread, 8) & AARCH64_HWCAP_PACA;
> -      /* A-profile MTE is 64-bit only.  */
> -      features.mte = linux_get_hwcap2 (current_thread, 8) & HWCAP2_MTE;
> -      features.tls = true;
> +      /* If ptrace fails we can't determine vq, but low_arch_setup always
> +	 succeeds so all we can do here is assert that features is valid.  */
> +      gdb_assert (features.has_value ());
>   
> -      current_process ()->tdesc = aarch64_linux_read_description (features);
> +      current_process ()->tdesc = aarch64_linux_read_description (*features);
>   
>         /* Adjust the register sets we should use for this particular set of
>   	 features.  */
> -      aarch64_adjust_register_sets (features);
> +      aarch64_adjust_register_sets (*features);
>       }
>     else
>       current_process ()->tdesc = aarch32_linux_read_description ();


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 6/8] gdbserver/linux-aarch64: When inferior stops, update its target description
  2022-09-08  6:41 ` [PATCH 6/8] gdbserver/linux-aarch64: When inferior stops, update its target description Thiago Jung Bauermann
@ 2022-09-20  8:48   ` Luis Machado
  2022-11-26  1:49     ` Thiago Jung Bauermann
  0 siblings, 1 reply; 25+ messages in thread
From: Luis Machado @ 2022-09-20  8:48 UTC (permalink / raw)
  To: Thiago Jung Bauermann, gdb-patches

On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote:
> Introduce new arch_update_tdesc method to the linux_process_target class.
> 
> This allows aarch64-linux to probe the inferior's vq register and provide
> an updated target description reflecting the new vector length.
> ---
>   gdbserver/linux-aarch64-low.cc | 21 +++++++++++++++++++++
>   gdbserver/linux-low.cc         | 18 ++++++++++++++++++
>   gdbserver/linux-low.h          |  5 +++++
>   3 files changed, 44 insertions(+)
> 
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index 8c4306ab8794..cc90eae1e36b 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;
> @@ -902,6 +905,24 @@ 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)
> +{
> +  gdb::optional<struct aarch64_features> features
> +      = aarch64_get_arch_features (thread);
> +
> +  if (!features.has_value())
> +    return {};
> +
> +  /* Adjust the register sets we should use for this particular set of
> +     features.  */
> +  aarch64_adjust_register_sets(*features);
> +
> +  return aarch64_linux_read_description (*features);
> +}
> +
>   /* 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 d37eb120e114..2a7c6f166a65 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)
> @@ -2354,6 +2360,18 @@ linux_process_target::filter_event (int lwpid, int wstat)
>   	      return;
>   	    }
>   	}
> +      else
> +	{
> +	  /* Give the arch code an opportunity to update the target
> +	     description.  */
> +	  gdb::optional<const struct target_desc *> new_tdesc
> +	      = arch_update_tdesc (thread);
> +	  if (new_tdesc.has_value () && *new_tdesc != thread->tdesc)
> +	    {
> +	      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 bd1ec31dc5a4..fba462744953 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -602,6 +602,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 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;

LGTM for the aarch64 parts. Someone else must approve the generic changes.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 7/8] gdb/aarch64: Factor out most of the thread_architecture method
  2022-09-08  6:41 ` [PATCH 7/8] gdb/aarch64: Factor out most of the thread_architecture method Thiago Jung Bauermann
@ 2022-09-20  8:52   ` Luis Machado
  2022-11-26  1:49     ` Thiago Jung Bauermann
  0 siblings, 1 reply; 25+ messages in thread
From: Luis Machado @ 2022-09-20  8:52 UTC (permalink / raw)
  To: Thiago Jung Bauermann, gdb-patches

On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches 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 | 25 +------------------------
>   gdb/aarch64-tdep.c      | 34 ++++++++++++++++++++++++++++++++++
>   gdb/aarch64-tdep.h      |  2 ++
>   3 files changed, 37 insertions(+), 24 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 633cbc08796c..ff4c69f6af7d 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -891,36 +891,13 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>     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);
>     int vq = aarch64_sve_get_vq (ptid.lwp ());
>   
>     /* If ptrace fails we can't determine vq, but the thread_architecture method
>        always succeeds so all we can do here is assert that vq is valid.  */
>     gdb_assert (vq >= 0);
> -  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 7229b53838e8..18c2b1ec1523 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3420,6 +3420,40 @@ 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_create_target_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 d8513023c376..3d78515e167f 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -139,4 +139,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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 8/8] gdb/aarch64: When remote debugging detect changes in the target description
  2022-09-08  6:41 ` [PATCH 8/8] gdb/aarch64: When remote debugging detect changes in the target description Thiago Jung Bauermann
@ 2022-09-20  8:59   ` Luis Machado
  2022-11-26  1:50     ` Thiago Jung Bauermann
  0 siblings, 1 reply; 25+ messages in thread
From: Luis Machado @ 2022-09-20  8:59 UTC (permalink / raw)
  To: Thiago Jung Bauermann, gdb-patches

On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote:
> If the remote target provides an expedited VG register, use it to update
> its target description.  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        | 28 +++++++++++++++++++++++++-
>   gdb/arch-utils.c          |  9 +++++++++
>   gdb/arch-utils.h          |  5 +++++
>   gdb/gdbarch-components.py | 16 +++++++++++++++
>   gdb/gdbarch-gen.h         | 11 ++++++++++
>   gdb/gdbarch.c             | 23 +++++++++++++++++++++
>   gdb/remote.c              | 42 +++++++++++++++++++++++++++++++++++++++
>   7 files changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 18c2b1ec1523..acfd84ea3f33 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3420,7 +3420,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.  */
> @@ -3454,6 +3455,30 @@ 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)
> +{
> +  gdb_assert (gdbarch != NULL);
> +
> +  /* 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
> @@ -3667,6 +3692,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 9bd4f0ddae66..1c75d100b55b 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1092,6 +1092,15 @@ default_read_core_file_mappings
>   {
>   }
>   
> +/* 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 f850e5fd6e78..f81e0ad78fa3 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -300,4 +300,9 @@ extern void default_read_core_file_mappings
>      struct bfd *cbfd,
>      read_core_file_mappings_pre_loop_ftype pre_loop_cb,
>      read_core_file_mappings_loop_ftype loop_cb);
> +
> +/* 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 71aa5991fbe0..5a23b2ac48b7 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -2652,3 +2652,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 0504962e50d2..b543a9878930 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1627,3 +1627,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 0edae7f6f0a2..4c9482f8fbf7 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -253,6 +253,7 @@ struct gdbarch
>     gdbarch_type_align_ftype *type_align = nullptr;
>     gdbarch_get_pc_address_flags_ftype *get_pc_address_flags = nullptr;
>     gdbarch_read_core_file_mappings_ftype *read_core_file_mappings = nullptr;
> +  gdbarch_update_architecture_ftype *update_architecture = nullptr;
>   };
>   
>   /* Create a new ``struct gdbarch'' based on information provided by
> @@ -368,6 +369,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
>     gdbarch->type_align = default_type_align;
>     gdbarch->get_pc_address_flags = default_get_pc_address_flags;
>     gdbarch->read_core_file_mappings = default_read_core_file_mappings;
> +  gdbarch->update_architecture = default_update_architecture;
>     /* gdbarch_alloc() */
>   
>     return gdbarch;
> @@ -605,6 +607,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 (__FILE__, __LINE__,
>   		    _("verify_gdbarch: the following are invalid ...%s"),
> @@ -1438,6 +1441,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);
>   }
> @@ -5363,3 +5369,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 70f918a7362c..4e6a33dfb686 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:
> @@ -8072,6 +8082,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);
>   
> @@ -14448,6 +14473,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 ()
>   {

LGTM for the aarch64 code. Someone else must approve the generic parts.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/8] gdbserver: Move target description from being per-process to being per-thread
  2022-09-08  6:41 ` [PATCH 5/8] gdbserver: Move target description from being per-process to being per-thread Thiago Jung Bauermann
@ 2022-09-20 11:21   ` Luis Machado
  2022-11-26  1:47     ` Thiago Jung Bauermann
  0 siblings, 1 reply; 25+ messages in thread
From: Luis Machado @ 2022-09-20 11:21 UTC (permalink / raw)
  To: Thiago Jung Bauermann, gdb-patches

On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote:
> This change allows aarch64-linux to support debugging programs where
> different threads have different SVE vector length.  This requires gdbserver
> to support different inferior threads having different target descriptions.
> 
> After this change, all targets except aarch64-linux will still use one
> target description for all threads in the inferior process (their tdesc
> member in struct thread_info will point to the same target description).
> This is done by linux_process_target::handle_extended_wait which copies the
> target description from the original thread to the new thread if
> low_new_thread doesn't provide one.
> 
> In the case of aarch64-linux, the low_new_thread method probes the thread's
> architecture features and assigns a description to it.
> ---
>   gdbserver/gdbthread.h            |  2 ++
>   gdbserver/inferiors.h            |  2 --
>   gdbserver/linux-aarch64-low.cc   | 48 ++++++++++++++++++++++++++++++--
>   gdbserver/linux-arc-low.cc       |  6 ++--
>   gdbserver/linux-arm-low.cc       |  4 +--
>   gdbserver/linux-ia64-low.cc      |  2 +-
>   gdbserver/linux-loongarch-low.cc |  2 +-
>   gdbserver/linux-low.cc           | 24 ++++++++++------
>   gdbserver/linux-m68k-low.cc      |  2 +-
>   gdbserver/linux-mips-low.cc      |  8 +++---
>   gdbserver/linux-nios2-low.cc     |  2 +-
>   gdbserver/linux-or1k-low.cc      |  2 +-
>   gdbserver/linux-ppc-low.cc       | 10 +++----
>   gdbserver/linux-riscv-low.cc     |  2 +-
>   gdbserver/linux-s390-low.cc      |  4 +--
>   gdbserver/linux-sh-low.cc        |  2 +-
>   gdbserver/linux-sparc-low.cc     |  2 +-
>   gdbserver/linux-tic6x-low.cc     |  2 +-
>   gdbserver/linux-x86-low.cc       |  2 +-
>   gdbserver/linux-xtensa-low.cc    |  2 +-
>   gdbserver/netbsd-aarch64-low.cc  |  2 +-
>   gdbserver/netbsd-amd64-low.cc    |  2 +-
>   gdbserver/netbsd-i386-low.cc     |  2 +-
>   gdbserver/regcache.cc            |  6 ++--
>   gdbserver/tdesc.cc               |  2 +-
>   25 files changed, 95 insertions(+), 49 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 keep a process-wide target description in case we fail to detect the target description for
a given thread? Also, it may simplify some of the changes in this patch.

Features detected in the process will always be present in the thread for aarch64. It is just the vector length of SVE
that might be different.

>   };
>   
>   extern std::list<thread_info *> all_threads;
> diff --git a/gdbserver/inferiors.h b/gdbserver/inferiors.h
> index 6de746cb228f..cdb39dd90d54 100644
> --- a/gdbserver/inferiors.h
> +++ b/gdbserver/inferiors.h
> @@ -65,8 +65,6 @@ struct process_info
>        for unfiltered syscall reporting.  */
>     std::vector<int> syscalls_to_catch;
>   
> -  const struct target_desc *tdesc = NULL;
> -
>     /* Private target data.  */
>     struct process_info_private *priv = NULL;
>   
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index 9b57be73818e..8c4306ab8794 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -191,9 +191,26 @@ struct arch_process_info
>   static int
>   is_64bit_tdesc (void)
>   {
> +  const target_desc *tdesc;
> +
> +  if (current_thread && current_thread->tdesc)
> +    tdesc = current_thread->tdesc;
> +  else
> +    {
> +      /* Find some thread that has a target description.  */
> +      const struct thread_info *thread
> +	  = find_thread (current_process ()->pid, [] (thread_info *thr) {
> +	      return thr->tdesc != nullptr;
> +	    });
> +
> +      gdb_assert (thread != nullptr);
> +
> +      tdesc = thread->tdesc;
> +    }
> +
>     /* We may not have a current thread at this point, so go straight to
>        the process's target description.  */
> -  return register_size (current_process ()->tdesc, 0) == 8;
> +  return register_size (tdesc, 0) == 8;
>   }

Keeping the process-wide target description would simplify this function in my opinion. We won't
have 64-bit processes spawning 32-bit threads.

>   
>   static void
> @@ -678,6 +695,31 @@ void
>   aarch64_target::low_new_thread (lwp_info *lwp)
>   {
>     aarch64_linux_new_thread (lwp);
> +
> +  ptid_t ptid = ptid_of_lwp (lwp);
> +  process_info *proc = find_process_pid (ptid.pid ());
> +
> +  /* If the inferior is still going through the shell or the thread isn't
> +     stopped we can't read its registers in order to read the target
> +     description.  */
> +  if (proc->starting_up)
> +    return;
> +
> +  thread_info *thread = get_lwp_thread (lwp);
> +  unsigned int machine;
> +  gdb_assert (ptid.lwp_p ());
> +  bool is_elf64 = linux_pid_exe_is_elf_64_file (ptid.lwp (), &machine);
> +
> +  if (is_elf64)
> +    {
> +      gdb::optional<struct aarch64_features> features
> +	  = aarch64_get_arch_features (thread);
> +
> +      if (features.has_value())
> +	thread->tdesc = aarch64_linux_read_description (*features);
> +    }
> +  else
> +    thread->tdesc = aarch32_linux_read_description ();
>   }
>   
>   void
> @@ -848,14 +890,14 @@ aarch64_target::low_arch_setup ()
>   	 succeeds so all we can do here is assert that features is valid.  */
>         gdb_assert (features.has_value ());
>   
> -      current_process ()->tdesc = aarch64_linux_read_description (*features);
> +      current_thread->tdesc = aarch64_linux_read_description (*features);
>   
>         /* Adjust the register sets we should use for this particular set of
>   	 features.  */
>         aarch64_adjust_register_sets (*features);
>       }
>     else
> -    current_process ()->tdesc = aarch32_linux_read_description ();
> +    current_thread->tdesc = aarch32_linux_read_description ();

Given 32-bit Arm processes will have the same target description throughout their lives, would it make
sense to have a single target description stored process-wide, and whenever we need the description we can
find it there?

Maybe code it as:

return current_thread->tdesc == nullptr? current_process ()->tdesc : current_thread->tdesc;

This would make the changes to all the other targets unnecessary, as those would be able to lookup their
target descriptions from the process instead of the threads.

>   
>     aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
>   }
> diff --git a/gdbserver/linux-arc-low.cc b/gdbserver/linux-arc-low.cc
> index fee1dd33eb20..1727bdede452 100644
> --- a/gdbserver/linux-arc-low.cc
> +++ b/gdbserver/linux-arc-low.cc
> @@ -123,19 +123,19 @@ arc_linux_read_description (void)
>   void
>   arc_target::low_arch_setup ()
>   {
> -  current_process ()->tdesc = arc_linux_read_description ();
> +  current_thread->tdesc = arc_linux_read_description ();
>   }
>   
>   bool
>   arc_target::low_cannot_fetch_register (int regno)
>   {
> -  return (regno >= current_process ()->tdesc->reg_defs.size ());
> +  return (regno >= current_thread->tdesc->reg_defs.size ());
>   }
>   
>   bool
>   arc_target::low_cannot_store_register (int regno)
>   {
> -  return (regno >= current_process ()->tdesc->reg_defs.size ());
> +  return (regno >= current_thread->tdesc->reg_defs.size ());
>   }
>   
>   /* This works for both endianness.  Below you see an illustration of how
> diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
> index d40ce412fe1c..5f52e061bcf6 100644
> --- a/gdbserver/linux-arm-low.cc
> +++ b/gdbserver/linux-arm-low.cc
> @@ -998,7 +998,7 @@ arm_target::low_arch_setup ()
>     /* Query hardware watchpoint/breakpoint capabilities.  */
>     arm_linux_init_hwbp_cap (tid);
>   
> -  current_process ()->tdesc = arm_read_description ();
> +  current_thread->tdesc = arm_read_description ();
>   
>     iov.iov_base = gpregs;
>     iov.iov_len = sizeof (gpregs);
> @@ -1118,7 +1118,7 @@ static struct regs_info regs_info_arm =
>   const regs_info *
>   arm_target::get_regs_info ()
>   {
> -  const struct target_desc *tdesc = current_process ()->tdesc;
> +  const struct target_desc *tdesc = current_thread->tdesc;
>   
>     if (have_ptrace_getregset == 1
>         && (is_aarch32_linux_description (tdesc)
> diff --git a/gdbserver/linux-ia64-low.cc b/gdbserver/linux-ia64-low.cc
> index 4530a283b376..3095038fc50e 100644
> --- a/gdbserver/linux-ia64-low.cc
> +++ b/gdbserver/linux-ia64-low.cc
> @@ -382,7 +382,7 @@ ia64_target::get_regs_info ()
>   void
>   ia64_target::low_arch_setup ()
>   {
> -  current_process ()->tdesc = tdesc_ia64;
> +  current_thread->tdesc = tdesc_ia64;
>   }
>   
>   /* The linux target ops object.  */
> diff --git a/gdbserver/linux-loongarch-low.cc b/gdbserver/linux-loongarch-low.cc
> index cccf1ba780b3..50cca5b2da10 100644
> --- a/gdbserver/linux-loongarch-low.cc
> +++ b/gdbserver/linux-loongarch-low.cc
> @@ -86,7 +86,7 @@ loongarch_target::low_arch_setup ()
>   
>     if (!tdesc->expedite_regs)
>       init_target_desc (tdesc.get (), expedite_regs);
> -  current_process ()->tdesc = tdesc.release ();
> +  current_thread->tdesc = tdesc.release ();
>   }
>   
>   /* Collect GPRs from REGCACHE into BUF.  */
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 9034a1d8fabe..d37eb120e114 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -585,8 +585,8 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>   	  clone_all_breakpoints (child_thr, event_thr);
>   
>   	  target_desc_up tdesc = allocate_target_description ();
> -	  copy_target_description (tdesc.get (), parent_proc->tdesc);
> -	  child_proc->tdesc = tdesc.release ();
> +	  copy_target_description (tdesc.get (), event_thr->tdesc);
> +	  child_thr->tdesc = tdesc.release ();
>   
>   	  /* Clone arch-specific process data.  */
>   	  low_new_fork (parent_proc, child_proc);
> @@ -642,6 +642,11 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>   	 before calling resume_one_lwp.  */
>         new_lwp->stopped = 1;
>   
> +      /* If low_new_thread () wasn't able to set the target description, copy
> +	 it from the original thread.  */
> +      if (new_lwp->thread->tdesc == nullptr)
> +	new_lwp->thread->tdesc = event_thr->tdesc;
> +
>         /* If we're suspending all threads, leave this one suspended
>   	 too.  If the fork/clone parent is stepping over a breakpoint,
>   	 all other threads have been suspended already.  Leave the
> @@ -1209,7 +1214,7 @@ linux_process_target::attach (unsigned long pid)
>   
>         async_file_mark ();
>   
> -      gdb_assert (proc->tdesc != NULL);
> +      gdb_assert (initial_thread->tdesc != NULL);
>       }
>   
>     return 0;
> @@ -2330,7 +2335,7 @@ linux_process_target::filter_event (int lwpid, int wstat)
>   
>         /* Architecture-specific setup after inferior is running.  */
>         proc = find_process_pid (pid_of (thread));
> -      if (proc->tdesc == NULL)
> +      if (thread->tdesc == NULL || proc->starting_up)
>   	{
>   	  if (proc->attached)
>   	    {
> @@ -3966,14 +3971,13 @@ linux_process_target::resume_one_lwp_throw (lwp_info *lwp, int step,
>   {
>     struct thread_info *thread = get_lwp_thread (lwp);
>     int ptrace_request;
> -  struct process_info *proc = get_thread_process (thread);
>   
>     /* Note that target description may not be initialised
> -     (proc->tdesc == NULL) at this point because the program hasn't
> +     (thread->tdesc == NULL) at this point because the program hasn't
>        stopped at the first instruction yet.  It means GDBserver skips
>        the extra traps from the wrapper program (see option --wrapper).
>        Code in this function that requires register access should be
> -     guarded by proc->tdesc == NULL or something else.  */
> +     guarded by thread->tdesc == NULL or something else.  */
>   
>     if (lwp->stopped == 0)
>       return;
> @@ -4090,7 +4094,9 @@ linux_process_target::resume_one_lwp_throw (lwp_info *lwp, int step,
>         step = single_step (lwp);
>       }
>   
> -  if (proc->tdesc != NULL && low_supports_breakpoints ())
> +  process_info *proc = find_process_pid(thread->id.pid());
> +
> +  if (!proc->starting_up && low_supports_breakpoints ())
>       {
>         struct regcache *regcache = get_thread_regcache (current_thread, 1);
>   
> @@ -4349,7 +4355,7 @@ linux_process_target::thread_needs_step_over (thread_info *thread)
>   
>     /* GDBserver is skipping the extra traps from the wrapper program,
>        don't have to do step over.  */
> -  if (proc->tdesc == NULL)
> +  if (proc->starting_up)
>       return false;
>   
>     /* LWPs which will not be resumed are not interesting, because we
> diff --git a/gdbserver/linux-m68k-low.cc b/gdbserver/linux-m68k-low.cc
> index 7a433ffab5ef..5b17e21c5258 100644
> --- a/gdbserver/linux-m68k-low.cc
> +++ b/gdbserver/linux-m68k-low.cc
> @@ -253,7 +253,7 @@ m68k_target::get_regs_info ()
>   void
>   m68k_target::low_arch_setup ()
>   {
> -  current_process ()->tdesc = tdesc_m68k;
> +  current_thread->tdesc = tdesc_m68k;
>   }
>   
>   /* The linux target ops object.  */
> diff --git a/gdbserver/linux-mips-low.cc b/gdbserver/linux-mips-low.cc
> index e72403bd91cb..6d9d110e412c 100644
> --- a/gdbserver/linux-mips-low.cc
> +++ b/gdbserver/linux-mips-low.cc
> @@ -213,7 +213,7 @@ mips_read_description (void)
>   void
>   mips_target::low_arch_setup ()
>   {
> -  current_process ()->tdesc = mips_read_description ();
> +  current_thread->tdesc = mips_read_description ();
>   }
>   
>   /* Per-process arch-specific data we want to keep.  */
> @@ -264,7 +264,7 @@ mips_target::low_cannot_fetch_register (int regno)
>     if (get_regs_info ()->usrregs->regmap[regno] == -1)
>       return true;
>   
> -  tdesc = current_process ()->tdesc;
> +  tdesc = current_thread->tdesc;
>   
>     /* On n32 we can't access 64-bit registers via PTRACE_PEEKUSR.  */
>     if (register_size (tdesc, regno) > sizeof (PTRACE_XFER_TYPE))
> @@ -284,7 +284,7 @@ mips_target::low_cannot_store_register (int regno)
>     if (get_regs_info ()->usrregs->regmap[regno] == -1)
>       return true;
>   
> -  tdesc = current_process ()->tdesc;
> +  tdesc = current_thread->tdesc;
>   
>     /* On n32 we can't access 64-bit registers via PTRACE_POKEUSR.  */
>     if (register_size (tdesc, regno) > sizeof (PTRACE_XFER_TYPE))
> @@ -308,7 +308,7 @@ mips_target::low_cannot_store_register (int regno)
>   bool
>   mips_target::low_fetch_register (regcache *regcache, int regno)
>   {
> -  const struct target_desc *tdesc = current_process ()->tdesc;
> +  const struct target_desc *tdesc = current_thread->tdesc;
>   
>     if (find_regno (tdesc, "r0") == regno)
>       {
> diff --git a/gdbserver/linux-nios2-low.cc b/gdbserver/linux-nios2-low.cc
> index e850fcfc1efc..675305f0de51 100644
> --- a/gdbserver/linux-nios2-low.cc
> +++ b/gdbserver/linux-nios2-low.cc
> @@ -119,7 +119,7 @@ static int nios2_regmap[] = {
>   void
>   nios2_target::low_arch_setup ()
>   {
> -  current_process ()->tdesc = tdesc_nios2_linux;
> +  current_thread->tdesc = tdesc_nios2_linux;
>   }
>   
>   /* Implement the low_cannot_fetch_register linux target ops method.  */
> diff --git a/gdbserver/linux-or1k-low.cc b/gdbserver/linux-or1k-low.cc
> index 4c6d69496fb2..0288d634367e 100644
> --- a/gdbserver/linux-or1k-low.cc
> +++ b/gdbserver/linux-or1k-low.cc
> @@ -115,7 +115,7 @@ static int or1k_regmap[] = {
>   void
>   or1k_target::low_arch_setup ()
>   {
> -  current_process ()->tdesc = tdesc_or1k_linux;
> +  current_thread->tdesc = tdesc_or1k_linux;
>   }
>   
>   /* Implement the low_cannot_fetch_register linux target ops method.  */
> diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc
> index 08608b2dcc38..486574bd3dbb 100644
> --- a/gdbserver/linux-ppc-low.cc
> +++ b/gdbserver/linux-ppc-low.cc
> @@ -212,7 +212,7 @@ ppc_check_regset (int tid, int regset_id, int regsetsize)
>   bool
>   ppc_target::low_cannot_store_register (int regno)
>   {
> -  const struct target_desc *tdesc = current_process ()->tdesc;
> +  const struct target_desc *tdesc = current_thread->tdesc;
>   
>   #ifndef __powerpc64__
>     /* Some kernels do not allow us to store fpscr.  */
> @@ -890,9 +890,9 @@ ppc_target::low_arch_setup ()
>     else
>         tdesc = tdesc_powerpc_64l;
>   
> -  current_process ()->tdesc = tdesc;
> +  current_thread->tdesc = tdesc;
>   
> -  /* The value of current_process ()->tdesc needs to be set for this
> +  /* The value of current_thread->tdesc needs to be set for this
>        call.  */
>     ppc_hwcap = linux_get_hwcap (current_thread, features.wordsize);
>     ppc_hwcap2 = linux_get_hwcap2 (current_thread, features.wordsize);
> @@ -952,7 +952,7 @@ ppc_target::low_arch_setup ()
>      }
>   #endif
>   
> -  current_process ()->tdesc = tdesc;
> +  current_thread->tdesc = tdesc;
>   
>     for (regset = ppc_regsets; regset->size >= 0; regset++)
>       switch (regset->get_request)
> @@ -1094,7 +1094,7 @@ is_elfv2_inferior (void)
>     CORE_ADDR phdr;
>     Elf64_Ehdr ehdr;
>   
> -  const struct target_desc *tdesc = current_process ()->tdesc;
> +  const struct target_desc *tdesc = current_thread->tdesc;
>     int wordsize = register_size (tdesc, 0);
>   
>     if (!linux_get_auxv (current_thread, wordsize, AT_PHDR, &phdr))
> diff --git a/gdbserver/linux-riscv-low.cc b/gdbserver/linux-riscv-low.cc
> index 6b2902e422d9..c1c3e5fa5cae 100644
> --- a/gdbserver/linux-riscv-low.cc
> +++ b/gdbserver/linux-riscv-low.cc
> @@ -92,7 +92,7 @@ riscv_target::low_arch_setup ()
>   
>     if (!tdesc->expedite_regs)
>       init_target_desc (tdesc.get (), expedite_regs);
> -  current_process ()->tdesc = tdesc.release ();
> +  current_thread->tdesc = tdesc.release ();
>   }
>   
>   /* Collect GPRs from REGCACHE into BUF.  */
> diff --git a/gdbserver/linux-s390-low.cc b/gdbserver/linux-s390-low.cc
> index 0d01dffa64a4..4d3e81a05c76 100644
> --- a/gdbserver/linux-s390-low.cc
> +++ b/gdbserver/linux-s390-low.cc
> @@ -697,7 +697,7 @@ s390_target::low_arch_setup ()
>   	  break;
>   	}
>   
> -  current_process ()->tdesc = tdesc;
> +  current_thread->tdesc = tdesc;
>   }
>   
>   
> @@ -771,7 +771,7 @@ s390_target::get_regs_info ()
>     if (have_hwcap_s390_high_gprs)
>       {
>   #ifdef __s390x__
> -      const struct target_desc *tdesc = current_process ()->tdesc;
> +      const struct target_desc *tdesc = current_thread->tdesc;
>   
>         if (register_size (tdesc, 0) == 4)
>   	return &regs_info_3264;
> diff --git a/gdbserver/linux-sh-low.cc b/gdbserver/linux-sh-low.cc
> index 966bdeb8ba7a..bee46b8892a3 100644
> --- a/gdbserver/linux-sh-low.cc
> +++ b/gdbserver/linux-sh-low.cc
> @@ -180,7 +180,7 @@ sh_target::get_regs_info ()
>   void
>   sh_target::low_arch_setup ()
>   {
> -  current_process ()->tdesc = tdesc_sh;
> +  current_thread->tdesc = tdesc_sh;
>   }
>   
>   /* The linux target ops object.  */
> diff --git a/gdbserver/linux-sparc-low.cc b/gdbserver/linux-sparc-low.cc
> index d4826d029a9d..7c9d4d456926 100644
> --- a/gdbserver/linux-sparc-low.cc
> +++ b/gdbserver/linux-sparc-low.cc
> @@ -299,7 +299,7 @@ sparc_target::low_breakpoint_at (CORE_ADDR where)
>   void
>   sparc_target::low_arch_setup ()
>   {
> -  current_process ()->tdesc = tdesc_sparc64;
> +  current_thread->tdesc = tdesc_sparc64;
>   }
>   
>   static struct regset_info sparc_regsets[] = {
> diff --git a/gdbserver/linux-tic6x-low.cc b/gdbserver/linux-tic6x-low.cc
> index 893b5f795ac1..02032112a1be 100644
> --- a/gdbserver/linux-tic6x-low.cc
> +++ b/gdbserver/linux-tic6x-low.cc
> @@ -389,7 +389,7 @@ tic6x_target::low_arch_setup ()
>       }
>     tic6x_usrregs_info.regmap = tic6x_regmap;
>   
> -  current_process ()->tdesc = tic6x_read_description (feature);
> +  current_thread->tdesc = tic6x_read_description (feature);
>   }
>   
>   static struct regsets_info tic6x_regsets_info =
> diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
> index d2b55f6f0d2b..14c42603a67e 100644
> --- a/gdbserver/linux-x86-low.cc
> +++ b/gdbserver/linux-x86-low.cc
> @@ -1091,7 +1091,7 @@ x86_target::get_regs_info ()
>   void
>   x86_target::low_arch_setup ()
>   {
> -  current_process ()->tdesc = x86_linux_read_description ();
> +  current_thread->tdesc = x86_linux_read_description ();
>   }
>   
>   bool
> diff --git a/gdbserver/linux-xtensa-low.cc b/gdbserver/linux-xtensa-low.cc
> index 8e7c4c08b7bd..7035edff7bd2 100644
> --- a/gdbserver/linux-xtensa-low.cc
> +++ b/gdbserver/linux-xtensa-low.cc
> @@ -311,7 +311,7 @@ static struct regs_info myregs_info =
>   void
>   xtensa_target::low_arch_setup ()
>   {
> -  current_process ()->tdesc = tdesc_xtensa;
> +  current_thread->tdesc = tdesc_xtensa;
>   }
>   
>   const regs_info *
> diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
> index f8447b0d1eec..211a0794b7fc 100644
> --- a/gdbserver/netbsd-aarch64-low.cc
> +++ b/gdbserver/netbsd-aarch64-low.cc
> @@ -101,7 +101,7 @@ netbsd_aarch64_target::low_arch_setup ()
>     static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>     init_target_desc (tdesc, expedite_regs_aarch64);
>   
> -  current_process ()->tdesc = tdesc;
> +  current_thread->tdesc = tdesc;
>   }
>   
>   /* The singleton target ops object.  */
> diff --git a/gdbserver/netbsd-amd64-low.cc b/gdbserver/netbsd-amd64-low.cc
> index d08e3489f580..024401a6418c 100644
> --- a/gdbserver/netbsd-amd64-low.cc
> +++ b/gdbserver/netbsd-amd64-low.cc
> @@ -194,7 +194,7 @@ netbsd_amd64_target::low_arch_setup ()
>   
>     init_target_desc (tdesc, amd64_expedite_regs);
>   
> -  current_process ()->tdesc = tdesc;
> +  current_thread->tdesc = tdesc;
>   }
>   
>   /* The singleton target ops object.  */
> diff --git a/gdbserver/netbsd-i386-low.cc b/gdbserver/netbsd-i386-low.cc
> index f6fa40c144e3..eec149aa9f99 100644
> --- a/gdbserver/netbsd-i386-low.cc
> +++ b/gdbserver/netbsd-i386-low.cc
> @@ -145,7 +145,7 @@ netbsd_i386_target::low_arch_setup ()
>   
>     init_target_desc (tdesc, i386_expedite_regs);
>   
> -  current_process ()->tdesc = tdesc;
> +  current_thread->tdesc = tdesc;
>   }
>   
>   /* The singleton target ops object.  */
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index ebaeb5e86895..9e8572fbee50 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -39,11 +39,9 @@ get_thread_regcache (struct thread_info *thread, int fetch)
>        have.  */
>     if (regcache == NULL)
>       {
> -      struct process_info *proc = get_thread_process (thread);
> +      gdb_assert (thread->tdesc != NULL);
>   
> -      gdb_assert (proc->tdesc != NULL);
> -
> -      regcache = new_register_cache (proc->tdesc);
> +      regcache = new_register_cache (thread->tdesc);
>         set_thread_regcache_data (thread, regcache);
>       }
>   
> diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
> index 5693cc6626fb..17666c33661f 100644
> --- a/gdbserver/tdesc.cc
> +++ b/gdbserver/tdesc.cc
> @@ -129,7 +129,7 @@ current_target_desc (void)
>     if (current_thread == NULL)
>       return &default_description;
>   
> -  return current_process ()->tdesc;
> +  return current_thread->tdesc;
>   }
>   
>   /* An empty structure.  */


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/8] gdbserver: Add asserts in register_size and register_data functions
  2022-09-20  7:36   ` Luis Machado
@ 2022-11-26  1:36     ` Thiago Jung Bauermann
  0 siblings, 0 replies; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-11-26  1:36 UTC (permalink / raw)
  To: Luis Machado; +Cc: gdb-patches


Hello Luis,

Sorry to take this long to respond. I'm posting v2 to address your
feedback. Thank you very much for reviewing these patches!

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

> On 9/8/22 07:41, 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 27491efc52d5..ebaeb5e86895 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. Someone else must approve, but I suppose it could go in on its own.

Thanks! This patch is unchanged in v2.

-- 
Thiago

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/8] gdbserver: Add thread parameter to linux_get_auxv and linux_get_hwcap
  2022-09-20  7:43   ` Luis Machado
@ 2022-11-26  1:37     ` Thiago Jung Bauermann
  0 siblings, 0 replies; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-11-26  1:37 UTC (permalink / raw)
  To: Luis Machado; +Cc: gdb-patches


Hello Luis,

Thank you for your review.

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

> On 9/8/22 07:41, 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
>> thread_info parameter seems more correct than setting current_thread in one
>> particular code path.
>> Changes are propagated to allow passing the thread parameter through the
>> call chain.
>
> I suppose the effects will only be known when later patches are
> applied. But the auxv data is per-process, right?

AFAIK yes.

> It sounds a bit strange to pass a thread for the hwcap/auxv lookup. Is
> there some other reason to do that?

Good point. I did it this way without thinking about it because the
read_auxv methods use current_thread. I changed the patch to add an
“int pid” parameter instead and it worked fine. I still get the PID
from current_thread when it's not available from the calling function
though.

-- 
Thiago

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 3/8] gdb, gdbserver/aarch64-linux: Allow aarch64_sve_get_vq to return error
  2022-09-20  8:07   ` Luis Machado
@ 2022-11-26  1:42     ` Thiago Jung Bauermann
  0 siblings, 0 replies; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-11-26  1:42 UTC (permalink / raw)
  To: Luis Machado; +Cc: gdb-patches


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

> On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote:
>> If ptrace fails, aarch64_sve_get_vq assumes that SVE isn't supported.
>> Because in a subsequent change this function will need to be called from
>> low_new_thread (which can be called whe the inferior thread isn't stopped),
>> it will need to distinguish between ptrace errors due to SVE not being
>> supported from ptrace errors due to the inferior thread not being stopped.
>
> My understanding is that we will always be able to validate supported
> features when we start/attach to a process. So if, say, we detect SVE
> support through HWCAP's in the beginning of the debugging session,
> that will still hold for any thread that is spawned. What may change
> later on is the VL (still, very unlikely).

I ended up dropping this patch when I implemented your idea of leaving
the process' target description in place and using a thread target
description only for the cases where it is needed (i.e., when the
inferior supports SVE). It simplified the code a lot (thanks!), and it's
not necessary any more to change low_new_thread so this patch became
unnecessary.

As an aside, we don't check for SVE support using HWCAP, just by getting
the NT_ARM_SVE regset via ptrace.

> When a thread is spawned, it will inherit SVE settings from its
> parent. From the Linux Kernel SVE documentation:
>
>   In particular, on return from a fork() or clone(), the parent and new child
>   process or thread share identical SVE configuration, matching that of the
>   parent before the call.
>
> Is that something we can use here? I suppose there may be corner cases
> where the parent spawned a thread and changed its VL size etc.

It is, and for a while I had changes in my local branch which in the
event of a clone or fork event copied the original thread's target
description to the new one.

After a while I noticed that in practice it was redundant, because when
the new thread stops gdbserver calls aarch64_target::arch_update_tdesc
which also sets up the thread's target description so I removed the
changes for the clone and fork events.

> If we can't and a thread is running, we won't be able to tell the VL,
> but we have a state that is meant to indicate "unknown VL", don't we?
> Maybe I'm misremembering, but -1 used to indicate that.

We used -1 in struct gdbarch_info's id field, but it became unused in
commit “Fix thread's gdbarch when SVE vector length changes” and was
removed. We don't have an “unknown VL” state anymore. And with v2 of
these patches, we can always use ptrace to get the VL so it's not
needed.

-- 
Thiago

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 4/8] gdbserver/linux-aarch64: Factor out function to get aarch64_features
  2022-09-20  8:08   ` Luis Machado
@ 2022-11-26  1:44     ` Thiago Jung Bauermann
  0 siblings, 0 replies; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-11-26  1:44 UTC (permalink / raw)
  To: Luis Machado; +Cc: gdb-patches


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

> On 9/8/22 07:41, 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 | 45 ++++++++++++++++++++++------------
>>   1 file changed, 29 insertions(+), 16 deletions(-)
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index 576925838f49..9b57be73818e 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -652,6 +652,28 @@ 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 gdb::optional<struct aarch64_features>
>> +aarch64_get_arch_features (const thread_info *thread)
>> +{
>> +  struct aarch64_features features;
>> +  int vq = aarch64_sve_get_vq (thread->id.lwp ());
>> +
>> +  if (vq < 0)
>> +    return {};
>
> If we can't identify SVE properly, should we carry on and disable SVE
> support but enable all the other features we can find?

Yes, that's what happens in v2 now. aarch64_sve_get_vq isn't changed by
the patch series anymore so it continues to return vq = 0 if ptrace
fails. Because of that, in v2 this function now returns an
aarch64_features struct directly.

>> +
>> +  features.vq = vq;
>> +  /* A-profile PAC is 64-bit only.  */
>> +  features.pauth = linux_get_hwcap (thread, 8) & AARCH64_HWCAP_PACA;
>> +  /* A-profile MTE is 64-bit only.  */
>> +  features.mte = linux_get_hwcap2 (thread, 8) & HWCAP2_MTE;
>> +  features.tls = true;
>> +
>> +  return features;
>> +}

-- 
Thiago

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/8] gdbserver: Move target description from being per-process to being per-thread
  2022-09-20 11:21   ` Luis Machado
@ 2022-11-26  1:47     ` Thiago Jung Bauermann
  0 siblings, 0 replies; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-11-26  1:47 UTC (permalink / raw)
  To: Luis Machado; +Cc: gdb-patches


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

> On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote:
>> --- 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 keep a process-wide target description in case we fail to
> detect the target description for a given thread?

That is a great idea. I implemented it in v2, with the difference that
we never fail to detect the target description for a thread. What can
happen is that we fail to detect SVE, and in that case it is assumed to
be unsupported.

The thread-specific target description is used only for inferiors where
we detect SVE, and the process-wide one used for all the others.

> Also, it may simplify some of the changes in this patch.

Indeed, it simplified many of the changes in this patch and the others.
Thank you for the suggestion!

>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -191,9 +191,26 @@ struct arch_process_info
>>   static int
>>   is_64bit_tdesc (void)
>>   {
>> +  const target_desc *tdesc;
>> +
>> +  if (current_thread && current_thread->tdesc)
>> +    tdesc = current_thread->tdesc;
>> +  else
>> +    {
>> +      /* Find some thread that has a target description.  */
>> +      const struct thread_info *thread
>> +	  = find_thread (current_process ()->pid, [] (thread_info *thr) {
>> +	      return thr->tdesc != nullptr;
>> +	    });
>> +
>> +      gdb_assert (thread != nullptr);
>> +
>> +      tdesc = thread->tdesc;
>> +    }
>> +
>>     /* We may not have a current thread at this point, so go straight to
>>        the process's target description.  */
>> -  return register_size (current_process ()->tdesc, 0) == 8;
>> +  return register_size (tdesc, 0) == 8;
>>   }
>
> Keeping the process-wide target description would simplify this
> function in my opinion. We won't have 64-bit processes spawning 32-bit
> threads.

You are right. This patch series doesn't change this function anymore.

>> @@ -678,6 +695,31 @@ void
>>   aarch64_target::low_new_thread (lwp_info *lwp)
>>   {
>>     aarch64_linux_new_thread (lwp);
>> +
>> +  ptid_t ptid = ptid_of_lwp (lwp);
>> +  process_info *proc = find_process_pid (ptid.pid ());
>> +
>> +  /* If the inferior is still going through the shell or the thread isn't
>> +     stopped we can't read its registers in order to read the target
>> +     description.  */
>> +  if (proc->starting_up)
>> +    return;
>> +
>> +  thread_info *thread = get_lwp_thread (lwp);
>> +  unsigned int machine;
>> +  gdb_assert (ptid.lwp_p ());
>> +  bool is_elf64 = linux_pid_exe_is_elf_64_file (ptid.lwp (), &machine);
>> +
>> +  if (is_elf64)
>> +    {
>> +      gdb::optional<struct aarch64_features> features
>> +	  = aarch64_get_arch_features (thread);
>> +
>> +      if (features.has_value())
>> +	thread->tdesc = aarch64_linux_read_description (*features);
>> +    }
>> +  else
>> +    thread->tdesc = aarch32_linux_read_description ();
>>   }
>>     void
>> @@ -848,14 +890,14 @@ aarch64_target::low_arch_setup ()
>>   	 succeeds so all we can do here is assert that features is valid.  */
>>         gdb_assert (features.has_value ());
>>   -      current_process ()->tdesc = aarch64_linux_read_description (*features);
>> +      current_thread->tdesc = aarch64_linux_read_description (*features);
>>           /* Adjust the register sets we should use for this particular set of
>>   	 features.  */
>>         aarch64_adjust_register_sets (*features);
>>       }
>>     else
>> -    current_process ()->tdesc = aarch32_linux_read_description ();
>> +    current_thread->tdesc = aarch32_linux_read_description ();
>
> Given 32-bit Arm processes will have the same target description
> throughout their lives, would it make sense to have a single target
> description stored process-wide, and whenever we need the description
> we can find it there?

Yes, you are right. In v2 low_arch_setup keeps 32-bit Arm the same as
before, with only process-wide target description. AArch64 inferiors
that don't support SVE also get only a process-wide target description.

> Maybe code it as:
>
> return current_thread->tdesc == nullptr? current_process ()->tdesc :
> current_thread->tdesc;

I did that. It turns out that this logic is only needed in two places:
regcache.cc::get_thread_regcache and tdesc.cc::current_target_desc.

> This would make the changes to all the other targets unnecessary, as
> those would be able to lookup their target descriptions from the
> process instead of the threads.

Indeed!

-- 
Thiago

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 6/8] gdbserver/linux-aarch64: When inferior stops, update its target description
  2022-09-20  8:48   ` Luis Machado
@ 2022-11-26  1:49     ` Thiago Jung Bauermann
  0 siblings, 0 replies; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-11-26  1:49 UTC (permalink / raw)
  To: Luis Machado; +Cc: gdb-patches


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

> On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote:
>> Introduce new arch_update_tdesc method to the linux_process_target class.
>> This allows aarch64-linux to probe the inferior's vq register and provide
>> an updated target description reflecting the new vector length.
>> ---
>>   gdbserver/linux-aarch64-low.cc | 21 +++++++++++++++++++++
>>   gdbserver/linux-low.cc         | 18 ++++++++++++++++++
>>   gdbserver/linux-low.h          |  5 +++++
>>   3 files changed, 44 insertions(+)
>
> LGTM for the aarch64 parts. Someone else must approve the generic changes.

Thanks! Since in v2 patch 5 became much smaller, I squashed it into this
one. The changes are described in the per-patch changes list in the
cover letter.

-- 
Thiago

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 7/8] gdb/aarch64: Factor out most of the thread_architecture method
  2022-09-20  8:52   ` Luis Machado
@ 2022-11-26  1:49     ` Thiago Jung Bauermann
  0 siblings, 0 replies; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-11-26  1:49 UTC (permalink / raw)
  To: Luis Machado; +Cc: gdb-patches


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

> On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote:
>> +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_create_target_description (features);

Oops, I hadn't noticed that this patch had a bug (memory leak,
actually): it inadvertently changed the moved code to call
aarch64_create_target_description instead of aarch64_read_description.

In v2 I fixed this, and double-checked that there's no actual change in
the moved code.

>> +  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 d8513023c376..3d78515e167f 100644
>> --- a/gdb/aarch64-tdep.h
>> +++ b/gdb/aarch64-tdep.h
>> @@ -139,4 +139,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

Thank you for your review!

-- 
Thiago

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 8/8] gdb/aarch64: When remote debugging detect changes in the target description
  2022-09-20  8:59   ` Luis Machado
@ 2022-11-26  1:50     ` Thiago Jung Bauermann
  0 siblings, 0 replies; 25+ messages in thread
From: Thiago Jung Bauermann @ 2022-11-26  1:50 UTC (permalink / raw)
  To: Luis Machado; +Cc: gdb-patches


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

> On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote:
>> If the remote target provides an expedited VG register, use it to update
>> its target description.  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        | 28 +++++++++++++++++++++++++-
>>   gdb/arch-utils.c          |  9 +++++++++
>>   gdb/arch-utils.h          |  5 +++++
>>   gdb/gdbarch-components.py | 16 +++++++++++++++
>>   gdb/gdbarch-gen.h         | 11 ++++++++++
>>   gdb/gdbarch.c             | 23 +++++++++++++++++++++
>>   gdb/remote.c              | 42 +++++++++++++++++++++++++++++++++++++++
>>   7 files changed, 133 insertions(+), 1 deletion(-)
>
> LGTM for the aarch64 code. Someone else must approve the generic parts.

Thank you again for your review. For v2 I changed the patch title and
description a little bit, but the code is the same.

-- 
Thiago

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2022-11-26  1:50 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-08  6:41 [PATCH 0/8] gdbserver improvements for AArch64 SVE support Thiago Jung Bauermann
2022-09-08  6:41 ` [PATCH 1/8] gdbserver: Add asserts in register_size and register_data functions Thiago Jung Bauermann
2022-09-20  7:36   ` Luis Machado
2022-11-26  1:36     ` Thiago Jung Bauermann
2022-09-08  6:41 ` [PATCH 2/8] gdbserver: Add thread parameter to linux_get_auxv and linux_get_hwcap Thiago Jung Bauermann
2022-09-20  7:43   ` Luis Machado
2022-11-26  1:37     ` Thiago Jung Bauermann
2022-09-08  6:41 ` [PATCH 3/8] gdb, gdbserver/aarch64-linux: Allow aarch64_sve_get_vq to return error Thiago Jung Bauermann
2022-09-20  8:07   ` Luis Machado
2022-11-26  1:42     ` Thiago Jung Bauermann
2022-09-08  6:41 ` [PATCH 4/8] gdbserver/linux-aarch64: Factor out function to get aarch64_features Thiago Jung Bauermann
2022-09-20  8:08   ` Luis Machado
2022-11-26  1:44     ` Thiago Jung Bauermann
2022-09-08  6:41 ` [PATCH 5/8] gdbserver: Move target description from being per-process to being per-thread Thiago Jung Bauermann
2022-09-20 11:21   ` Luis Machado
2022-11-26  1:47     ` Thiago Jung Bauermann
2022-09-08  6:41 ` [PATCH 6/8] gdbserver/linux-aarch64: When inferior stops, update its target description Thiago Jung Bauermann
2022-09-20  8:48   ` Luis Machado
2022-11-26  1:49     ` Thiago Jung Bauermann
2022-09-08  6:41 ` [PATCH 7/8] gdb/aarch64: Factor out most of the thread_architecture method Thiago Jung Bauermann
2022-09-20  8:52   ` Luis Machado
2022-11-26  1:49     ` Thiago Jung Bauermann
2022-09-08  6:41 ` [PATCH 8/8] gdb/aarch64: When remote debugging detect changes in the target description Thiago Jung Bauermann
2022-09-20  8:59   ` Luis Machado
2022-11-26  1:50     ` Thiago Jung Bauermann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).