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

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

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