diff mbox series

[02/24] gdb: use reg_buffer_common throughout gdbsupport/common-regcache.h

Message ID 20231108051222.1275306-3-simon.marchi@polymtl.ca
State New
Headers
Series Fix reading and writing pseudo registers in non-current frames |

Commit Message

Simon Marchi Nov. 8, 2023, 5 a.m. UTC 
  From: Simon Marchi <simon.marchi@efficios.com>

Right now, gdbsupport/common-regcache.h contains two abstractons for a
regcache.  An opaque type `regcache` (gdb and gdbserver both have their
own regcache that is the concrete version of this) and an abstract base
class `reg_buffer_common`, that is the base of regcaches on both sides.
These abstractions allow code to be written for both gdb and gdbserver,
for instance in the gdb/arch sub-directory.

However, having two
different abstractions is impractical.  If some common code has a regcache,
and wants to use an operation defined on reg_buffer_common, it can't.
It would be better to have just one.  Change all instances of `regcache
*` in gdbsupport/common-regcache.h to be `reg_buffer_common *`, then fix
fallouts.

Implementations in gdb and gdbserver now need to down-cast (using
gdb::checked_static_cast) from reg_buffer_common to their concrete
regcache type.  Some of them could be avoided by changing free functions
(like regcache_register_size) to be virtual methods on
reg_buffer_common.  I tried it, it seems to work, but I did not include
it in this series to avoid adding unnecessary changes.

Change-Id: Ia5503adb6b5509a0f4604bd2a68b4642cc5283fd
---
 gdb/arch/arm-get-next-pcs.c   |  6 +++---
 gdb/arch/arm-get-next-pcs.h   |  5 +++--
 gdb/arch/arm.c                |  2 +-
 gdb/arch/arm.h                |  4 ++--
 gdb/arm-linux-tdep.c          | 11 ++++++-----
 gdb/arm-tdep.c                |  5 +++--
 gdb/nat/aarch64-hw-point.c    |  3 +--
 gdb/nat/linux-btrace.c        |  3 +--
 gdb/regcache.c                | 17 ++++++++++-------
 gdbserver/linux-arm-low.cc    |  4 +++-
 gdbserver/regcache.cc         | 19 ++++++++++++-------
 gdbsupport/common-regcache.cc |  2 +-
 gdbsupport/common-regcache.h  | 17 ++++++++++-------
 13 files changed, 56 insertions(+), 42 deletions(-)

Comments

John Baldwin Nov. 11, 2023, 7:42 p.m. UTC | #1 
On 11/7/23 9:00 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> Right now, gdbsupport/common-regcache.h contains two abstractons for a
> regcache.  An opaque type `regcache` (gdb and gdbserver both have their
> own regcache that is the concrete version of this) and an abstract base
> class `reg_buffer_common`, that is the base of regcaches on both sides.
> These abstractions allow code to be written for both gdb and gdbserver,
> for instance in the gdb/arch sub-directory.
> 
> However, having two
> different abstractions is impractical.  If some common code has a regcache,
> and wants to use an operation defined on reg_buffer_common, it can't.
> It would be better to have just one.  Change all instances of `regcache
> *` in gdbsupport/common-regcache.h to be `reg_buffer_common *`, then fix
> fallouts.
> 
> Implementations in gdb and gdbserver now need to down-cast (using
> gdb::checked_static_cast) from reg_buffer_common to their concrete
> regcache type.  Some of them could be avoided by changing free functions
> (like regcache_register_size) to be virtual methods on
> reg_buffer_common.  I tried it, it seems to work, but I did not include
> it in this series to avoid adding unnecessary changes.

I agree that just using reg_buffer_common in shared code is cleaner.
Adding more virtual methods to remove static_casts in a followup seems
sensible, but definitely fine to defer that to a separate followup.

Reviewed-by: John Baldwin <jhb@FreeBSD.org>
diff mbox series

Patch

diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c
index dcbb5a5e2e69..6b033993304b 100644
--- a/gdb/arch/arm-get-next-pcs.c
+++ b/gdb/arch/arm-get-next-pcs.c
@@ -32,7 +32,7 @@  arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
 		       int byte_order,
 		       int byte_order_for_code,
 		       int has_thumb2_breakpoint,
-		       struct regcache *regcache)
+		       reg_buffer_common *regcache)
 {
   self->ops = ops;
   self->byte_order = byte_order;
@@ -273,7 +273,7 @@  thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
   unsigned short inst1;
   CORE_ADDR nextpc = pc + 2;		/* Default is next instruction.  */
   ULONGEST status, itstate;
-  struct regcache *regcache = self->regcache;
+  reg_buffer_common *regcache = self->regcache;
   std::vector<CORE_ADDR> next_pcs;
 
   nextpc = MAKE_THUMB_ADDR (nextpc);
@@ -653,7 +653,7 @@  arm_get_next_pcs_raw (struct arm_get_next_pcs *self)
   unsigned long this_instr = 0;
   unsigned long status;
   CORE_ADDR nextpc;
-  struct regcache *regcache = self->regcache;
+  reg_buffer_common *regcache = self->regcache;
   CORE_ADDR pc = regcache_read_pc (self->regcache);
   std::vector<CORE_ADDR> next_pcs;
 
diff --git a/gdb/arch/arm-get-next-pcs.h b/gdb/arch/arm-get-next-pcs.h
index e6bb8d832286..ec347f01b4fd 100644
--- a/gdb/arch/arm-get-next-pcs.h
+++ b/gdb/arch/arm-get-next-pcs.h
@@ -24,6 +24,7 @@ 
 
 /* Forward declaration.  */
 struct arm_get_next_pcs;
+struct reg_buffer_common;
 
 /* get_next_pcs operations.  */
 struct arm_get_next_pcs_ops
@@ -50,7 +51,7 @@  struct arm_get_next_pcs
      not.  */
   int has_thumb2_breakpoint;
   /* Registry cache.  */
-  struct regcache *regcache;
+  reg_buffer_common *regcache;
 };
 
 /* Initialize arm_get_next_pcs.  */
@@ -59,7 +60,7 @@  void arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
 			    int byte_order,
 			    int byte_order_for_code,
 			    int has_thumb2_breakpoint,
-			    struct regcache *regcache);
+			    reg_buffer_common *regcache);
 
 /* Find the next possible PCs after the current instruction executes.  */
 std::vector<CORE_ADDR> arm_get_next_pcs (struct arm_get_next_pcs *self);
diff --git a/gdb/arch/arm.c b/gdb/arch/arm.c
index 4720c201c532..88737fc357f8 100644
--- a/gdb/arch/arm.c
+++ b/gdb/arch/arm.c
@@ -322,7 +322,7 @@  thumb2_instruction_changes_pc (unsigned short inst1, unsigned short inst2)
 /* See arm.h.  */
 
 unsigned long
