public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/5] Improve support for regcache_map_entry with variable register base
@ 2022-07-08  0:58 John Baldwin
  2022-07-08  0:58 ` [PATCH 1/5] regcache: Add collect/supply_regset variants that accept a " John Baldwin
                   ` (5 more replies)
  0 siblings, 6 replies; 20+ messages in thread
From: John Baldwin @ 2022-07-08  0:58 UTC (permalink / raw)
  To: gdb-patches

When I added support for TLS to the ARM and AArch64 architectures on
FreeBSD, I had to construct register maps and associated register sets
on the fly in both the callback for processing register core dump
notes and in the native target routines to fetch and store registers.

The reason I had to create these on the fly is that the register
number was not known at compile time, so the register map and register
set could not be constant structures shared among the -tdep and -nat
code the way other register sets were.

This series aims to rectify that.  The first patch adds new variants
of regcache::collect_regset and regcache::supply_regset that accept an
additional 'regbase' argument.  When these functions iterate over
regcache map entries, the effective register number for the entry is
computed by adding the value in the entry to 'regbase' permittting the
regcache map entries to hold relative register numbers for a block of
registers.

The rest of the series then makes use of this to use a single, shared
register map and register set for TLS on FreeBSD/ARM and
FreeBSD/AArch64.  Using this requires custom supply and collect regset
routines for the TLS regsets that extract the base register number
from the gdbarch's tdep and invoke the new regcache class methods, so
patch 2 updates the wrappers for dealing with regsets in fbsd-nat to
always invoke the regset routines from the regset instead of directly
calling the default functions.  Patch 3 is another change to fbsd-nat
to cope with the fact that regcache_map_supplies() needs to pass in
the relative register number in the wrapper routines rather than the
absolute register number.

Patches 4 and 5 are the updates to the ARM and AArch64 FreeBSD
architectures and targets.

I have not done the work to make use of this in the Linux AArch64
architecture, though I think it would apply to at least the TLS and
MTE register sets there in a similar fashion.  SVE is harder because
the register sizes change (though SVE might be able to make use of
this if register_size() returns the right value by using a register
size of 0 in the relevant register cache map entries).

John Baldwin (5):
  regcache: Add collect/supply_regset variants that accept a register
    base.
  fbsd-nat: Use regset supply/collect methods.
  fbsd-nat: Pass an optional register base to the register set helpers.
  arm-fbsd: Use a static regset for the TLS register set.
  aarch64-fbsd: Use a static regset for the TLS register set.

 gdb/aarch64-fbsd-nat.c  | 36 +++------------------------
 gdb/aarch64-fbsd-tdep.c | 54 ++++++++++++++++++++++++++++-------------
 gdb/aarch64-fbsd-tdep.h |  1 +
 gdb/arm-fbsd-nat.c      | 34 +++-----------------------
 gdb/arm-fbsd-tdep.c     | 52 +++++++++++++++++++++++++++------------
 gdb/arm-fbsd-tdep.h     |  1 +
 gdb/fbsd-nat.c          | 42 +++++++++++++++++---------------
 gdb/fbsd-nat.h          | 49 +++++++++++++++++++++----------------
 gdb/regcache.c          | 28 ++++++++++++++++++---
 gdb/regcache.h          | 24 ++++++++++++++++--
 10 files changed, 181 insertions(+), 140 deletions(-)

-- 
2.36.1


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

* [PATCH 1/5] regcache: Add collect/supply_regset variants that accept a register base.
  2022-07-08  0:58 [PATCH 0/5] Improve support for regcache_map_entry with variable register base John Baldwin
@ 2022-07-08  0:58 ` John Baldwin
  2022-11-22 19:56   ` Simon Marchi
  2022-07-08  0:58 ` [PATCH 2/5] fbsd-nat: Use regset supply/collect methods John Baldwin
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 20+ messages in thread
From: John Baldwin @ 2022-07-08  0:58 UTC (permalink / raw)
  To: gdb-patches

Some register sets described by an array of regcache_map_entry
structures do not have fixed register numbers in their associated
architecture but do describe a block of registers whose numbers are at
fixed offsets relative to some base register value.  An example of
this are the TLS register sets for the ARM and AArch64 architectures.

Currently OS-specific architectures create register maps and register
sets dynamically using the register base number.  However, this
requires duplicating the code to create the register map and register
set.  To reduce duplication, add variants of the collect_regset and
supply_regset regcache methods which accept a base register number.
For valid register map entries (i.e. not REGCACHE_MAP_SKIP), add this
base register number to the value from the map entry to determine the
final register number.
---
 gdb/regcache.c | 28 +++++++++++++++++++++++++---
 gdb/regcache.h | 24 ++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/gdb/regcache.c b/gdb/regcache.c
index 037659ef8fa..1db3d972ef8 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1180,7 +1180,7 @@ regcache::transfer_regset_register (struct regcache *out_regcache, int regnum,
 /* See regcache.h.  */
 
 void
-regcache::transfer_regset (const struct regset *regset,
+regcache::transfer_regset (const struct regset *regset, int regbase,
 			   struct regcache *out_regcache,
 			   int regnum, const gdb_byte *in_buf,
 			   gdb_byte *out_buf, size_t size) const
@@ -1195,6 +1195,9 @@ regcache::transfer_regset (const struct regset *regset,
       int regno = map->regno;
       int slot_size = map->size;
 
+      if (regno != REGCACHE_MAP_SKIP)
+	regno += regbase;
+
       if (slot_size == 0 && regno != REGCACHE_MAP_SKIP)
 	slot_size = m_descr->sizeof_register[regno];
 
@@ -1242,7 +1245,18 @@ void
 regcache::supply_regset (const struct regset *regset,
 			 int regnum, const void *buf, size_t size)
 {
-  transfer_regset (regset, this, regnum, (const gdb_byte *) buf, nullptr, size);
+  transfer_regset (regset, 0, this, regnum, (const gdb_byte *) buf, nullptr,
+		   size);
+}
+
+/* See regcache.h.  */
+
+void
+regcache::supply_regset (const struct regset *regset, int regbase,
+			 int regnum, const void *buf, size_t size)
+{
+  transfer_regset (regset, regbase, this, regnum, (const gdb_byte *) buf,
+		   nullptr, size);
 }
 
 /* Collect register REGNUM from REGCACHE to BUF, using the register
@@ -1261,11 +1275,19 @@ void
 regcache::collect_regset (const struct regset *regset,
 			 int regnum, void *buf, size_t size) const
 {
-  transfer_regset (regset, nullptr, regnum, nullptr, (gdb_byte *) buf, size);
+  transfer_regset (regset, 0, nullptr, regnum, nullptr, (gdb_byte *) buf, size);
 }
 
 /* See regcache.h  */
 
+void
+regcache::collect_regset (const struct regset *regset, int regbase,
+			 int regnum, void *buf, size_t size) const
+{
+  transfer_regset (regset, regbase, nullptr, regnum, nullptr, (gdb_byte *) buf,
+		   size);
+}
+
 bool
 regcache_map_supplies (const struct regcache_map_entry *map, int regnum,
 		       struct gdbarch *gdbarch, size_t size)
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 1dbba5ce9af..f01127b20fb 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -150,6 +150,14 @@ extern void regcache_collect_regset (const struct regset *regset,
 				     int regnum, void *buf, size_t size);
 
 
+extern void regcache_supply_regset (const struct regset *regset, int regbase,
+				    struct regcache *regcache,
+				    int regnum, const void *buf,
+				    size_t size);
+extern void regcache_collect_regset (const struct regset *regset, int regbase,
+				     const struct regcache *regcache,
+				     int regnum, void *buf, size_t size);
+
 /* Return true if a set of registers contains the value of the
    register numbered REGNUM.  The size of the set of registers is
    given in SIZE, and the layout of the set of registers is described
@@ -375,10 +383,22 @@ class regcache : public detached_regcache
   void cooked_write_part (int regnum, int offset, int len,
 			  const gdb_byte *buf);
 
+  /* Transfer a set of registers (as described by REGSET) between
+     REGCACHE and BUF.  If REGNUM == -1, transfer all registers
+     belonging to the regset, otherwise just the register numbered
+     REGNUM.  The REGSET's 'regmap' field must point to an array of
+     'struct regcache_map_entry'.  The valid register numbers in each
+     entry in 'struct regcache_map_entry' are offset by REGBASE.  */
+
+  void supply_regset (const struct regset *regset, int regbase,
+		      int regnum, const void *buf, size_t size);
+
+  void collect_regset (const struct regset *regset, int regbase, int regnum,
+		       void *buf, size_t size) const;
+
   void supply_regset (const struct regset *regset,
 		      int regnum, const void *buf, size_t size);
 
-
   void collect_regset (const struct regset *regset, int regnum,
 		       void *buf, size_t size) const;
 
@@ -419,7 +439,7 @@ class regcache : public detached_regcache
   /* Transfer a single or all registers belonging to a certain register
      set to or from a buffer.  This is the main worker function for
      regcache_supply_regset and regcache_collect_regset.  */
-  void transfer_regset (const struct regset *regset,
+  void transfer_regset (const struct regset *regset, int regbase,
 			struct regcache *out_regcache,
 			int regnum, const gdb_byte *in_buf,
 			gdb_byte *out_buf, size_t size) const;
-- 
2.36.1


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

* [PATCH 2/5] fbsd-nat: Use regset supply/collect methods.
  2022-07-08  0:58 [PATCH 0/5] Improve support for regcache_map_entry with variable register base John Baldwin
  2022-07-08  0:58 ` [PATCH 1/5] regcache: Add collect/supply_regset variants that accept a " John Baldwin
