Remove MAX_REGISTER_SIZE from regcache.c

  Alan Hayward <Alan.Hayward@arm.com> writes:

>>> @@ -395,9 +404,9 @@ regcache_restore (struct regcache *dst,
>>> 	{
>>> 	  enum register_status status;
>> 
>> Can we move "buf" here? and initialize it with the register_size,
>> 
>>           std::vector<gdb_byte> buf (register_size (gdbarch, regnum));
>> 
>> then, we don't need max_register_size ().
>> 
>
> Problem with this is that we are then creating a brand new buffer for each
> iteration of the loop, which is a little heavyweight.
> We could create an empty buf outside the loop and re-size it each iteration,
> but that's still going to cost.
>

How is this patch below?  I class-fied regcache last month in my local
tree, and MAX_REGISTER_SIZE is disappeared from regcache.c.  I suggested
using std::vector in a loop so that it is easy to rebase my patches.
  

> On 10 Apr 2017, at 09:59, Yao Qi <qiyaoltc@gmail.com> wrote:

> 

> Alan Hayward <Alan.Hayward@arm.com> writes:

> 

>>>> @@ -395,9 +404,9 @@ regcache_restore (struct regcache *dst,

>>>> 	{

>>>> 	  enum register_status status;

>>> 

>>> Can we move "buf" here? and initialize it with the register_size,

>>> 

>>>          std::vector<gdb_byte> buf (register_size (gdbarch, regnum));

>>> 

>>> then, we don't need max_register_size ().

>>> 

>> 

>> Problem with this is that we are then creating a brand new buffer for each

>> iteration of the loop, which is a little heavyweight.

>> We could create an empty buf outside the loop and re-size it each iteration,

>> but that's still going to cost.

>> 

> 

> How is this patch below?  I class-fied regcache last month in my local

> tree, and MAX_REGISTER_SIZE is disappeared from regcache.c.  I suggested

> using std::vector in a loop so that it is easy to rebase my patches.

> 


I’m happy with this. It also simplifies the code logic a little, which is good.

This would then reduce my patch to recache_save and regcache_dump changes.

Alan.


> -- 

> Yao (齐尧)

> From 25d562b5f858314ad9d41a7ffa825d8d24e3828e Mon Sep 17 00:00:00 2001

> From: Yao Qi <yao.qi@linaro.org>

> Date: Fri, 7 Apr 2017 22:50:27 +0100

> Subject: [PATCH] Simplify regcache_restore

> 

> This patches removes the 2nd argument of regcache_restore, because it

> is only called by regcache_cpy.  In regcache_cpy, if regcache_restore

> is called, dst is not readonly, but src is readonly.  So this patch

> adds an assert that src is readonly in regcache_restore.

> regcache_cook_read read everything from a readonly regcache cache

> (src)'s register_buffer, and register status is from src->register_status.

> 

> gdb:

> 

> 2017-04-07  Yao Qi  <yao.qi@linaro.org>

> 

> 	* regcache.c (regcache_restore): Remove argument 2.  Replace

> 	argument 3 with regcache.  Get register status from

> 	src->register_status and get register contents from

> 	register_buffer (src, regnum).

> 	(regcache_cpy): Update.

> 

> diff --git a/gdb/regcache.c b/gdb/regcache.c

> index 37bc2f0..41c23a5 100644

> --- a/gdb/regcache.c

> +++ b/gdb/regcache.c

> @@ -374,17 +374,15 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,

> }

> 

> static void

> -regcache_restore (struct regcache *dst,

> -		  regcache_cooked_read_ftype *cooked_read,

> -		  void *cooked_read_context)

> +regcache_restore (struct regcache *dst, struct regcache *src)

> {

>   struct gdbarch *gdbarch = dst->descr->gdbarch;

> -  gdb_byte buf[MAX_REGISTER_SIZE];

>   int regnum;

> 

>   /* The dst had better not be read-only.  If it is, the `restore'

>      doesn't make much sense.  */

>   gdb_assert (!dst->readonly_p);

> +  gdb_assert (src->readonly_p);

>   /* Copy over any registers, being careful to only restore those that

>      were both saved and need to be restored.  The full [0 .. gdbarch_num_regs

>      + gdbarch_num_pseudo_regs) range is checked since some architectures need

> @@ -393,11 +391,8 @@ regcache_restore (struct regcache *dst,

>     {

>       if (gdbarch_register_reggroup_p (gdbarch, regnum, restore_reggroup))

> 	{

> -	  enum register_status status;

> -

> -	  status = cooked_read (cooked_read_context, regnum, buf);

> -	  if (status == REG_VALID)

> -	    regcache_cooked_write (dst, regnum, buf);

> +	  if (src->register_status[regnum] == REG_VALID)

> +	    regcache_cooked_write (dst, regnum, register_buffer (src, regnum));

> 	}

>     }

> }

> @@ -424,7 +419,7 @@ regcache_cpy (struct regcache *dst, struct regcache *src)

>   if (!src->readonly_p)

>     regcache_save (dst, do_cooked_read, src);

>   else if (!dst->readonly_p)

> -    regcache_restore (dst, do_cooked_read, src);

> +    regcache_restore (dst, src);

>   else

>     regcache_cpy_no_passthrough (dst, src);

> }
  

On Mon, Apr 10, 2017 at 9:59 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
>
> This patches removes the 2nd argument of regcache_restore, because it
> is only called by regcache_cpy.  In regcache_cpy, if regcache_restore
> is called, dst is not readonly, but src is readonly.  So this patch
> adds an assert that src is readonly in regcache_restore.
> regcache_cook_read read everything from a readonly regcache cache
> (src)'s register_buffer, and register status is from src->register_status.
>
> gdb:
>
> 2017-04-07  Yao Qi  <yao.qi@linaro.org>
>
>         * regcache.c (regcache_restore): Remove argument 2.  Replace
>         argument 3 with regcache.  Get register status from
>         src->register_status and get register contents from
>         register_buffer (src, regnum).
>         (regcache_cpy): Update.
>

Tested it again on x86_64-linux.  Pushed it in.