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

[Simon Marchi <simon.marchi@efficios.com>]

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