@ 2022-07-08  0:58 ` John Baldwin
  2022-11-22 20:20   ` Simon Marchi
  2022-07-08  0:58 ` [PATCH 3/5] fbsd-nat: Pass an optional register base to the register set helpers John Baldwin
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 20+ messages in thread
From: John Baldwin @ 2022-07-08  0:58 UTC (permalink / raw)
  To: gdb-patches

fbsd-nat includes various helper routines for fetching and storing
register sets via ptrace where the register set is described by a
regset.  These helper routines directly invoke the
supply/collect_regset regcache methods which doesn't permit a regset
to provide custom logic when fetching or storing a register set.
Instead, just use the function pointers from the struct regset
directly.
---
 gdb/fbsd-nat.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 281b034b115..9d7383ac0ab 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -1744,7 +1744,7 @@ fbsd_nat_target::fetch_register_set (struct regcache *regcache, int regnum,
       if (ptrace (fetch_op, pid, (PTRACE_TYPE_ARG3) regs, 0) == -1)
 	perror_with_name (_("Couldn't get registers"));
 
-      regcache->supply_regset (regset, regnum, regs, size);
+      regset->supply_regset (regset, regcache, regnum, regs, size);
       return true;
     }
   return false;
@@ -1768,7 +1768,7 @@ fbsd_nat_target::store_register_set (struct regcache *regcache, int regnum,
       if (ptrace (fetch_op, pid, (PTRACE_TYPE_ARG3) regs, 0) == -1)
 	perror_with_name (_("Couldn't get registers"));
 
-      regcache->collect_regset (regset, regnum, regs, size);
+      regset->collect_regset (regset, regcache, regnum, regs, size);
 
       if (ptrace (store_op, pid, (PTRACE_TYPE_ARG3) regs, 0) == -1)
 	perror_with_name (_("Couldn't write registers"));
@@ -1813,7 +1813,7 @@ fbsd_nat_target::fetch_regset (struct regcache *regcache, int regnum, int note,
       if (ptrace (PT_GETREGSET, pid, (PTRACE_TYPE_ARG3) &iov, note) == -1)
 	perror_with_name (_("Couldn't get registers"));
 
-      regcache->supply_regset (regset, regnum, regs, size);
+      regset->supply_regset (regset, regcache, regnum, regs, size);
       return true;
     }
   return false;
@@ -1838,7 +1838,7 @@ fbsd_nat_target::store_regset (struct regcache *regcache, int regnum, int note,
       if (ptrace (PT_GETREGSET, pid, (PTRACE_TYPE_ARG3) &iov, note) == -1)
 	perror_with_name (_("Couldn't get registers"));
 
-      regcache->collect_regset (regset, regnum, regs, size);
+      regset->collect_regset (regset, regcache, regnum, regs, size);
 
       if (ptrace (PT_SETREGSET, pid, (PTRACE_TYPE_ARG3) &iov, note) == -1)
 	perror_with_name (_("Couldn't write registers"));
-- 
2.36.1


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

* [PATCH 3/5] fbsd-nat: Pass an optional register base to the register set helpers.
  2022-07-08  0:58 [PATCH 0/5] Improve support for regcache_map_entry with variable register base John Baldwin
  2022-07-08  0:58 ` [PATCH 1/5] regcache: Add collect/supply_regset variants that accept a " John Baldwin
  2022-07-08  0:58 ` [PATCH 2/5] fbsd-nat: Use regset supply/collect methods John Baldwin
@ 2022-07-08  0:58 ` John Baldwin
  2022-11-22 20:38   ` Simon Marchi
  2022-07-08  0:58 ` [PATCH 4/5] arm-fbsd: Use a static regset for the TLS register set John Baldwin
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 20+ messages in thread
From: John Baldwin @ 2022-07-08  0:58 UTC (permalink / raw)
  To: gdb-patches

