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

Message ID 20231124212656.96801-3-simon.marchi@efficios.com
State New
Headers
Series Fix reading and writing pseudo registers in non-current frames |

Commit Message

Simon Marchi Nov. 24, 2023, 9:26 p.m. UTC
  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
Reviewed-by: John Baldwin <jhb@FreeBSD.org>
---
 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

Aktemur, Tankut Baris Nov. 30, 2023, 11:45 a.m. UTC | #1
On Friday, November 24, 2023 10:26 PM, Simon Marchi wrote:
> Right now, gdbsupport/common-regcache.h contains two abstractons for a

Typo in "abstractons".

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

In the series posted at

  https://sourceware.org/pipermail/gdb-patches/2023-February/197487.html

I had proposed some similar changes (not for regcache_register_size, but several
other functions).  In 

  https://sourceware.org/pipermail/gdb-patches/2023-March/197724.html

Tom had said:

  I haven't read these patches, but I wanted to mention that, over time,
  we've been trying to bring gdb and gdbserver closer together where
  possible.  And, I'm wondering how this series fits into this.  At the
  end, are the two register caches more similar?  More divergent?

  I'm not necessarily saying this is the most important thing, but for
  example what would be unfortunate is if the two ended up with similar
  functionality but very different expressions, which would make the
  sharing of other code even harder.

I was going to rebase the series and send a ping.  Given that your
submission is likely to be merged earlier and based on Tom's comment,
I can wait and do the rebase later.  I'd be glad to hear if you have
comments/directions.

> Change-Id: Ia5503adb6b5509a0f4604bd2a68b4642cc5283fd
> Reviewed-by: John Baldwin <jhb@FreeBSD.org>
> ---
>  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(-)
> 
> 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);

Here, the argument to regcache_read_pc is left as self->regcache, but ...

>    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 8117d35a4d37..05538251612b 100644
> --- a/gdb/arm-linux-tdep.c
> +++ b/gdb/arm-linux-tdep.c
> @@ -903,8 +903,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);

... here the argument is changed to regcache.  It is not incorrect,
but I'm just pointing to this in case you want to keep it consistent.

> +  int is_thumb = arm_is_thumb (regcache);
>    ULONGEST svc_number = 0;
> 
>    if (is_thumb)
> @@ -914,7 +916,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 =
> @@ -937,8 +939,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 7a93b0982478..d87d8e0fc1c2 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 c0cebbb2f02d..4369ff0ef782 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 9dc354ec2b3a..7f1f07694d8a 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);

Redundant space after '('.

Thanks
-Baris

>  }
> 
>  reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo)
> @@ -417,7 +418,7 @@ get_thread_regcache (thread_info *thread)
> 
>  /* 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
> @@ -630,11 +631,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
> @@ -1314,8 +1316,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;
> @@ -1342,7 +1345,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 2e75a948a198..336b00008be5 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
>  {
> --
> 2.43.0


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  
Aktemur, Tankut Baris Nov. 30, 2023, 4:11 p.m. UTC | #2
On Friday, November 24, 2023 10:26 PM, Simon Marchi wrote:
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 2e75a948a198..336b00008be5 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);
> -
> +

Trailing space seems to have been inserted here.

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  
Simon Marchi Nov. 30, 2023, 6:01 p.m. UTC | #3
On 11/30/23 11:11, Aktemur, Tankut Baris wrote:
> On Friday, November 24, 2023 10:26 PM, Simon Marchi wrote:
>> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
>> index 2e75a948a198..336b00008be5 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);
>> -
>> +
> 
> Trailing space seems to have been inserted here.

Thanks, fixed.

Simon
  
Simon Marchi Nov. 30, 2023, 9:02 p.m. UTC | #4
On 11/30/23 06:45, Aktemur, Tankut Baris wrote:
> On Friday, November 24, 2023 10:26 PM, Simon Marchi wrote:
>> Right now, gdbsupport/common-regcache.h contains two abstractons for a
> 
> Typo in "abstractons".
> 
>> 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.
> 
> In the series posted at
> 
>   https://sourceware.org/pipermail/gdb-patches/2023-February/197487.html
> 
> I had proposed some similar changes (not for regcache_register_size, but several
> other functions).  In 
> 
>   https://sourceware.org/pipermail/gdb-patches/2023-March/197724.html

Thanks for pointing me to this, I haven't been able to keep up with
gdb-patches lately.

> Tom had said:
> 
>   I haven't read these patches, but I wanted to mention that, over time,
>   we've been trying to bring gdb and gdbserver closer together where
>   possible.  And, I'm wondering how this series fits into this.  At the
>   end, are the two register caches more similar?  More divergent?
> 
>   I'm not necessarily saying this is the most important thing, but for
>   example what would be unfortunate is if the two ended up with similar
>   functionality but very different expressions, which would make the
>   sharing of other code even harder.
> 
> I was going to rebase the series and send a ping.  Given that your
> submission is likely to be merged earlier and based on Tom's comment,
> I can wait and do the rebase later.  I'd be glad to hear if you have
> comments/directions.

I read the cover letter and commit titles, it seems nice.  I agree with
Tom for the general direction, if GDB and GDBserver are going to have
the same functionality they might as well share them.  This could then
help move some code that uses regcaches to common subdirectories.  But
sometimes the concepts are really specific to one side, I think we
should not force it if the concepts are not compatible.  But I'll have
more of an opinion when I'll read your series.  Since I have the
regcache code cache hot in my head, I can help you with this series in
the following weeks.

>> @@ -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);
> 
> Here, the argument to regcache_read_pc is left as self->regcache, but ...
> 
>>    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 8117d35a4d37..05538251612b 100644
>> --- a/gdb/arm-linux-tdep.c
>> +++ b/gdb/arm-linux-tdep.c
>> @@ -903,8 +903,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);
> 
> ... here the argument is changed to regcache.  It is not incorrect,
> but I'm just pointing to this in case you want to keep it consistent.

Thanks, I fixed arm_get_next_pcs and another instance of it in
thumb_get_next_pcs_raw (moving the declaration of `reg_buffer_common
*regcache` a bit higher).

> 
>> +  int is_thumb = arm_is_thumb (regcache);
>>    ULONGEST svc_number = 0;
>>
>>    if (is_thumb)
>> @@ -914,7 +916,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 =
>> @@ -937,8 +939,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 7a93b0982478..d87d8e0fc1c2 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 c0cebbb2f02d..4369ff0ef782 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 9dc354ec2b3a..7f1f07694d8a 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);
> 
> Redundant space after '('.

Thanks, fixed.

Simon
  

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 8117d35a4d37..05538251612b 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -903,8 +903,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)
@@ -914,7 +916,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 = 
@@ -937,8 +939,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 7a93b0982478..d87d8e0fc1c2 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 c0cebbb2f02d..4369ff0ef782 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 9dc354ec2b3a..7f1f07694d8a 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)
@@ -417,7 +418,7 @@  get_thread_regcache (thread_info *thread)
 
 /* 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
@@ -630,11 +631,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
@@ -1314,8 +1316,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;
@@ -1342,7 +1345,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 2e75a948a198..336b00008be5 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
 {