-shifted_reg_val (struct regcache *regcache, unsigned long inst,
+shifted_reg_val (reg_buffer_common *regcache, unsigned long inst,
 		 int carry, unsigned long pc_val, unsigned long status_reg)
 {
   unsigned long res, shift;
diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
index c64a15600de3..b6c316191877 100644
--- a/gdb/arch/arm.h
+++ b/gdb/arch/arm.h
@@ -188,7 +188,7 @@  enum system_register_address : CORE_ADDR
   ((CORE_ADDR) (((unsigned long) (addr)) + 8 + (sbits (instr, 0, 23) << 2)))
 
 /* Forward declaration.  */
-struct regcache;
+struct reg_buffer_common;
 
 /* Return the size in bytes of the complete Thumb instruction whose
    first halfword is INST1.  */
@@ -213,7 +213,7 @@  int thumb_advance_itstate (unsigned int itstate);
 
 /* Decode shifted register value.  */
 
-unsigned long shifted_reg_val (struct regcache *regcache,
+unsigned long shifted_reg_val (reg_buffer_common *regcache,
 			       unsigned long inst,
 			       int carry,
 			       unsigned long pc_val,
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index dfa816990ff5..909a20324fd5 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -869,8 +869,10 @@  static CORE_ADDR
 arm_linux_get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
 {
   CORE_ADDR next_pc = 0;
-  CORE_ADDR pc = regcache_read_pc (self->regcache);
-  int is_thumb = arm_is_thumb (self->regcache);
+  regcache *regcache
+    = gdb::checked_static_cast<struct regcache *> (self->regcache);
+  CORE_ADDR pc = regcache_read_pc (regcache);
+  int is_thumb = arm_is_thumb (regcache);
   ULONGEST svc_number = 0;
 
   if (is_thumb)
@@ -880,7 +882,7 @@  arm_linux_get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
     }
   else
     {
-      struct gdbarch *gdbarch = self->regcache->arch ();
+      struct gdbarch *gdbarch = regcache->arch ();
       enum bfd_endian byte_order_for_code = 
 	gdbarch_byte_order_for_code (gdbarch);
       unsigned long this_instr = 
@@ -903,8 +905,7 @@  arm_linux_get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
     {
       /* SIGRETURN or RT_SIGRETURN may affect the arm thumb mode, so
 	 update IS_THUMB.   */
-      next_pc = arm_linux_sigreturn_next_pc (self->regcache, svc_number,
-					     &is_thumb);
+      next_pc = arm_linux_sigreturn_next_pc (regcache, svc_number, &is_thumb);
     }
 
   /* Addresses for calling Thumb functions have the bit 0 set.  */
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index d4047ddbb868..2018f4ed0edd 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -7255,7 +7255,8 @@  CORE_ADDR
 arm_get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self,
 				   CORE_ADDR val)
 {
-  return gdbarch_addr_bits_remove (self->regcache->arch (), val);
+  return gdbarch_addr_bits_remove
+    (gdb::checked_static_cast<regcache *> (self->regcache)->arch (), val);
 }
 
 /* Wrapper over syscall_next_pc for use in get_next_pcs.  */
@@ -7271,7 +7272,7 @@  arm_get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
 int
 arm_get_next_pcs_is_thumb (struct arm_get_next_pcs *self)
 {
-  return arm_is_thumb (self->regcache);
+  return arm_is_thumb (gdb::checked_static_cast<regcache *> (self->regcache));
 }
 
 /* single_step() is called just before we want to resume the inferior,
diff --git a/gdb/nat/aarch64-hw-point.c b/gdb/nat/aarch64-hw-point.c
index 6747e61e0265..8e9a532cd2a4 100644
--- a/gdb/nat/aarch64-hw-point.c
+++ b/gdb/nat/aarch64-hw-point.c
@@ -137,8 +137,7 @@  aarch64_point_is_aligned (ptid_t ptid, int is_watchpoint, CORE_ADDR addr,
     alignment = AARCH64_HWP_ALIGNMENT;
   else
     {
-      struct regcache *regcache
-	= get_thread_regcache_for_ptid (ptid);
+      reg_buffer_common *regcache = get_thread_regcache_for_ptid (ptid);
 
       /* Set alignment to 2 only if the current process is 32-bit,
 	 since thumb instruction can be 2-byte aligned.  Otherwise, set
diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index 3c217daa4886..7c4b40e40b8a 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -284,13 +284,12 @@  perf_event_read_bts (btrace_target_info *tinfo, const uint8_t *begin,
   struct perf_event_sample sample;
   size_t read = 0;
   struct btrace_block block = { 0, 0 };
-  struct regcache *regcache;
 
   gdb_assert (begin <= start);
   gdb_assert (start <= end);
 
   /* The first block ends at the current pc.  */
-  regcache = get_thread_regcache_for_ptid (tinfo->ptid);
+  reg_buffer_common *regcache = get_thread_regcache_for_ptid (tinfo->ptid);
   block.end = regcache_read_pc (regcache);
 
   /* The buffer may contain a partial record as its last entry (i.e. when the
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 2e48c0258091..11a6ecc3469e 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -180,9 +180,10 @@  register_size (struct gdbarch *gdbarch, int regnum)
 /* See gdbsupport/common-regcache.h.  */
 
 int
-regcache_register_size (const struct regcache *regcache, int n)
+regcache_register_size (const reg_buffer_common *regcache, int n)
 {
-  return register_size (regcache->arch (), n);
+  return register_size
+    ( gdb::checked_static_cast<const struct regcache *> (regcache)->arch (), n);
 }
 
 reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo)
@@ -435,7 +436,7 @@  get_current_regcache (void)
 
 /* See gdbsupport/common-regcache.h.  */
 
-struct regcache *
+reg_buffer_common *
 get_thread_regcache_for_ptid (ptid_t ptid)
 {
   /* This function doesn't take a process_stratum_target parameter
@@ -648,11 +649,12 @@  readable_regcache::raw_read (int regnum, T *val)
 }
 
 enum register_status
-regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
+regcache_raw_read_unsigned (reg_buffer_common *regcache, int regnum,
 			    ULONGEST *val)
 {
   gdb_assert (regcache != NULL);
-  return regcache->raw_read (regnum, val);
+  return gdb::checked_static_cast<struct regcache *> (regcache)->raw_read
+    (regnum, val);
 }
 
 void
@@ -1332,8 +1334,9 @@  reg_buffer::raw_compare (int regnum, const void *buf, int offset) const
 /* Special handling for register PC.  */
 
 CORE_ADDR
-regcache_read_pc (struct regcache *regcache)
+regcache_read_pc (reg_buffer_common *reg_buf)
 {
+  regcache *regcache = gdb::checked_static_cast<struct regcache *> (reg_buf);
   struct gdbarch *gdbarch = regcache->arch ();
 
   CORE_ADDR pc_val;
@@ -1360,7 +1363,7 @@  regcache_read_pc (struct regcache *regcache)
 /* See gdbsupport/common-regcache.h.  */
 
 CORE_ADDR
-regcache_read_pc_protected (regcache *regcache)
+regcache_read_pc_protected (reg_buffer_common *regcache)
 {
   CORE_ADDR pc;
   try
diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
index 5975b44af0ae..0a6f3622695a 100644
--- a/gdbserver/linux-arm-low.cc
+++ b/gdbserver/linux-arm-low.cc
@@ -24,6 +24,7 @@ 
 #include "linux-aarch32-low.h"
 #include "linux-aarch32-tdesc.h"
 #include "linux-arm-tdesc.h"
+#include "gdbsupport/gdb-checked-static-cast.h"
 
 #include <sys/uio.h>
 /* Don't include elf.h if linux/elf.h got included by gdb_proc_service.h.
@@ -913,7 +914,8 @@  get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
   CORE_ADDR pc = regcache_read_pc (self->regcache);
   int is_thumb = arm_is_thumb_mode ();
   ULONGEST svc_number = 0;
-  struct regcache *regcache = self->regcache;
+  regcache *regcache
+    = gdb::checked_static_cast<struct regcache *> (self->regcache);
 
   if (is_thumb)
     {
diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 0b1141662ac6..4ac0fb659c3b 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -21,6 +21,8 @@ 
 #include "gdbthread.h"
 #include "tdesc.h"
 #include "gdbsupport/rsp-low.h"
+#include "gdbsupport/gdb-checked-static-cast.h"
+
 #ifndef IN_PROCESS_AGENT
 
 struct regcache *
@@ -64,7 +66,7 @@  get_thread_regcache (struct thread_info *thread, int fetch)
 
 /* See gdbsupport/common-regcache.h.  */
 
-struct regcache *
+reg_buffer_common *
 get_thread_regcache_for_ptid (ptid_t ptid)
 {
   return get_thread_regcache (find_thread_ptid (ptid), 1);
@@ -307,9 +309,10 @@  register_size (const struct target_desc *tdesc, int n)
 /* See gdbsupport/common-regcache.h.  */
 
 int
-regcache_register_size (const struct regcache *regcache, int n)
+regcache_register_size (const reg_buffer_common *regcache, int n)
 {
-  return register_size (regcache->tdesc, n);
+  return register_size
+    (gdb::checked_static_cast<const struct regcache *> (regcache)->tdesc, n);
 }
 
 static unsigned char *
@@ -437,13 +440,14 @@  regcache::raw_collect (int n, void *buf) const
 }
 
 enum register_status
-regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
+regcache_raw_read_unsigned (reg_buffer_common *reg_buf, int regnum,
 			    ULONGEST *val)
 {
   int size;
+  regcache *regcache = gdb::checked_static_cast<struct regcache *> (reg_buf);
 
   gdb_assert (regcache != NULL);
-
+  
   size = register_size (regcache->tdesc, regnum);
 
   if (size > (int) sizeof (ULONGEST))
@@ -486,9 +490,10 @@  collect_register_by_name (struct regcache *regcache,
 /* Special handling for register PC.  */
 
 CORE_ADDR
-regcache_read_pc (struct regcache *regcache)
+regcache_read_pc (reg_buffer_common *regcache)
 {
-  return the_target->read_pc (regcache);
+  return the_target->read_pc
+    (gdb::checked_static_cast<struct regcache *> (regcache));
 }
 
 void
diff --git a/gdbsupport/common-regcache.cc b/gdbsupport/common-regcache.cc
index 3515bedb3830..b6f02bac16e2 100644
--- a/gdbsupport/common-regcache.cc
+++ b/gdbsupport/common-regcache.cc
@@ -23,7 +23,7 @@ 
 /* Return the register's value or throw if it's not available.  */
 
 ULONGEST
-regcache_raw_get_unsigned (struct regcache *regcache, int regnum)
+regcache_raw_get_unsigned (reg_buffer_common *regcache, int regnum)
 {
   ULONGEST value;
   enum register_status status;
diff --git a/gdbsupport/common-regcache.h b/gdbsupport/common-regcache.h
index e462f532407b..6d98ca8c92ed 100644
--- a/gdbsupport/common-regcache.h
+++ b/gdbsupport/common-regcache.h
@@ -20,6 +20,8 @@ 
 #ifndef COMMON_COMMON_REGCACHE_H
 #define COMMON_COMMON_REGCACHE_H
 
+struct reg_buffer_common;
+
 /* This header is a stopgap until we have an independent regcache.  */
 
 enum register_status : signed char
@@ -44,28 +46,29 @@  enum register_status : signed char
    thread specified by PTID.  This function must be provided by
    the client.  */
 
-extern struct regcache *get_thread_regcache_for_ptid (ptid_t ptid);
+extern reg_buffer_common *get_thread_regcache_for_ptid (ptid_t ptid);
 
 /* Return the size of register numbered N in REGCACHE.  This function
    must be provided by the client.  */
 
-extern int regcache_register_size (const struct regcache *regcache, int n);
+extern int regcache_register_size (const reg_buffer_common *regcache, int n);
 
 /* Read the PC register.  This function must be provided by the
    client.  */
 
-extern CORE_ADDR regcache_read_pc (struct regcache *regcache);
+extern CORE_ADDR regcache_read_pc (reg_buffer_common *regcache);
 
 /* Read the PC register.  If PC cannot be read, return 0.
    This is a wrapper around 'regcache_read_pc'.  */
 
-extern CORE_ADDR regcache_read_pc_protected (regcache *regcache);
+extern CORE_ADDR regcache_read_pc_protected (reg_buffer_common *regcache);
 
 /* Read a raw register into a unsigned integer.  */
-extern enum register_status regcache_raw_read_unsigned
-  (struct regcache *regcache, int regnum, ULONGEST *val);
+extern enum register_status
+regcache_raw_read_unsigned (reg_buffer_common *regcache, int regnum,
+			    ULONGEST *val);
 
-ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum);
+ULONGEST regcache_raw_get_unsigned (reg_buffer_common *regcache, int regnum);
 
 struct reg_buffer_common
 {