This is needed to permit using the helpers for register sets with a
variable base.  In particular regnum needs to be converted into a
relative register number before passed to regcache_map_supplies.
---
 gdb/fbsd-nat.c | 34 +++++++++++++++++++---------------
 gdb/fbsd-nat.h | 49 ++++++++++++++++++++++++++++---------------------
 2 files changed, 47 insertions(+), 36 deletions(-)

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 9d7383ac0ab..edcd3fc3ed1 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -1732,14 +1732,15 @@ fbsd_nat_target::supports_disable_randomization ()
 bool
 fbsd_nat_target::fetch_register_set (struct regcache *regcache, int regnum,
 				     int fetch_op, const struct regset *regset,
-				     void *regs, size_t size)
+				     int regbase, void *regs, size_t size)
 {
   const struct regcache_map_entry *map
     = (const struct regcache_map_entry *) regset->regmap;
   pid_t pid = get_ptrace_pid (regcache->ptid ());
 
-  if (regnum == -1 || regcache_map_supplies (map, regnum, regcache->arch(),
-					     size))
+  if (regnum == -1
+      || (regnum >= regbase && regcache_map_supplies (map, regnum - regbase,
+						      regcache->arch(), size)))
     {
       if (ptrace (fetch_op, pid, (PTRACE_TYPE_ARG3) regs, 0) == -1)
 	perror_with_name (_("Couldn't get registers"));
@@ -1755,15 +1756,16 @@ fbsd_nat_target::fetch_register_set (struct regcache *regcache, int regnum,
 bool
 fbsd_nat_target::store_register_set (struct regcache *regcache, int regnum,
 				     int fetch_op, int store_op,
-				     const struct regset *regset, void *regs,
-				     size_t size)
+				     const struct regset *regset, int regbase,
+				     void *regs, size_t size)
 {
   const struct regcache_map_entry *map
     = (const struct regcache_map_entry *) regset->regmap;
   pid_t pid = get_ptrace_pid (regcache->ptid ());
 
-  if (regnum == -1 || regcache_map_supplies (map, regnum, regcache->arch(),
-					     size))
+  if (regnum == -1
+      || (regnum >= regbase && regcache_map_supplies (map, regnum - regbase,
+						      regcache->arch(), size)))
     {
       if (ptrace (fetch_op, pid, (PTRACE_TYPE_ARG3) regs, 0) == -1)
 	perror_with_name (_("Couldn't get registers"));
@@ -1796,15 +1798,16 @@ fbsd_nat_target::have_regset (ptid_t ptid, int note)
 
 bool
 fbsd_nat_target::fetch_regset (struct regcache *regcache, int regnum, int note,
-			       const struct regset *regset, void *regs,
-			       size_t size)
+			       const struct regset *regset, int regbase,
+			       void *regs, size_t size)
 {
   const struct regcache_map_entry *map
     = (const struct regcache_map_entry *) regset->regmap;
   pid_t pid = get_ptrace_pid (regcache->ptid ());
 
-  if (regnum == -1 || regcache_map_supplies (map, regnum, regcache->arch(),
-					     size))
+  if (regnum == -1
+      || (regnum >= regbase && regcache_map_supplies (map, regnum - regbase,
+						      regcache->arch(), size)))
     {
       struct iovec iov;
 
@@ -1821,15 +1824,16 @@ fbsd_nat_target::fetch_regset (struct regcache *regcache, int regnum, int note,
 
 bool
 fbsd_nat_target::store_regset (struct regcache *regcache, int regnum, int note,
-			       const struct regset *regset, void *regs,
-			       size_t size)
+			       const struct regset *regset, int regbase,
+			       void *regs, size_t size)
 {
   const struct regcache_map_entry *map
     = (const struct regcache_map_entry *) regset->regmap;
   pid_t pid = get_ptrace_pid (regcache->ptid ());
 
-  if (regnum == -1 || regcache_map_supplies (map, regnum, regcache->arch(),
-					     size))
+  if (regnum == -1
+      || (regnum >= regbase && regcache_map_supplies (map, regnum - regbase,
+						      regcache->arch(), size)))
     {
       struct iovec iov;
 
diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
index 3a13bc8711f..22e47dcca0d 100644
--- a/gdb/fbsd-nat.h
+++ b/gdb/fbsd-nat.h
@@ -133,7 +133,8 @@ class fbsd_nat_target : public inf_ptrace_target
   /* Helper routines for use in fetch_registers and store_registers in
      subclasses.  These routines fetch and store a single set of
      registers described by REGSET.  The REGSET's 'regmap' field must
-     point to an array of 'struct regcache_map_entry'.
+     point to an array of 'struct regcache_map_entry'.  The valid
+     register numbers in the register map are relative to REGBASE.
 
      FETCH_OP is a ptrace operation to fetch the set of registers from
      a native thread.  STORE_OP is a ptrace operation to store the set
@@ -143,24 +144,27 @@ class fbsd_nat_target : public inf_ptrace_target
      and SIZE is the size of the storage.
 
      Returns true if the register set was transferred due to a
-     matching REGNUM.*/
+     matching REGNUM.  */
 
   bool fetch_register_set (struct regcache *regcache, int regnum, int fetch_op,
-			   const struct regset *regset, void *regs, size_t size);
+			   const struct regset *regset, int regbase, void *regs,
+			   size_t size);
 
   bool store_register_set (struct regcache *regcache, int regnum, int fetch_op,
 			   int store_op, const struct regset *regset,
-			   void *regs, size_t size);
+			   int regbase, void *regs, size_t size);
 
   /* Helper routines which use PT_GETREGSET and PT_SETREGSET for the
      specified NOTE instead of regset-specific fetch and store
      ops.  */
 
   bool fetch_regset (struct regcache *regcache, int regnum, int note,
-		     const struct regset *regset, void *regs, size_t size);
+		     const struct regset *regset, int regbase, void *regs,
+		     size_t size);
 
   bool store_regset (struct regcache *regcache, int regnum, int note,
-		     const struct regset *regset, void *regs, size_t size);
+		     const struct regset *regset, int regbase, void *regs,
+		     size_t size);
 
 protected:
   /* Wrapper versions of the above helpers which accept a register set
@@ -168,22 +172,23 @@ class fbsd_nat_target : public inf_ptrace_target
 
   template <class Regset>
   bool fetch_register_set (struct regcache *regcache, int regnum, int fetch_op,
-			   const struct regset *regset)
+			   const struct regset *regset, int regbase = 0)
   {
     Regset regs;
-    return fetch_register_set (regcache, regnum, fetch_op, regset, &regs,
-			       sizeof (regs));
-  }
-
-  template <class Regset>
-  bool store_register_set (struct regcache *regcache, int regnum, int fetch_op,
-			   int store_op, const struct regset *regset)
-  {
-    Regset regs;
-    return store_register_set (regcache, regnum, fetch_op, store_op, regset,
+    return fetch_register_set (regcache, regnum, fetch_op, regset, regbase,
 			       &regs, sizeof (regs));
   }
 
+  template <class Regset>
+  bool store_register_set (struct regcache *regcache, int regnum, int fetch_op,
+			   int store_op, const struct regset *regset,
+			   int regbase = 0)
+  {
+    Regset regs;
+    return store_register_set (regcache, regnum, fetch_op, store_op, regset,
+			       regbase, &regs, sizeof (regs));
+  }
+
   /* Helper routine for use in read_description in subclasses.  This
      routine checks if the register set for the specified NOTE is
      present for a given PTID.  If the register set is present, the
@@ -197,18 +202,20 @@ class fbsd_nat_target : public inf_ptrace_target
 
   template <class Regset>
   bool fetch_regset (struct regcache *regcache, int regnum, int note,
-		     const struct regset *regset)
+		     const struct regset *regset, int regbase = 0)
   {
     Regset regs;
-    return fetch_regset (regcache, regnum, note, regset, &regs, sizeof (regs));
+    return fetch_regset (regcache, regnum, note, regset, regbase, &regs,
+			 sizeof (regs));
   }
 
   template <class Regset>
   bool store_regset (struct regcache *regcache, int regnum, int note,
-		     const struct regset *regset)
+		     const struct regset *regset, int regbase = 0)
   {
     Regset regs;
-    return store_regset (regcache, regnum, note, regset, &regs, sizeof (regs));
+    return store_regset (regcache, regnum, note, regset, regbase, &regs,
+			 sizeof (regs));
   }
 };
 
-- 
2.36.1


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

* [PATCH 4/5] arm-fbsd: Use a static regset for the TLS register set.
  2022-07-08  0:58 [PATCH 0/5] Improve support for regcache_map_entry with variable register base John Baldwin
                   ` (2 preceding siblings ...)
  2022-07-08  0:58 ` [PATCH 3/5] fbsd-nat: Pass an optional register base to the register set helpers John Baldwin
@ 2022-07-08  0:58 ` John Baldwin
  2022-11-22 20:39   ` Simon Marchi
  2022-07-08  0:58 ` [PATCH 5/5] aarch64-fbsd: " John Baldwin
  2022-07-21 14:53 ` [PATCH 0/5] Improve support for regcache_map_entry with variable register base John Baldwin
  5 siblings, 1 reply; 20+ messages in thread
From: John Baldwin @ 2022-07-08  0:58 UTC (permalink / raw)
  To: gdb-patches

This uses custom collect/supply regset handlers which pass the TLS
register number from the gdbarch_tdep as the base register number.
---
 gdb/arm-fbsd-nat.c  | 34 ++++-------------------------
 gdb/arm-fbsd-tdep.c | 52 +++++++++++++++++++++++++++++++--------------
 gdb/arm-fbsd-tdep.h |  1 +
 3 files changed, 41 insertions(+), 46 deletions(-)

diff --git a/gdb/arm-fbsd-nat.c b/gdb/arm-fbsd-nat.c
index a306e1e2ee0..ceda6337ab9 100644
--- a/gdb/arm-fbsd-nat.c
+++ b/gdb/arm-fbsd-nat.c
@@ -58,21 +58,8 @@ arm_fbsd_nat_target::fetch_registers (struct regcache *regcache, int regnum)
   arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
 
   if (tdep->tls_regnum > 0)
-    {
-      const struct regcache_map_entry arm_fbsd_tlsregmap[] =
-	{
-	  { 1, tdep->tls_regnum, 4 },
-	  { 0 }
-	};
-
-      const struct regset arm_fbsd_tlsregset =
-	{
-	  arm_fbsd_tlsregmap,
-	  regcache_supply_regset, regcache_collect_regset
-	};
-
-      fetch_regset<uint32_t> (regcache, regnum, NT_ARM_TLS, &arm_fbsd_tlsregset);
-    }
+    fetch_regset<uint32_t> (regcache, regnum, NT_ARM_TLS, &arm_fbsd_tls_regset,
+			    tdep->tls_regnum);
 #endif
 }
 
@@ -93,21 +80,8 @@ arm_fbsd_nat_target::store_registers (struct regcache *regcache, int regnum)
   arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
 
   if (tdep->tls_regnum > 0)
-    {
-      const struct regcache_map_entry arm_fbsd_tlsregmap[] =
-	{
-	  { 1, tdep->tls_regnum, 4 },
-	  { 0 }
-	};
-
-      const struct regset arm_fbsd_tlsregset =
-	{
-	  arm_fbsd_tlsregmap,
-	  regcache_supply_regset, regcache_collect_regset
-	};
-
-      store_regset<uint32_t> (regcache, regnum, NT_ARM_TLS, &arm_fbsd_tlsregset);
-    }
+    store_regset<uint32_t> (regcache, regnum, NT_ARM_TLS, &arm_fbsd_tls_regset,
+			    tdep->tls_regnum);
 #endif
 }
 
diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
index 483820c1092..2a73643b763 100644
--- a/gdb/arm-fbsd-tdep.c
+++ b/gdb/arm-fbsd-tdep.c
@@ -52,6 +52,12 @@ static const struct regcache_map_entry arm_fbsd_vfpregmap[] =
     { 0 }
   };
 
+static const struct regcache_map_entry arm_fbsd_tls_regmap[] =
+  {
+    { 1, 0, 4 },
+    { 0 }
+  };
+
 /* In a signal frame, sp points to a 'struct sigframe' which is
    defined as:
 
@@ -151,6 +157,34 @@ const struct regset arm_fbsd_vfpregset =
     regcache_supply_regset, regcache_collect_regset
   };
 
+static void
+arm_fbsd_supply_tls_regset (const struct regset *regset,
+			    struct regcache *regcache,
+			    int regnum, const void *buf, size_t size)
+{
+  struct gdbarch *gdbarch = regcache->arch ();
+  arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
+
+  regcache->supply_regset (regset, tdep->tls_regnum, regnum, buf, size);
+}
+
+static void
+arm_fbsd_collect_tls_regset (const struct regset *regset,
+			     const struct regcache *regcache,
+			     int regnum, void *buf, size_t size)
+{
+  struct gdbarch *gdbarch = regcache->arch ();
+  arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
+
+  regcache->collect_regset (regset, tdep->tls_regnum, regnum, buf, size);
+}
+
+const struct regset arm_fbsd_tls_regset =
+  {
+    arm_fbsd_tls_regmap,
+    arm_fbsd_supply_tls_regset, arm_fbsd_collect_tls_regset
+  };
+
 /* Implement the "iterate_over_regset_sections" gdbarch method.  */
 
 static void
@@ -165,22 +199,8 @@ arm_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
       &arm_fbsd_gregset, NULL, cb_data);
 
   if (tdep->tls_regnum > 0)
-    {
-      const struct regcache_map_entry arm_fbsd_tlsregmap[] =
-	{
-	  { 1, tdep->tls_regnum, 4 },
-	  { 0 }
-	};
-
-      const struct regset arm_fbsd_tlsregset =
-	{
-	  arm_fbsd_tlsregmap,
-	  regcache_supply_regset, regcache_collect_regset
-	};
-
-      cb (".reg-aarch-tls", ARM_FBSD_SIZEOF_TLSREGSET, ARM_FBSD_SIZEOF_TLSREGSET,
-	  &arm_fbsd_tlsregset, NULL, cb_data);
-    }
+    cb (".reg-aarch-tls", ARM_FBSD_SIZEOF_TLSREGSET, ARM_FBSD_SIZEOF_TLSREGSET,
+	&arm_fbsd_tls_regset, NULL, cb_data);
 
   /* While FreeBSD/arm cores do contain a NT_FPREGSET / ".reg2"
      register set, it is not populated with register values by the
diff --git a/gdb/arm-fbsd-tdep.h b/gdb/arm-fbsd-tdep.h
index 193eb76df3c..923b749367e 100644
--- a/gdb/arm-fbsd-tdep.h
+++ b/gdb/arm-fbsd-tdep.h
@@ -35,6 +35,7 @@
 
 extern const struct regset arm_fbsd_gregset;
 extern const struct regset arm_fbsd_vfpregset;
+extern const struct regset arm_fbsd_tls_regset;
 
 /* Flags passed in AT_HWCAP. */
 #define	HWCAP_VFP		0x00000040
-- 
2.36.1


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

* [PATCH 5/5] aarch64-fbsd: Use a static regset for the TLS register set.
  2022-07-08  0:58 [PATCH 0/5] Improve support for regcache_map_entry with variable register base John Baldwin
                   ` (3 preceding siblings ...)
  2022-07-08  0:58 ` [PATCH 4/5] arm-fbsd: Use a static regset for the TLS register set John Baldwin
@ 2022-07-08  0:58 ` John Baldwin
  2022-11-22 20:40   ` Simon Marchi
  2022-07-21 14:53 ` [PATCH 0/5] Improve support for regcache_map_entry with variable register base John Baldwin
  5 siblings, 1 reply; 20+ messages in thread
From: John Baldwin @ 2022-07-08  0:58 UTC (permalink / raw)
  To: gdb-patches

This uses custom collect/supply regset handlers which pass the TLS
register number from the gdbarch_tdep as the base register number.
---
 gdb/aarch64-fbsd-nat.c  | 36 +++------------------------
 gdb/aarch64-fbsd-tdep.c | 54 ++++++++++++++++++++++++++++-------------
 gdb/aarch64-fbsd-tdep.h |  1 +
 3 files changed, 42 insertions(+), 49 deletions(-)

diff --git a/gdb/aarch64-fbsd-nat.c b/gdb/aarch64-fbsd-nat.c
index d8cf6227e73..4c8e9c4fa81 100644
--- a/gdb/aarch64-fbsd-nat.c
+++ b/gdb/aarch64-fbsd-nat.c
@@ -92,22 +92,8 @@ aarch64_fbsd_nat_target::fetch_registers (struct regcache *regcache,
   gdbarch *gdbarch = regcache->arch ();
   aarch64_gdbarch_tdep *tdep = (aarch64_gdbarch_tdep *) gdbarch_tdep (gdbarch);
   if (tdep->has_tls ())
-    {
-      const struct regcache_map_entry aarch64_fbsd_tls_regmap[] =
-	{
-	  { 1, tdep->tls_regnum, 8 },
-	  { 0 }
-	};
-
-      const struct regset aarch64_fbsd_tls_regset =
-	{
-	  aarch64_fbsd_tls_regmap,
-	  regcache_supply_regset, regcache_collect_regset
-	};
-
-      fetch_regset<uint64_t> (regcache, regnum, NT_ARM_TLS,
-			      &aarch64_fbsd_tls_regset);
-    }
+    fetch_regset<uint64_t> (regcache, regnum, NT_ARM_TLS,
+			    &aarch64_fbsd_tls_regset, tdep->tls_regnum);
 }
 
 /* Store register REGNUM back into the inferior.  If REGNUM is -1, do
@@ -125,22 +111,8 @@ aarch64_fbsd_nat_target::store_registers (struct regcache *regcache,
   gdbarch *gdbarch = regcache->arch ();
   aarch64_gdbarch_tdep *tdep = (aarch64_gdbarch_tdep *) gdbarch_tdep (gdbarch);
   if (tdep->has_tls ())
-    {
-      const struct regcache_map_entry aarch64_fbsd_tls_regmap[] =
-	{
-	  { 1, tdep->tls_regnum, 8 },
-	  { 0 }
-	};
-
-      const struct regset aarch64_fbsd_tls_regset =
-	{
-	  aarch64_fbsd_tls_regmap,
-	  regcache_supply_regset, regcache_collect_regset
-	};
-
-      store_regset<uint64_t> (regcache, regnum, NT_ARM_TLS,
-			      &aarch64_fbsd_tls_regset);
-    }
+    store_regset<uint64_t> (regcache, regnum, NT_ARM_TLS,
+			    &aarch64_fbsd_tls_regset, tdep->tls_regnum);
 }
 
 /* Implement the target read_description method.  */
diff --git a/gdb/aarch64-fbsd-tdep.c b/gdb/aarch64-fbsd-tdep.c
index 891546b3c64..d7d15009efe 100644
--- a/gdb/aarch64-fbsd-tdep.c
+++ b/gdb/aarch64-fbsd-tdep.c
@@ -50,6 +50,12 @@ static const struct regcache_map_entry aarch64_fbsd_fpregmap[] =
     { 0 }
   };
 
+static const struct regcache_map_entry aarch64_fbsd_tls_regmap[] =
+  {
+    { 1, 0, 8 },
+    { 0 }
+  };
+
 /* In a signal frame, sp points to a 'struct sigframe' which is
    defined as:
 
@@ -135,6 +141,34 @@ const struct regset aarch64_fbsd_fpregset =
     regcache_supply_regset, regcache_collect_regset
   };
 
+static void
+aarch64_fbsd_supply_tls_regset (const struct regset *regset,
+				struct regcache *regcache,
+				int regnum, const void *buf, size_t size)
+{
+  struct gdbarch *gdbarch = regcache->arch ();
+  aarch64_gdbarch_tdep *tdep = (aarch64_gdbarch_tdep *) gdbarch_tdep (gdbarch);
+
+  regcache->supply_regset (regset, tdep->tls_regnum, regnum, buf, size);
+}
+
+static void
+aarch64_fbsd_collect_tls_regset (const struct regset *regset,
+				 const struct regcache *regcache,
+				 int regnum, void *buf, size_t size)
+{
+  struct gdbarch *gdbarch = regcache->arch ();
+  aarch64_gdbarch_tdep *tdep = (aarch64_gdbarch_tdep *) gdbarch_tdep (gdbarch);
+
+  regcache->collect_regset (regset, tdep->tls_regnum, regnum, buf, size);
+}
+
+const struct regset aarch64_fbsd_tls_regset =
+  {
+    aarch64_fbsd_tls_regmap,
+    aarch64_fbsd_supply_tls_regset, aarch64_fbsd_collect_tls_regset
+  };
+
 /* Implement the "iterate_over_regset_sections" gdbarch method.  */
 
 static void
@@ -151,23 +185,9 @@ aarch64_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
       &aarch64_fbsd_fpregset, NULL, cb_data);
 
   if (tdep->has_tls ())
-    {
-      const struct regcache_map_entry aarch64_fbsd_tls_regmap[] =
-	{
-	  { 1, tdep->tls_regnum, 8 },
-	  { 0 }
-	};
-
-      const struct regset aarch64_fbsd_tls_regset =
-	{
-	  aarch64_fbsd_tls_regmap,
-	  regcache_supply_regset, regcache_collect_regset
-	};
-
-      cb (".reg-aarch-tls", AARCH64_FBSD_SIZEOF_TLSREGSET,
-	  AARCH64_FBSD_SIZEOF_TLSREGSET, &aarch64_fbsd_tls_regset,
-	  "TLS register", cb_data);
-    }
+    cb (".reg-aarch-tls", AARCH64_FBSD_SIZEOF_TLSREGSET,
+	AARCH64_FBSD_SIZEOF_TLSREGSET, &aarch64_fbsd_tls_regset,
+	"TLS register", cb_data);
 }
 
 /* Implement the "core_read_description" gdbarch method.  */
diff --git a/gdb/aarch64-fbsd-tdep.h b/gdb/aarch64-fbsd-tdep.h
index 7419ea6be03..eaf0774f595 100644
--- a/gdb/aarch64-fbsd-tdep.h
+++ b/gdb/aarch64-fbsd-tdep.h
@@ -37,5 +37,6 @@
 
 extern const struct regset aarch64_fbsd_gregset;
 extern const struct regset aarch64_fbsd_fpregset;
+extern const struct regset aarch64_fbsd_tls_regset;
 
 #endif /* AARCH64_FBSD_TDEP_H */
-- 
2.36.1


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

* Re: [PATCH 0/5] Improve support for regcache_map_entry with variable register base
  2022-07-08  0:58 [PATCH 0/5] Improve support for regcache_map_entry with variable register base John Baldwin
                   ` (4 preceding siblings ...)
  2022-07-08  0:58 ` [PATCH 5/5] aarch64-fbsd: " John Baldwin
@ 2022-07-21 14:53 ` John Baldwin
  2022-08-22 18:11   ` [PING] " John Baldwin
  5 siblings, 1 reply; 20+ messages in thread
From: John Baldwin @ 2022-07-21 14:53 UTC (permalink / raw)
  To: gdb-patches

On 7/7/22 5:58 PM, John Baldwin wrote:
> When I added support for TLS to the ARM and AArch64 architectures on
> FreeBSD, I had to construct register maps and associated register sets
> on the fly in both the callback for processing register core dump
> notes and in the native target routines to fetch and store registers.
> 
> The reason I had to create these on the fly is that the register
> number was not known at compile time, so the register map and register
> set could not be constant structures shared among the -tdep and -nat
> code the way other register sets were.
> 
> This series aims to rectify that.  The first patch adds new variants
> of regcache::collect_regset and regcache::supply_regset that accept an
> additional 'regbase' argument.  When these functions iterate over
> regcache map entries, the effective register number for the entry is
> computed by adding the value in the entry to 'regbase' permittting the
> regcache map entries to hold relative register numbers for a block of
> registers.
> 
> The rest of the series then makes use of this to use a single, shared
> register map and register set for TLS on FreeBSD/ARM and
> FreeBSD/AArch64.  Using this requires custom supply and collect regset
> routines for the TLS regsets that extract the base register number
> from the gdbarch's tdep and invoke the new regcache class methods, so
> patch 2 updates the wrappers for dealing with regsets in fbsd-nat to
> always invoke the regset routines from the regset instead of directly
> calling the default functions.  Patch 3 is another change to fbsd-nat
> to cope with the fact that regcache_map_supplies() needs to pass in
> the relative register number in the wrapper routines rather than the
> absolute register number.
> 
> Patches 4 and 5 are the updates to the ARM and AArch64 FreeBSD
> architectures and targets.
> 
> I have not done the work to make use of this in the Linux AArch64
> architecture, though I think it would apply to at least the TLS and
> MTE register sets there in a similar fashion.  SVE is harder because
> the register sizes change (though SVE might be able to make use of
> this if register_size() returns the right value by using a register
> size of 0 in the relevant register cache map entries).
> 
> John Baldwin (5):
>    regcache: Add collect/supply_regset variants that accept a register
>      base.
>    fbsd-nat: Use regset supply/collect methods.
>    fbsd-nat: Pass an optional register base to the register set helpers.
>    arm-fbsd: Use a static regset for the TLS register set.
>    aarch64-fbsd: Use a static regset for the TLS register set.
> 
>   gdb/aarch64-fbsd-nat.c  | 36 +++------------------------
>   gdb/aarch64-fbsd-tdep.c | 54 ++++++++++++++++++++++++++++-------------
>   gdb/aarch64-fbsd-tdep.h |  1 +
>   gdb/arm-fbsd-nat.c      | 34 +++-----------------------
>   gdb/arm-fbsd-tdep.c     | 52 +++++++++++++++++++++++++++------------
>   gdb/arm-fbsd-tdep.h     |  1 +
>   gdb/fbsd-nat.c          | 42 +++++++++++++++++---------------
>   gdb/fbsd-nat.h          | 49 +++++++++++++++++++++----------------
>   gdb/regcache.c          | 28 ++++++++++++++++++---
>   gdb/regcache.h          | 24 ++++++++++++++++--
>   10 files changed, 181 insertions(+), 140 deletions(-)

Ping.

-- 
John Baldwin

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

* Re: [PING] [PATCH 0/5] Improve support for regcache_map_entry with variable register base
  2022-07-21 14:53 ` [PATCH 0/5] Improve support for regcache_map_entry with variable register base John Baldwin
@ 2022-08-22 18:11   ` John Baldwin
  2022-09-20 17:50     ` John Baldwin
  0 siblings, 1 reply; 20+ messages in thread
From: John Baldwin @ 2022-08-22 18:11 UTC (permalink / raw)
  To: gdb-patches

On 7/21/22 7:53 AM, John Baldwin wrote:
> On 7/7/22 5:58 PM, John Baldwin wrote:
>> When I added support for TLS to the ARM and AArch64 architectures on
>> FreeBSD, I had to construct register maps and associated register sets
>> on the fly in both the callback for processing register core dump
>> notes and in the native target routines to fetch and store registers.
>>
>> The reason I had to create these on the fly is that the register
>> number was not known at compile time, so the register map and register
>> set could not be constant structures shared among the -tdep and -nat
>> code the way other register sets were.
>>
>> This series aims to rectify that.  The first patch adds new variants
>> of regcache::collect_regset and regcache::supply_regset that accept an
>> additional 'regbase' argument.  When these functions iterate over
>> regcache map entries, the effective register number for the entry is
>> computed by adding the value in the entry to 'regbase' permittting the
>> regcache map entries to hold relative register numbers for a block of
>> registers.
>>
>> The rest of the series then makes use of this to use a single, shared
>> register map and register set for TLS on FreeBSD/ARM and
>> FreeBSD/AArch64.  Using this requires custom supply and collect regset
>> routines for the TLS regsets that extract the base register number
>> from the gdbarch's tdep and invoke the new regcache class methods, so
>> patch 2 updates the wrappers for dealing with regsets in fbsd-nat to
>> always invoke the regset routines from the regset instead of directly
>> calling the default functions.  Patch 3 is another change to fbsd-nat
>> to cope with the fact that regcache_map_supplies() needs to pass in
>> the relative register number in the wrapper routines rather than the
>> absolute register number.
>>
>> Patches 4 and 5 are the updates to the ARM and AArch64 FreeBSD
>> architectures and targets.
>>
>> I have not done the work to make use of this in the Linux AArch64
>> architecture, though I think it would apply to at least the TLS and
>> MTE register sets there in a similar fashion.  SVE is harder because
>> the register sizes change (though SVE might be able to make use of
>> this if register_size() returns the right value by using a register
>> size of 0 in the relevant register cache map entries).
>>
>> John Baldwin (5):
>>     regcache: Add collect/supply_regset variants that accept a register
>>       base.
>>     fbsd-nat: Use regset supply/collect methods.
>>     fbsd-nat: Pass an optional register base to the register set helpers.
>>     arm-fbsd: Use a static regset for the TLS register set.
>>     aarch64-fbsd: Use a static regset for the TLS register set.
>>
>>    gdb/aarch64-fbsd-nat.c  | 36 +++------------------------
>>    gdb/aarch64-fbsd-tdep.c | 54 ++++++++++++++++++++++++++++-------------
>>    gdb/aarch64-fbsd-tdep.h |  1 +
>>    gdb/arm-fbsd-nat.c      | 34 +++-----------------------
>>    gdb/arm-fbsd-tdep.c     | 52 +++++++++++++++++++++++++++------------
>>    gdb/arm-fbsd-tdep.h     |  1 +
>>    gdb/fbsd-nat.c          | 42 +++++++++++++++++---------------
>>    gdb/fbsd-nat.h          | 49 +++++++++++++++++++++----------------
>>    gdb/regcache.c          | 28 ++++++++++++++++++---
>>    gdb/regcache.h          | 24 ++++++++++++++++--
>>    10 files changed, 181 insertions(+), 140 deletions(-)
> 
> Ping.
> 


-- 
John Baldwin

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

* Re: [PING] [PATCH 0/5] Improve support for regcache_map_entry with variable register base
  2022-08-22 18:11   ` [PING] " John Baldwin
@ 2022-09-20 17:50     ` John Baldwin
  2022-10-20 20:26       ` [PING 4] " John Baldwin
  0 siblings, 1 reply; 20+ messages in thread
From: John Baldwin @ 2022-09-20 17:50 UTC (permalink / raw)
  To: gdb-patches

On 8/22/22 7:11 PM, John Baldwin wrote:
> On 7/21/22 7:53 AM, John Baldwin wrote:
>> On 7/7/22 5:58 PM, John Baldwin wrote:
>>> When I added support for TLS to the ARM and AArch64 architectures on
>>> FreeBSD, I had to construct register maps and associated register sets
>>> on the fly in both the callback for processing register core dump
>>> notes and in the native target routines to fetch and store registers.
>>>
>>> The reason I had to create these on the fly is that the register
>>> number was not known at compile time, so the register map and register
>>> set could not be constant structures shared among the -tdep and -nat
>>> code the way other register sets were.
>>>
>>> This series aims to rectify that.  The first patch adds new variants
>>> of regcache::collect_regset and regcache::supply_regset that accept an
>>> additional 'regbase' argument.  When these functions iterate over
>>> regcache map entries, the effective register number for the entry is
>>> computed by adding the value in the entry to 'regbase' permittting the
>>> regcache map entries to hold relative register numbers for a block of
>>> registers.
>>>
>>> The rest of the series then makes use of this to use a single, shared
>>> register map and register set for TLS on FreeBSD/ARM and
>>> FreeBSD/AArch64.  Using this requires custom supply and collect regset
>>> routines for the TLS regsets that extract the base register number
>>> from the gdbarch's tdep and invoke the new regcache class methods, so
>>> patch 2 updates the wrappers for dealing with regsets in fbsd-nat to
>>> always invoke the regset routines from the regset instead of directly
>>> calling the default functions.  Patch 3 is another change to fbsd-nat
>>> to cope with the fact that regcache_map_supplies() needs to pass in
>>> the relative register number in the wrapper routines rather than the
>>> absolute register number.
>>>
>>> Patches 4 and 5 are the updates to the ARM and AArch64 FreeBSD
>>> architectures and targets.
>>>
>>> I have not done the work to make use of this in the Linux AArch64
>>> architecture, though I think it would apply to at least the TLS and
>>> MTE register sets there in a similar fashion.  SVE is harder because
>>> the register sizes change (though SVE might be able to make use of
>>> this if register_size() returns the right value by using a register
>>> size of 0 in the relevant register cache map entries).
>>>
>>> John Baldwin (5):
>>>      regcache: Add collect/supply_regset variants that accept a register
>>>        base.
>>>      fbsd-nat: Use regset supply/collect methods.
>>>      fbsd-nat: Pass an optional register base to the register set helpers.
>>>      arm-fbsd: Use a static regset for the TLS register set.
>>>      aarch64-fbsd: Use a static regset for the TLS register set.
>>>
>>>     gdb/aarch64-fbsd-nat.c  | 36 +++------------------------
>>>     gdb/aarch64-fbsd-tdep.c | 54 ++++++++++++++++++++++++++++-------------
>>>     gdb/aarch64-fbsd-tdep.h |  1 +
>>>     gdb/arm-fbsd-nat.c      | 34 +++-----------------------
>>>     gdb/arm-fbsd-tdep.c     | 52 +++++++++++++++++++++++++++------------
>>>     gdb/arm-fbsd-tdep.h     |  1 +
>>>     gdb/fbsd-nat.c          | 42 +++++++++++++++++---------------
>>>     gdb/fbsd-nat.h          | 49 +++++++++++++++++++++----------------
>>>     gdb/regcache.c          | 28 ++++++++++++++++++---
>>>     gdb/regcache.h          | 24 ++++++++++++++++--
>>>     10 files changed, 181 insertions(+), 140 deletions(-)
>>
>> Ping.
>>
> 
> 

Pinging again.  Only patch 1 needs approval by another maintainer, the rest
are FreeBSD specific.

-- 
John Baldwin

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

* Re: [PING 4] [PATCH 0/5] Improve support for regcache_map_entry with variable register base
  2022-09-20 17:50     ` John Baldwin
@ 2022-10-20 20:26       ` John Baldwin
  2022-11-21 18:21         ` [PING 5] " John Baldwin
  0 siblings, 1 reply; 20+ messages in thread
From: John Baldwin @ 2022-10-20 20:26 UTC (permalink / raw)
  To: gdb-patches

On 9/20/22 10:50 AM, John Baldwin wrote:
> On 8/22/22 7:11 PM, John Baldwin wrote:
>> On 7/21/22 7:53 AM, John Baldwin wrote:
>>> On 7/7/22 5:58 PM, John Baldwin wrote:
>>>> When I added support for TLS to the ARM and AArch64 architectures on
>>>> FreeBSD, I had to construct register maps and associated register sets
>>>> on the fly in both the callback for processing register core dump
>>>> notes and in the native target routines to fetch and store registers.
>>>>
>>>> The reason I had to create these on the fly is that the register
>>>> number was not known at compile time, so the register map and register
>>>> set could not be constant structures shared among the -tdep and -nat
>>>> code the way other register sets were.
>>>>
>>>> This series aims to rectify that.  The first patch adds new variants
>>>> of regcache::collect_regset and regcache::supply_regset that accept an
>>>> additional 'regbase' argument.  When these functions iterate over
>>>> regcache map entries, the effective register number for the entry is
>>>> computed by adding the value in the entry to 'regbase' permittting the
>>>> regcache map entries to hold relative register numbers for a block of
>>>> registers.
>>>>
>>>> The rest of the series then makes use of this to use a single, shared
>>>> register map and register set for TLS on FreeBSD/ARM and
>>>> FreeBSD/AArch64.  Using this requires custom supply and collect regset
>>>> routines for the TLS regsets that extract the base register number
>>>> from the gdbarch's tdep and invoke the new regcache class methods, so
>>>> patch 2 updates the wrappers for dealing with regsets in fbsd-nat to
>>>> always invoke the regset routines from the regset instead of directly
>>>> calling the default functions.  Patch 3 is another change to fbsd-nat
>>>> to cope with the fact that regcache_map_supplies() needs to pass in
>>>> the relative register number in the wrapper routines rather than the
>>>> absolute register number.
>>>>
>>>> Patches 4 and 5 are the updates to the ARM and AArch64 FreeBSD
>>>> architectures and targets.
>>>>
>>>> I have not done the work to make use of this in the Linux AArch64
>>>> architecture, though I think it would apply to at least the TLS and
>>>> MTE register sets there in a similar fashion.  SVE is harder because
>>>> the register sizes change (though SVE might be able to make use of
>>>> this if register_size() returns the right value by using a register
>>>> size of 0 in the relevant register cache map entries).
>>>>
>>>> John Baldwin (5):
>>>>       regcache: Add collect/supply_regset variants that accept a register
>>>>         base.
>>>>       fbsd-nat: Use regset supply/collect methods.
>>>>       fbsd-nat: Pass an optional register base to the register set helpers.
>>>>       arm-fbsd: Use a static regset for the TLS register set.
>>>>       aarch64-fbsd: Use a static regset for the TLS register set.
>>>>
>>>>      gdb/aarch64-fbsd-nat.c  | 36 +++------------------------
>>>>      gdb/aarch64-fbsd-tdep.c | 54 ++++++++++++++++++++++++++++-------------
>>>>      gdb/aarch64-fbsd-tdep.h |  1 +
>>>>      gdb/arm-fbsd-nat.c      | 34 +++-----------------------
>>>>      gdb/arm-fbsd-tdep.c     | 52 +++++++++++++++++++++++++++------------
>>>>      gdb/arm-fbsd-tdep.h     |  1 +
>>>>      gdb/fbsd-nat.c          | 42 +++++++++++++++++---------------
>>>>      gdb/fbsd-nat.h          | 49 +++++++++++++++++++++----------------
>>>>      gdb/regcache.c          | 28 ++++++++++++++++++---
>>>>      gdb/regcache.h          | 24 ++++++++++++++++--
>>>>      10 files changed, 181 insertions(+), 140 deletions(-)
>>>
>>> Ping.
>>>
>>
>>
> 
> Pinging again.  Only patch 1 needs approval by another maintainer, the rest
> are FreeBSD specific.
> 


-- 
John Baldwin

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

* Re: [PING 5] [PATCH 0/5] Improve support for regcache_map_entry with variable register base
  2022-10-20 20:26       ` [PING 4] " John Baldwin
@ 2022-11-21 18:21         ` John Baldwin
  0 siblings, 0 replies; 20+ messages in thread
From: John Baldwin @ 2022-11-21 18:21 UTC (permalink / raw)
  To: gdb-patches

On 10/20/22 1:26 PM, John Baldwin wrote:
> On 9/20/22 10:50 AM, John Baldwin wrote:
>> On 8/22/22 7:11 PM, John Baldwin wrote:
>>> On 7/21/22 7:53 AM, John Baldwin wrote:
>>>> On 7/7/22 5:58 PM, John Baldwin wrote:
>>>>> When I added support for TLS to the ARM and AArch64 architectures on
>>>>> FreeBSD, I had to construct register maps and associated register sets
>>>>> on the fly in both the callback for processing register core dump
>>>>> notes and in the native target routines to fetch and store registers.
>>>>>
>>>>> The reason I had to create these on the fly is that the register
>>>>> number was not known at compile time, so the register map and register
>>>>> set could not be constant structures shared among the -tdep and -nat
>>>>> code the way other register sets were.
>>>>>
>>>>> This series aims to rectify that.  The first patch adds new variants
>>>>> of regcache::collect_regset and regcache::supply_regset that accept an
>>>>> additional 'regbase' argument.  When these functions iterate over
>>>>> regcache map entries, the effective register number for the entry is
>>>>> computed by adding the value in the entry to 'regbase' permittting the
>>>>> regcache map entries to hold relative register numbers for a block of
>>>>> registers.
>>>>>
>>>>> The rest of the series then makes use of this to use a single, shared
>>>>> register map and register set for TLS on FreeBSD/ARM and
>>>>> FreeBSD/AArch64.  Using this requires custom supply and collect regset
>>>>> routines for the TLS regsets that extract the base register number
>>>>> from the gdbarch's tdep and invoke the new regcache class methods, so
>>>>> patch 2 updates the wrappers for dealing with regsets in fbsd-nat to
>>>>> always invoke the regset routines from the regset instead of directly
>>>>> calling the default functions.  Patch 3 is another change to fbsd-nat
>>>>> to cope with the fact that regcache_map_supplies() needs to pass in
>>>>> the relative register number in the wrapper routines rather than the
>>>>> absolute register number.
>>>>>
>>>>> Patches 4 and 5 are the updates to the ARM and AArch64 FreeBSD
>>>>> architectures and targets.
>>>>>
>>>>> I have not done the work to make use of this in the Linux AArch64
>>>>> architecture, though I think it would apply to at least the TLS and
>>>>> MTE register sets there in a similar fashion.  SVE is harder because
>>>>> the register sizes change (though SVE might be able to make use of
>>>>> this if register_size() returns the right value by using a register
>>>>> size of 0 in the relevant register cache map entries).
>>>>>
>>>>> John Baldwin (5):
>>>>>        regcache: Add collect/supply_regset variants that accept a register
>>>>>          base.
>>>>>        fbsd-nat: Use regset supply/collect methods.
>>>>>        fbsd-nat: Pass an optional register base to the register set helpers.
>>>>>        arm-fbsd: Use a static regset for the TLS register set.
>>>>>        aarch64-fbsd: Use a static regset for the TLS register set.
>>>>>
>>>>>       gdb/aarch64-fbsd-nat.c  | 36 +++------------------------
>>>>>       gdb/aarch64-fbsd-tdep.c | 54 ++++++++++++++++++++++++++++-------------
>>>>>       gdb/aarch64-fbsd-tdep.h |  1 +
>>>>>       gdb/arm-fbsd-nat.c      | 34 +++-----------------------
>>>>>       gdb/arm-fbsd-tdep.c     | 52 +++++++++++++++++++++++++++------------
>>>>>       gdb/arm-fbsd-tdep.h     |  1 +
>>>>>       gdb/fbsd-nat.c          | 42 +++++++++++++++++---------------
>>>>>       gdb/fbsd-nat.h          | 49 +++++++++++++++++++++----------------
>>>>>       gdb/regcache.c          | 28 ++++++++++++++++++---
>>>>>       gdb/regcache.h          | 24 ++++++++++++++++--
>>>>>       10 files changed, 181 insertions(+), 140 deletions(-)
>>>>
>>>> Ping.
>>>>
>>>
>>>
>>
>> Pinging again.  Only patch 1 needs approval by another maintainer, the rest
>> are FreeBSD specific.
>>
> 
> 

Is there a better way to ping things such that this gets on someone's list to look at?

-- 
John Baldwin


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

* Re: [PATCH 1/5] regcache: Add collect/supply_regset variants that accept a register base.
  2022-07-08  0:58 ` [PATCH 1/5] regcache: Add collect/supply_regset variants that accept a " John Baldwin
@ 2022-11-22 19:56   ` Simon Marchi
  2022-11-22 20:18     ` Simon Marchi
  2022-11-22 22:32     ` John Baldwin
  0 siblings, 2 replies; 20+ messages in thread
From: Simon Marchi @ 2022-11-22 19:56 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

On 7/7/22 20:58, John Baldwin wrote:
> Some register sets described by an array of regcache_map_entry
> structures do not have fixed register numbers in their associated
> architecture but do describe a block of registers whose numbers are at
> fixed offsets relative to some base register value.  An example of
> this are the TLS register sets for the ARM and AArch64 architectures.
> 
> Currently OS-specific architectures create register maps and register
> sets dynamically using the register base number.  However, this
> requires duplicating the code to create the register map and register
> set.  To reduce duplication, add variants of the collect_regset and
> supply_regset regcache methods which accept a base register number.
> For valid register map entries (i.e. not REGCACHE_MAP_SKIP), add this
> base register number to the value from the map entry to determine the
> final register number.

The patch LGTM.  I have some small comments, once addressed you can put
my:

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


> ---
>  gdb/regcache.c | 28 +++++++++++++++++++++++++---
>  gdb/regcache.h | 24 ++++++++++++++++++++++--
>  2 files changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 037659ef8fa..1db3d972ef8 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1180,7 +1180,7 @@ regcache::transfer_regset_register (struct regcache *out_regcache, int regnum,
>  /* See regcache.h.  */
>  
>  void
> -regcache::transfer_regset (const struct regset *regset,
> +regcache::transfer_regset (const struct regset *regset, int regbase,
>  			   struct regcache *out_regcache,
>  			   int regnum, const gdb_byte *in_buf,
>  			   gdb_byte *out_buf, size_t size) const
> @@ -1195,6 +1195,9 @@ regcache::transfer_regset (const struct regset *regset,
>        int regno = map->regno;
>        int slot_size = map->size;
>  
> +      if (regno != REGCACHE_MAP_SKIP)
> +	regno += regbase;
> +
>        if (slot_size == 0 && regno != REGCACHE_MAP_SKIP)
>  	slot_size = m_descr->sizeof_register[regno];
>  
> @@ -1242,7 +1245,18 @@ void
>  regcache::supply_regset (const struct regset *regset,
>  			 int regnum, const void *buf, size_t size)
>  {
> -  transfer_regset (regset, this, regnum, (const gdb_byte *) buf, nullptr, size);
> +  transfer_regset (regset, 0, this, regnum, (const gdb_byte *) buf, nullptr,
> +		   size);
> +}

Can the old regcache::{supply,collect}_regset (without regbase) become
trivial wrappers around the new ones (with regbase)?  I would put the
implementation directly in the .h if doing that.

> +
> +/* See regcache.h.  */
> +
> +void
> +regcache::supply_regset (const struct regset *regset, int regbase,
> +			 int regnum, const void *buf, size_t size)
> +{
> +  transfer_regset (regset, regbase, this, regnum, (const gdb_byte *) buf,
> +		   nullptr, size);
>  }
>  
>  /* Collect register REGNUM from REGCACHE to BUF, using the register
> @@ -1261,11 +1275,19 @@ void
>  regcache::collect_regset (const struct regset *regset,
>  			 int regnum, void *buf, size_t size) const
>  {
> -  transfer_regset (regset, nullptr, regnum, nullptr, (gdb_byte *) buf, size);
> +  transfer_regset (regset, 0, nullptr, regnum, nullptr, (gdb_byte *) buf, size);
>  }
>  
>  /* See regcache.h  */
>  
> +void
> +regcache::collect_regset (const struct regset *regset, int regbase,
> +			 int regnum, void *buf, size_t size) const
> +{
> +  transfer_regset (regset, regbase, nullptr, regnum, nullptr, (gdb_byte *) buf,
> +		   size);
> +}
> +
>  bool
>  regcache_map_supplies (const struct regcache_map_entry *map, int regnum,
>  		       struct gdbarch *gdbarch, size_t size)
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index 1dbba5ce9af..f01127b20fb 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -150,6 +150,14 @@ extern void regcache_collect_regset (const struct regset *regset,
>  				     int regnum, void *buf, size_t size);
>  
>  
> +extern void regcache_supply_regset (const struct regset *regset, int regbase,
> +				    struct regcache *regcache,
> +				    int regnum, const void *buf,
> +				    size_t size);
> +extern void regcache_collect_regset (const struct regset *regset, int regbase,
> +				     const struct regcache *regcache,
> +				     int regnum, void *buf, size_t size);

These don't have a definition, I guess they are not needed.

(Could we make a patch that removes regcache_supply_regset and
regcache_collect_regset, since they just forward to the regcache
methods?)

> +     belonging to the regset, otherwise just the register numbered
> +     REGNUM.  The REGSET's 'regmap' field must point to an array of
> +     'struct regcache_map_entry'.  The valid register numbers in each
> +     entry in 'struct regcache_map_entry' are offset by REGBASE.  */
> +
> +  void supply_regset (const struct regset *regset, int regbase,
> +		      int regnum, const void *buf, size_t size);
> +
> +  void collect_regset (const struct regset *regset, int regbase, int regnum,
> +		       void *buf, size_t size) const;
> +
>    void supply_regset (const struct regset *regset,
>  		      int regnum, const void *buf, size_t size);
>  
> -
>    void collect_regset (const struct regset *regset, int regnum,
>  		       void *buf, size_t size) const;

Can you document the last two, just by saying something like "Same as
the above, but with REGBASE == 0"?

Simon

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

* Re: [PATCH 1/5] regcache: Add collect/supply_regset variants that accept a register base.
  2022-11-22 19:56   ` Simon Marchi
@ 2022-11-22 20:18     ` Simon Marchi
  2022-11-22 22:32     ` John Baldwin
  1 sibling, 0 replies; 20+ messages in thread
From: Simon Marchi @ 2022-11-22 20:18 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

>> diff --git a/gdb/regcache.h b/gdb/regcache.h
>> index 1dbba5ce9af..f01127b20fb 100644
>> --- a/gdb/regcache.h
>> +++ b/gdb/regcache.h
>> @@ -150,6 +150,14 @@ extern void regcache_collect_regset (const struct regset *regset,
>>  				     int regnum, void *buf, size_t size);
>>  
>>  
>> +extern void regcache_supply_regset (const struct regset *regset, int regbase,
>> +				    struct regcache *regcache,
>> +				    int regnum, const void *buf,
>> +				    size_t size);
>> +extern void regcache_collect_regset (const struct regset *regset, int regbase,
>> +				     const struct regcache *regcache,
>> +				     int regnum, void *buf, size_t size);
> 
> These don't have a definition, I guess they are not needed.
> 
> (Could we make a patch that removes regcache_supply_regset and
> regcache_collect_regset, since they just forward to the regcache
> methods?)

Answering my own question, no we can't.  They are used as callbacks in
regset structures.

Simon


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

* Re: [PATCH 2/5] fbsd-nat: Use regset supply/collect methods.
  2022-07-08  0:58 ` [PATCH 2/5] fbsd-nat: Use regset supply/collect methods John Baldwin
@ 2022-11-22 20:20   ` Simon Marchi
  0 siblings, 0 replies; 20+ messages in thread
From: Simon Marchi @ 2022-11-22 20:20 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

On 7/7/22 20:58, John Baldwin wrote:
> fbsd-nat includes various helper routines for fetching and storing
> register sets via ptrace where the register set is described by a
> regset.  These helper routines directly invoke the
> supply/collect_regset regcache methods which doesn't permit a regset
> to provide custom logic when fetching or storing a register set.
> Instead, just use the function pointers from the struct regset
> directly.
> ---
>  gdb/fbsd-nat.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 281b034b115..9d7383ac0ab 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -1744,7 +1744,7 @@ fbsd_nat_target::fetch_register_set (struct regcache *regcache, int regnum,
>        if (ptrace (fetch_op, pid, (PTRACE_TYPE_ARG3) regs, 0) == -1)
>  	perror_with_name (_("Couldn't get registers"));
>  
> -      regcache->supply_regset (regset, regnum, regs, size);
> +      regset->supply_regset (regset, regcache, regnum, regs, size);
>        return true;
>      }
>    return false;
> @@ -1768,7 +1768,7 @@ fbsd_nat_target::store_register_set (struct regcache *regcache, int regnum,
>        if (ptrace (fetch_op, pid, (PTRACE_TYPE_ARG3) regs, 0) == -1)
>  	perror_with_name (_("Couldn't get registers"));
>  
> -      regcache->collect_regset (regset, regnum, regs, size);
> +      regset->collect_regset (regset, regcache, regnum, regs, size);
>  
>        if (ptrace (store_op, pid, (PTRACE_TYPE_ARG3) regs, 0) == -1)
>  	perror_with_name (_("Couldn't write registers"));
> @@ -1813,7 +1813,7 @@ fbsd_nat_target::fetch_regset (struct regcache *regcache, int regnum, int note,
>        if (ptrace (PT_GETREGSET, pid, (PTRACE_TYPE_ARG3) &iov, note) == -1)
>  	perror_with_name (_("Couldn't get registers"));
>  
> -      regcache->supply_regset (regset, regnum, regs, size);
> +      regset->supply_regset (regset, regcache, regnum, regs, size);
>        return true;
>      }
>    return false;
> @@ -1838,7 +1838,7 @@ fbsd_nat_target::store_regset (struct regcache *regcache, int regnum, int note,
>        if (ptrace (PT_GETREGSET, pid, (PTRACE_TYPE_ARG3) &iov, note) == -1)
>  	perror_with_name (_("Couldn't get registers"));
>  
> -      regcache->collect_regset (regset, regnum, regs, size);
> +      regset->collect_regset (regset, regcache, regnum, regs, size);
>  
>        if (ptrace (PT_SETREGSET, pid, (PTRACE_TYPE_ARG3) &iov, note) == -1)
>  	perror_with_name (_("Couldn't write registers"));
> -- 
> 2.36.1
> 

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

Simon

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

* Re: [PATCH 3/5] fbsd-nat: Pass an optional register base to the register set helpers.
  2022-07-08  0:58 ` [PATCH 3/5] fbsd-nat: Pass an optional register base to the register set helpers John Baldwin
@ 2022-11-22 20:38   ` Simon Marchi
  2022-11-22 22:16     ` John Baldwin
  0 siblings, 1 reply; 20+ messages in thread
From: Simon Marchi @ 2022-11-22 20:38 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

On 7/7/22 20:58, John Baldwin wrote:
> This is needed to permit using the helpers for register sets with a
> variable base.  In particular regnum needs to be converted into a
> relative register number before passed to regcache_map_supplies.
> ---
>  gdb/fbsd-nat.c | 34 +++++++++++++++++++---------------
>  gdb/fbsd-nat.h | 49 ++++++++++++++++++++++++++++---------------------
>  2 files changed, 47 insertions(+), 36 deletions(-)
> 
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 9d7383ac0ab..edcd3fc3ed1 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -1732,14 +1732,15 @@ fbsd_nat_target::supports_disable_randomization ()
>  bool
>  fbsd_nat_target::fetch_register_set (struct regcache *regcache, int regnum,
>  				     int fetch_op, const struct regset *regset,
> -				     void *regs, size_t size)
> +				     int regbase, void *regs, size_t size)
>  {
>    const struct regcache_map_entry *map
>      = (const struct regcache_map_entry *) regset->regmap;
>    pid_t pid = get_ptrace_pid (regcache->ptid ());
>  
> -  if (regnum == -1 || regcache_map_supplies (map, regnum, regcache->arch(),
> -					     size))
> +  if (regnum == -1
> +      || (regnum >= regbase && regcache_map_supplies (map, regnum - regbase,
> +						      regcache->arch(), size)))

My understanding is that it would be an internal error if the caller
passed a regnum that is < regbase.  In order words, should you do:

  gdb_assert (regnum >= regbase);

?

Actually, after reading the following patches, I think the answer is
no.  When requesting a single register, we will still go through all the
fetch functions and we expect them to do nothing if the register is not
theirs.  Leaving the question there in case others wonder about the same
thing.

I was also wondering if it would be better to do the absolute ->
relative conversion in the caller.  All this function knows is the
regset-relative view of things.  In the end it doesn't matter much, it
just moves a condition from here to the callers.  I'm fine with whatever
you prefer.

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

Simon

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

* Re: [PATCH 4/5] arm-fbsd: Use a static regset for the TLS register set.
  2022-07-08  0:58 ` [PATCH 4/5] arm-fbsd: Use a static regset for the TLS register set John Baldwin
@ 2022-11-22 20:39   ` Simon Marchi
  0 siblings, 0 replies; 20+ messages in thread
From: Simon Marchi @ 2022-11-22 20:39 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

> diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
> index 483820c1092..2a73643b763 100644
> --- a/gdb/arm-fbsd-tdep.c
> +++ b/gdb/arm-fbsd-tdep.c
> @@ -52,6 +52,12 @@ static const struct regcache_map_entry arm_fbsd_vfpregmap[] =
>      { 0 }
>    };
>  
> +static const struct regcache_map_entry arm_fbsd_tls_regmap[] =
> +  {
> +    { 1, 0, 4 },
> +    { 0 }
> +  };
I would suggest adding a comment to say that this regno is relative, and
relative to what.

> +
>  /* In a signal frame, sp points to a 'struct sigframe' which is
>     defined as:
>  
> @@ -151,6 +157,34 @@ const struct regset arm_fbsd_vfpregset =
>      regcache_supply_regset, regcache_collect_regset
>    };
>  
> +static void
> +arm_fbsd_supply_tls_regset (const struct regset *regset,
> +			    struct regcache *regcache,
> +			    int regnum, const void *buf, size_t size)
> +{
> +  struct gdbarch *gdbarch = regcache->arch ();
> +  arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);

These can now be changed to:

  arm_gdbarch_tdep *tdep = gdbarch_tdep<arm_gdbarch_tdep> (gdbarch);

With those fixed,

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

Simon


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

* Re: [PATCH 5/5] aarch64-fbsd: Use a static regset for the TLS register set.
  2022-07-08  0:58 ` [PATCH 5/5] aarch64-fbsd: " John Baldwin
@ 2022-11-22 20:40   ` Simon Marchi
  0 siblings, 0 replies; 20+ messages in thread
From: Simon Marchi @ 2022-11-22 20:40 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

On 7/7/22 20:58, John Baldwin wrote:
> This uses custom collect/supply regset handlers which pass the TLS
> register number from the gdbarch_tdep as the base register number.
> ---
>  gdb/aarch64-fbsd-nat.c  | 36 +++------------------------
>  gdb/aarch64-fbsd-tdep.c | 54 ++++++++++++++++++++++++++++-------------
>  gdb/aarch64-fbsd-tdep.h |  1 +
>  3 files changed, 42 insertions(+), 49 deletions(-)

Same comments as previous patch, after which:

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

Simon

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

* Re: [PATCH 3/5] fbsd-nat: Pass an optional register base to the register set helpers.
  2022-11-22 20:38   ` Simon Marchi
@ 2022-11-22 22:16     ` John Baldwin
  2022-11-22 22:25       ` John Baldwin
  0 siblings, 1 reply; 20+ messages in thread
From: John Baldwin @ 2022-11-22 22:16 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches

On 11/22/22 12:38 PM, Simon Marchi wrote:
> On 7/7/22 20:58, John Baldwin wrote:
>> This is needed to permit using the helpers for register sets with a
>> variable base.  In particular regnum needs to be converted into a
>> relative register number before passed to regcache_map_supplies.
>> ---
>>   gdb/fbsd-nat.c | 34 +++++++++++++++++++---------------
>>   gdb/fbsd-nat.h | 49 ++++++++++++++++++++++++++++---------------------
>>   2 files changed, 47 insertions(+), 36 deletions(-)
>>
>> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
>> index 9d7383ac0ab..edcd3fc3ed1 100644
>> --- a/gdb/fbsd-nat.c
>> +++ b/gdb/fbsd-nat.c
>> @@ -1732,14 +1732,15 @@ fbsd_nat_target::supports_disable_randomization ()
>>   bool
>>   fbsd_nat_target::fetch_register_set (struct regcache *regcache, int regnum,
>>   				     int fetch_op, const struct regset *regset,
>> -				     void *regs, size_t size)
>> +				     int regbase, void *regs, size_t size)
>>   {
>>     const struct regcache_map_entry *map
>>       = (const struct regcache_map_entry *) regset->regmap;
>>     pid_t pid = get_ptrace_pid (regcache->ptid ());
>>   
>> -  if (regnum == -1 || regcache_map_supplies (map, regnum, regcache->arch(),
>> -					     size))
>> +  if (regnum == -1
>> +      || (regnum >= regbase && regcache_map_supplies (map, regnum - regbase,
>> +						      regcache->arch(), size)))
> 
> My understanding is that it would be an internal error if the caller
> passed a regnum that is < regbase.  In order words, should you do:
> 
>    gdb_assert (regnum >= regbase);
> 
> ?
> 
> Actually, after reading the following patches, I think the answer is
> no.  When requesting a single register, we will still go through all the
> fetch functions and we expect them to do nothing if the register is not
> theirs.  Leaving the question there in case others wonder about the same
> thing.
> 
> I was also wondering if it would be better to do the absolute ->
> relative conversion in the caller.  All this function knows is the
> regset-relative view of things.  In the end it doesn't matter much, it
> just moves a condition from here to the callers.  I'm fine with whatever
> you prefer.

If you do the adjustment to an absolute value in the caller you have to still
special case for regnum == -1 when fetching all registers.  The current approach
here means that the special case for that is handled down in
regcache:transfer_regset which already has a special case for -1.

-- 
John Baldwin


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

* Re: [PATCH 3/5] fbsd-nat: Pass an optional register base to the register set helpers.
  2022-11-22 22:16     ` John Baldwin
@ 2022-11-22 22:25       ` John Baldwin
  0 siblings, 0 replies; 20+ messages in thread
From: John Baldwin @ 2022-11-22 22:25 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches

On 11/22/22 2:16 PM, John Baldwin wrote:
> On 11/22/22 12:38 PM, Simon Marchi wrote:
>> On 7/7/22 20:58, John Baldwin wrote:
>>> This is needed to permit using the helpers for register sets with a
>>> variable base.  In particular regnum needs to be converted into a
>>> relative register number before passed to regcache_map_supplies.
>>> ---
>>>    gdb/fbsd-nat.c | 34 +++++++++++++++++++---------------
>>>    gdb/fbsd-nat.h | 49 ++++++++++++++++++++++++++++---------------------
>>>    2 files changed, 47 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
>>> index 9d7383ac0ab..edcd3fc3ed1 100644
>>> --- a/gdb/fbsd-nat.c
>>> +++ b/gdb/fbsd-nat.c
>>> @@ -1732,14 +1732,15 @@ fbsd_nat_target::supports_disable_randomization ()
>>>    bool
>>>    fbsd_nat_target::fetch_register_set (struct regcache *regcache, int regnum,
>>>    				     int fetch_op, const struct regset *regset,
>>> -				     void *regs, size_t size)
>>> +				     int regbase, void *regs, size_t size)
>>>    {
>>>      const struct regcache_map_entry *map
>>>        = (const struct regcache_map_entry *) regset->regmap;
>>>      pid_t pid = get_ptrace_pid (regcache->ptid ());
>>>    
>>> -  if (regnum == -1 || regcache_map_supplies (map, regnum, regcache->arch(),
>>> -					     size))
>>> +  if (regnum == -1
>>> +      || (regnum >= regbase && regcache_map_supplies (map, regnum - regbase,
>>> +						      regcache->arch(), size)))
>>
>> My understanding is that it would be an internal error if the caller
>> passed a regnum that is < regbase.  In order words, should you do:
>>
>>     gdb_assert (regnum >= regbase);
>>
>> ?
>>
>> Actually, after reading the following patches, I think the answer is
>> no.  When requesting a single register, we will still go through all the
>> fetch functions and we expect them to do nothing if the register is not
>> theirs.  Leaving the question there in case others wonder about the same
>> thing.
>>
>> I was also wondering if it would be better to do the absolute ->
>> relative conversion in the caller.  All this function knows is the
>> regset-relative view of things.  In the end it doesn't matter much, it
>> just moves a condition from here to the callers.  I'm fine with whatever
>> you prefer.
> 
> If you do the adjustment to an absolute value in the caller you have to still
> special case for regnum == -1 when fetching all registers.  The current approach
> here means that the special case for that is handled down in
> regcache:transfer_regset which already has a special case for -1.

Ah, I think I misunderstood your point.  While I could perhaps push the
regcache_map_supplies check out to the callers, that would somewhat defeat the
point of these helpers which is to reduce duplicated code in callers.

-- 
John Baldwin


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

* Re: [PATCH 1/5] regcache: Add collect/supply_regset variants that accept a register base.
  2022-11-22 19:56   ` Simon Marchi
  2022-11-22 20:18     ` Simon Marchi
@ 2022-11-22 22:32     ` John Baldwin
  1 sibling, 0 replies; 20+ messages in thread
From: John Baldwin @ 2022-11-22 22:32 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches

On 11/22/22 11:56 AM, Simon Marchi wrote:
> On 7/7/22 20:58, John Baldwin wrote:
>> ---
>>   gdb/regcache.c | 28 +++++++++++++++++++++++++---
>>   gdb/regcache.h | 24 ++++++++++++++++++++++--
>>   2 files changed, 47 insertions(+), 5 deletions(-)
>>
>> diff --git a/gdb/regcache.c b/gdb/regcache.c
>> index 037659ef8fa..1db3d972ef8 100644
>> --- a/gdb/regcache.c
>> +++ b/gdb/regcache.c
>> @@ -1242,7 +1245,18 @@ void
>>   regcache::supply_regset (const struct regset *regset,
>>   			 int regnum, const void *buf, size_t size)
>>   {
>> -  transfer_regset (regset, this, regnum, (const gdb_byte *) buf, nullptr, size);
>> +  transfer_regset (regset, 0, this, regnum, (const gdb_byte *) buf, nullptr,
>> +		   size);
>> +}
> 
> Can the old regcache::{supply,collect}_regset (without regbase) become
> trivial wrappers around the new ones (with regbase)?  I would put the
> implementation directly in the .h if doing that.

Yes, that will work and I will make that change.
  
>> diff --git a/gdb/regcache.h b/gdb/regcache.h
>> index 1dbba5ce9af..f01127b20fb 100644
>> --- a/gdb/regcache.h
>> +++ b/gdb/regcache.h
>> @@ -150,6 +150,14 @@ extern void regcache_collect_regset (const struct regset *regset,
>>   				     int regnum, void *buf, size_t size);
>>   
>>   
>> +extern void regcache_supply_regset (const struct regset *regset, int regbase,
>> +				    struct regcache *regcache,
>> +				    int regnum, const void *buf,
>> +				    size_t size);
>> +extern void regcache_collect_regset (const struct regset *regset, int regbase,
>> +				     const struct regcache *regcache,
>> +				     int regnum, void *buf, size_t size);
> 
> These don't have a definition, I guess they are not needed.

Oops, yes.  I think I was originally going to add them for completeness,
but I think it's better to not add them so will axe the prototypes.
  
> (Could we make a patch that removes regcache_supply_regset and
> regcache_collect_regset, since they just forward to the regcache
> methods?)
> 
>> +     belonging to the regset, otherwise just the register numbered
>> +     REGNUM.  The REGSET's 'regmap' field must point to an array of
>> +     'struct regcache_map_entry'.  The valid register numbers in each
>> +     entry in 'struct regcache_map_entry' are offset by REGBASE.  */
>> +
>> +  void supply_regset (const struct regset *regset, int regbase,
>> +		      int regnum, const void *buf, size_t size);
>> +
>> +  void collect_regset (const struct regset *regset, int regbase, int regnum,
>> +		       void *buf, size_t size) const;
>> +
>>     void supply_regset (const struct regset *regset,
>>   		      int regnum, const void *buf, size_t size);
>>   
>> -
>>     void collect_regset (const struct regset *regset, int regnum,
>>   		       void *buf, size_t size) const;
> 
> Can you document the last two, just by saying something like "Same as
> the above, but with REGBASE == 0"?

Will do.

-- 
John Baldwin


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

end of thread, other threads:[~2022-11-22 22:32 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-08  0:58 [PATCH 0/5] Improve support for regcache_map_entry with variable register base John Baldwin
2022-07-08  0:58 ` [PATCH 1/5] regcache: Add collect/supply_regset variants that accept a " John Baldwin
2022-11-22 19:56   ` Simon Marchi
2022-11-22 20:18     ` Simon Marchi
2022-11-22 22:32     ` John Baldwin
2022-07-08  0:58 ` [PATCH 2/5] fbsd-nat: Use regset supply/collect methods John Baldwin
2022-11-22 20:20   ` Simon Marchi
2022-07-08  0:58 ` [PATCH 3/5] fbsd-nat: Pass an optional register base to the register set helpers John Baldwin
2022-11-22 20:38   ` Simon Marchi
2022-11-22 22:16     ` John Baldwin
2022-11-22 22:25       ` John Baldwin
2022-07-08  0:58 ` [PATCH 4/5] arm-fbsd: Use a static regset for the TLS register set John Baldwin
2022-11-22 20:39   ` Simon Marchi
2022-07-08  0:58 ` [PATCH 5/5] aarch64-fbsd: " John Baldwin
2022-11-22 20:40   ` Simon Marchi
2022-07-21 14:53 ` [PATCH 0/5] Improve support for regcache_map_entry with variable register base John Baldwin
2022-08-22 18:11   ` [PING] " John Baldwin
2022-09-20 17:50     ` John Baldwin
2022-10-20 20:26       ` [PING 4] " John Baldwin
2022-11-21 18:21         ` [PING 5] " John Baldwin

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