diff mbox

Removal of uses of MAX_REGISTER_SIZE

Message ID 5F3D30AE-9A53-493A-B6DC-DF594C2FAB18@arm.com
State New
Headers show

Commit Message

Alan Hayward Feb. 7, 2017, 4:33 p.m. UTC
> On 6 Feb 2017, at 15:26, Yao Qi <qiyaoltc@gmail.com> wrote:

> 

> On 17-02-06 09:33:25, Alan Hayward wrote:

>> 

>> Ok, Given that, I've put together a quick patch to:

>> * Use M68K_MAX_REGISTER_SIZE and I386_MAX_REGISTER_SIZE where possible.

> 

> They can be in a separate patch if you want.

> 

>> * Add max_register_size() function

>> * Remove MAX_REGISTER_SIZE from all common files by using std::vector.

>> Contents should be very similar to previous patches posted.

>> 

>> This patch ok?

>> 

>> 2017-02-06  Alan Hayward  <alan.hayward@arm.com\\>

>> 

>> 	* frame.c (frame_unwind_register_signed): Use std::vector.

>> 	(frame_unwind_register_unsigned): Likewise.

>> 	(get_frame_register_bytes): Likewise.

>> 	(put_frame_register_bytes): Likewise.

>> 	* i386-tdep.c (i386_pseudo_register_read_into_value): Use

>> 	I386_MAX_REGISTER_SIZE.

>> 	(i386_pseudo_register_write): Likewise.

>> 	(i386_process_record): Likewise.

>> 	* i387-tdep.c (i387_supply_xsave): Likewise.

>> 	* m68k-linux-nat.c (fetch_register): Use M68K_MAX_REGISTER_SIZE.

>> 	(store_register): Likewise.

>> 	* mi/mi-main.c (register_changed_p): Use std::vector.

>> 	* record-full.c (record_full_exec_insn): Likewise.

>> 	(record_full_core_open_1): Use max_register_size ().

>> 	(record_full_core_fetch_registers): Likewise.

>> 	(record_full_core_store_registers): Likewise.

>> 	(init_record_full_core_ops): Likewise.

>> 	* remote-sim.c (gdbsim_fetch_register): Likewise.

>> 	(gdbsim_store_register): Likewise.

>> 	* regcache.c (struct regcache_descr): Add max_register_size.

>> 	(max_register_size): New.

>> 	(init_regcache_descr): Find max register size.

>> 	(regcache_save): Use std::vector.

>> 	(regcache_restore): Likewise.

>> 	(regcache_dump): Likewise.

>> 	* regcache.h (max_register_size): New.

>> 	* remote.c (remote_prepare_to_store): Use std::vector.

>> 	* sol-thread.c (sol_thread_store_registers): Likewise.

>> 	* stack.c (frame_info): Likewise.

>> 	* target.c (debug_print_register): Likewise.

>> 

>> 

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

>> index d98003dee7c34a63bd25356e6674721664a4b2f3..1700308371d9d795545c0a8bb26dba289b18217d 100644

>> --- a/gdb/frame.c

>> +++ b/gdb/frame.c

>> @@ -1252,10 +1252,10 @@ frame_unwind_register_signed (struct frame_info *frame, int regnum)

>>   struct gdbarch *gdbarch = frame_unwind_arch (frame);

>>   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

>>   int size = register_size (gdbarch, regnum);

>> -  gdb_byte buf[MAX_REGISTER_SIZE];

>> +  std::vector<gdb_byte> buf (size);

>> 

>> -  frame_unwind_register (frame, regnum, buf);

>> -  return extract_signed_integer (buf, size, byte_order);

>> +  frame_unwind_register (frame, regnum, buf.data ());

>> +  return extract_signed_integer (buf.data (), size, byte_order);

> 

> We can replace MAX_REGISTER_SIZE with 'sizeof (LONGEST)', because

> extract_signed_integer checks 'size' shouldn't be greater than

> 'sizeof (LONGEST)'.

> 

>  if (len > (int) sizeof (LONGEST))

>    error (_("\

> That operation is not available on integers of more than %d bytes."),

> 	   (int) sizeof (LONGEST));


Ok.

> 

>> }

>> 

>> LONGEST

>> @@ -1270,10 +1270,10 @@ frame_unwind_register_unsigned (struct frame_info *frame, int regnum)

>>   struct gdbarch *gdbarch = frame_unwind_arch (frame);

>>   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

>>   int size = register_size (gdbarch, regnum);

>> -  gdb_byte buf[MAX_REGISTER_SIZE];

>> +  std::vector<gdb_byte> buf (size);

> 

> Likewise,

> replace MAX_REGISTER_SIZE with 'sizeof (ULONGEST)’


Ok.

> 

>> 

>> -  frame_unwind_register (frame, regnum, buf);

>> -  return extract_unsigned_integer (buf, size, byte_order);

>> +  frame_unwind_register (frame, regnum, buf.data ());

>> +  return extract_unsigned_integer (buf.data (), size, byte_order);

>> }

>> 

>> ULONGEST

>> @@ -1389,6 +1389,8 @@ get_frame_register_bytes (struct frame_info *frame, int regnum,

>>     error (_("Bad debug information detected: "

>> 	     "Attempt to read %d bytes from registers."), len);

>> 

>> +  std::vector<gdb_byte> buf (max_register_size (gdbarch));

>> +

>>   /* Copy the data.  */

>>   while (len > 0)

>>     {

>> @@ -1410,16 +1412,15 @@ get_frame_register_bytes (struct frame_info *frame, int regnum,

>> 	}

>>       else

>> 	{

>> -	  gdb_byte buf[MAX_REGISTER_SIZE];

>> 	  enum lval_type lval;

>> 	  CORE_ADDR addr;

>> 	  int realnum;

>> 

>> 	  frame_register (frame, regnum, optimizedp, unavailablep,

>> -			  &lval, &addr, &realnum, buf);

>> +			  &lval, &addr, &realnum, buf.data ());

> 

> I am wondering that we can replace frame_register with

> 

> value = frame_unwind_register_value (frame->next, regnum);

> 

> because frame_register -> frame_register_unwind  -> frame_unwind_register_value

> and we can get register contents from value, so we don't need 'buf' at all.


Ok.

> 

>> 	  if (*optimizedp || *unavailablep)

>> 	    return 0;

>> -	  memcpy (myaddr, buf + offset, curr_len);

>> +	  memcpy (myaddr, buf.data () + offset, curr_len);

>> 	}

>> 

>>       myaddr += curr_len;

>> @@ -1446,6 +1447,8 @@ put_frame_register_bytes (struct frame_info *frame, int regnum,

>>       regnum++;

>>     }

>> 

>> +  std::vector<gdb_byte> buf (max_register_size (gdbarch));

>> +

>>   /* Copy the data.  */

>>   while (len > 0)

>>     {

>> @@ -1460,11 +1463,9 @@ put_frame_register_bytes (struct frame_info *frame, int regnum,

>> 	}

>>       else

>> 	{

>> -	  gdb_byte buf[MAX_REGISTER_SIZE];

>> -

>> -	  deprecated_frame_register_read (frame, regnum, buf);

>> -	  memcpy (buf + offset, myaddr, curr_len);

>> -	  put_frame_register (frame, regnum, buf);

>> +	  deprecated_frame_register_read (frame, regnum, buf.data ());

>> +	  memcpy (buf.data () + offset, myaddr, curr_len);

>> +	  put_frame_register (frame, regnum, buf.data ());

> 

> Can you try using frame_unwind_register_value to read the register value?

> Get the value content, patch it, and pass it to put_frame_register.


Yes. Technically, the value content is a const, but we need to cast from void* to char* anyway.

> 

>> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c

>> index 57c23ebf5d6b2d3b398aa40ebd9b3cb70c56125c..b07335d82723f3ee9c7602a8fa284acb7e3b3a25 100644

>> --- a/gdb/mi/mi-main.c

>> +++ b/gdb/mi/mi-main.c

>> @@ -1135,8 +1135,8 @@ register_changed_p (int regnum, struct regcache *prev_regs,

>> 		    struct regcache *this_regs)

>> {

>>   struct gdbarch *gdbarch = get_regcache_arch (this_regs);

>> -  gdb_byte prev_buffer[MAX_REGISTER_SIZE];

>> -  gdb_byte this_buffer[MAX_REGISTER_SIZE];

>> +  std::vector<gdb_byte> prev_buffer (max_register_size (gdbarch));

> 

> register_size (gdbarch, regnum) instead of max_register_size?


Ok.

> 

>> +  std::vector<gdb_byte> this_buffer (max_register_size (gdbarch));

>>   enum register_status prev_status;

>>   enum register_status this_status;

>> 

>> @@ -1146,13 +1146,13 @@ register_changed_p (int regnum, struct regcache *prev_regs,

>>     return 1;

>> 

>>   /* Get register contents and compare.  */

>> -  prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer);

>> -  this_status = regcache_cooked_read (this_regs, regnum, this_buffer);

>> +  prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer.data ());

>> +  this_status = regcache_cooked_read (this_regs, regnum, this_buffer.data ());

>> 

>>   if (this_status != prev_status)

>>     return 1;

>>   else if (this_status == REG_VALID)

>> -    return memcmp (prev_buffer, this_buffer,

>> +    return memcmp (prev_buffer.data (), this_buffer.data (),

>> 		   register_size (gdbarch, regnum)) != 0;

>>   else

>>     return 0;

> 

>> diff --git a/gdb/record-full.c b/gdb/record-full.c

>> index fdd613b6e41bbfcd8644b02ccfeb98b53b518bff..a619e09a646c48c632a60407d7018979d805a08f 100644

>> --- a/gdb/record-full.c

>> +++ b/gdb/record-full.c

>> @@ -698,7 +698,7 @@ record_full_exec_insn (struct regcache *regcache,

>>     {

>>     case record_full_reg: /* reg */

>>       {

>> -        gdb_byte reg[MAX_REGISTER_SIZE];

>> +	std::vector<gdb_byte> reg (register_size (gdbarch, entry->u.reg.num));

>> 

>>         if (record_debug > 1)

>>           fprintf_unfiltered (gdb_stdlog,

>> @@ -707,10 +707,10 @@ record_full_exec_insn (struct regcache *regcache,

>>                               host_address_to_string (entry),

>>                               entry->u.reg.num);

>> 

>> -        regcache_cooked_read (regcache, entry->u.reg.num, reg);

>> -        regcache_cooked_write (regcache, entry->u.reg.num,

>> +	regcache_cooked_read (regcache, entry->u.reg.num, reg.data ());

>> +	regcache_cooked_write (regcache, entry->u.reg.num,

>> 			       record_full_get_loc (entry));

>> -        memcpy (record_full_get_loc (entry), reg, entry->u.reg.len);

>> +	memcpy (record_full_get_loc (entry), reg.data (), entry->u.reg.len);

> 

> Looks the code above copies register entry->u.reg.num contents from

> regcache to record_full_get_loc (entry), so we may rewrite the code using

> regcache_raw_collect,

> 

> regcache_raw_collect (regcache, entry->u.reg.num, record_full_get_loc (entry));

> 

> so array reg is not needed anymore.  Let me post a patch to fix it.


Looks to me like the three lines of code are swapping the contents of regcache
and record_full_get_loc.

temp <= regcache
regcache <= record_full_get_loc
record_full_get_loc <= temp

I can’t see a way of doing this without the buffer.
Left code as is in patch.


> 

>>       }

>>       break;

>> 

>> @@ -792,15 +792,17 @@ static void

>> record_full_core_open_1 (const char *name, int from_tty)

>> {

>>   struct regcache *regcache = get_current_regcache ();

>> -  int regnum = gdbarch_num_regs (get_regcache_arch (regcache));

>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);

>> +  int regnum = gdbarch_num_regs (gdbarch);

>> +  int regmaxsize = max_register_size (gdbarch);

>>   int i;

>> 

>>   /* Get record_full_core_regbuf.  */

>>   target_fetch_registers (regcache, -1);

>> -  record_full_core_regbuf = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE * regnum);

>> +  record_full_core_regbuf = (gdb_byte *) xmalloc (regmaxsize * regnum);

>>   for (i = 0; i < regnum; i ++)

>>     regcache_raw_collect (regcache, i,

>> -			  record_full_core_regbuf + MAX_REGISTER_SIZE * i);

>> +			  record_full_core_regbuf + regmaxsize * i);

>> 

>>   /* Get record_full_core_start and record_full_core_end.  */

>>   if (build_section_table (core_bfd, &record_full_core_start,

>> @@ -2038,6 +2040,9 @@ record_full_core_fetch_registers (struct target_ops *ops,

>> 				  struct regcache *regcache,

>> 				  int regno)

>> {

>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);

>> +  int regmaxsize = max_register_size (gdbarch);

>> +

>>   if (regno < 0)

>>     {

>>       int num = gdbarch_num_regs (get_regcache_arch (regcache));

>> @@ -2045,11 +2050,11 @@ record_full_core_fetch_registers (struct target_ops *ops,

>> 

>>       for (i = 0; i < num; i ++)

>>         regcache_raw_supply (regcache, i,

>> -                             record_full_core_regbuf + MAX_REGISTER_SIZE * i);

>> +			     record_full_core_regbuf + regmaxsize * i);

>>     }

>>   else

>>     regcache_raw_supply (regcache, regno,

>> -                         record_full_core_regbuf + MAX_REGISTER_SIZE * regno);

>> +			 record_full_core_regbuf + regmaxsize * regno);

>> }

>> 

>> /* "to_prepare_to_store" method for prec over corefile.  */

>> @@ -2067,9 +2072,12 @@ record_full_core_store_registers (struct target_ops *ops,

>>                              struct regcache *regcache,

>>                              int regno)

>> {

>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);

>> +  int regmaxsize = max_register_size (gdbarch);

>> +

>>   if (record_full_gdb_operation_disable)

>>     regcache_raw_collect (regcache, regno,

>> -                          record_full_core_regbuf + MAX_REGISTER_SIZE * regno);

>> +			  record_full_core_regbuf + regmaxsize * regno);

>>   else

>>     error (_("You can't do that without a process to debug."));

>> }

>> @@ -2265,7 +2273,7 @@ init_record_full_core_ops (void)

>>      record_full_reg:

>>        1 byte:  record type (record_full_reg, see enum record_full_type).

>>        8 bytes: register id (network byte order).

>> -       MAX_REGISTER_SIZE bytes: register value.

>> +       max_register_size bytes: register value.

>>      record_full_mem:

>>        1 byte:  record type (record_full_mem, see enum record_full_type).

>>        8 bytes: memory length (network byte order).

> 

> This comment is about log file format version 1, but we are using version 2, IIUC.

> We probably need to remove all the comments for version 1.

> 


Ok, removed all of Version 1 from section.

> 

>> @@ -327,7 +341,7 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,

>> 	       void *src)

>> {

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

>> -  gdb_byte buf[MAX_REGISTER_SIZE];

>> +  std::vector<gdb_byte> buf (max_register_size (gdbarch));

>>   int regnum;

>> 

>>   /* The DST should be `read-only', if it wasn't then the save would

>> @@ -346,10 +360,10 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,

>>     {

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

>> 	{

>> -	  enum register_status status = cooked_read (src, regnum, buf);

>> +	  enum register_status status = cooked_read (src, regnum, buf.data ());

>> 

>> 	  if (status == REG_VALID)

>> -	    memcpy (register_buffer (dst, regnum), buf,

>> +	    memcpy (register_buffer (dst, regnum), buf.data (),

>> 		    register_size (gdbarch, regnum));

>> 	  else

>> 	    {

> 

> Did you try removing variable 'buf'? like this,

> 

>  enum register_status status = cooked_read (src, regnum, register_buffer (dst, regnum));

> 

>  if (status != REG_VALID)

>    {

>       gdb_assert (status != REG_UNKNOWN);

> 

>      memset (register_buffer (dst, regnum), 0,

> 	      register_size (gdbarch, regnum));

>    }

> 

> I suppose function cooked_read shouldn't overflow the buffer (its third

> argument).


Ok.

> 

>> @@ -369,7 +383,7 @@ regcache_restore (struct regcache *dst,

>> 		  void *cooked_read_context)

>> {

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

>> -  gdb_byte buf[MAX_REGISTER_SIZE];

>> +  std::vector<gdb_byte> buf (max_register_size (gdbarch));

>>   int regnum;

>> 

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

>> @@ -385,9 +399,9 @@ regcache_restore (struct regcache *dst,

>> 	{

>> 	  enum register_status status;

>> 

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

>> +	  status = cooked_read (cooked_read_context, regnum, buf.data ());

>> 	  if (status == REG_VALID)

>> -	    regcache_cooked_write (dst, regnum, buf);

>> +	    regcache_cooked_write (dst, regnum, buf.data ());

>> 	}

>>     }

>> }

>> @@ -1279,7 +1293,7 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,

>>   int footnote_register_offset = 0;

>>   int footnote_register_type_name_null = 0;

>>   long register_offset = 0;

>> -  gdb_byte buf[MAX_REGISTER_SIZE];

>> +  std::vector<gdb_byte> buf (max_register_size (gdbarch));

>> 

>> #if 0

>>   fprintf_unfiltered (file, "nr_raw_registers %d\n",

>> @@ -1406,8 +1420,8 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,

>> 	    fprintf_unfiltered (file, "<unavailable>");

>> 	  else

>> 	    {

>> -	      regcache_raw_read (regcache, regnum, buf);

>> -	      print_hex_chars (file, buf,

>> +	      regcache_raw_read (regcache, regnum, buf.data ());

>> +	      print_hex_chars (file, buf.data (),

> 

> We can get register contents buffer by register_buffer (regcache, regnum),

> but we need to make sure that regcache is up-to-date.

> 

> Probably, we can add a function,

> 

> void

> regcache_raw_update (struct regcache *regcache, int regnum)

> {

>    if (!regcache->readonly_p

>      && regcache_register_status (regcache, regnum) == REG_UNKNOWN)

>    {

>      struct cleanup *old_chain = save_inferior_ptid ();

> 

>      inferior_ptid = regcache->ptid;

>      target_fetch_registers (regcache, regnum);

>      do_cleanups (old_chain);

> 

>      /* A number of targets can't access the whole set of raw

> 	 registers (because the debug API provides no means to get at

> 	 them).  */

>      if (regcache->register_status[regnum] == REG_UNKNOWN)

> 	regcache->register_status[regnum] = REG_UNAVAILABLE;

>    }

> }

> 

> in regcache_dump, we can call regcache_raw_update, and then get

> register contents from register_buffer (regcache, regnum).


Ok.

> 

>> 			       regcache->descr->sizeof_register[regnum],

>> 			       gdbarch_byte_order (gdbarch));

> 

> 

>> 	    }

>> @@ -1422,13 +1436,13 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,

>> 	    {

>> 	      enum register_status status;

>> 

>> -	      status = regcache_cooked_read (regcache, regnum, buf);

>> +	      status = regcache_cooked_read (regcache, regnum, buf.data ());

>> 	      if (status == REG_UNKNOWN)

>> 		fprintf_unfiltered (file, "<invalid>");

>> 	      else if (status == REG_UNAVAILABLE)

>> 		fprintf_unfiltered (file, "<unavailable>");

>> 	      else

>> -		print_hex_chars (file, buf,

>> +		print_hex_chars (file, buf.data (),

>> 				 regcache->descr->sizeof_register[regnum],

>> 				 gdbarch_byte_order (gdbarch));

>> 	    }

>> diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c

>> index b0c68c617e365c0ea18ac2eca525598b688ffbb5..c5b1a480c52e12af01dc2d6c959fd429735bc805 100644

>> --- a/gdb/remote-sim.c

>> +++ b/gdb/remote-sim.c

>> @@ -439,6 +439,9 @@ gdbsim_fetch_register (struct target_ops *ops,

>>       return;

>>     }

>> 

>> +  int regsize = register_size (gdbarch, regno);

>> +  std::vector<gdb_byte> buf (regsize);

>> +

>>   switch (gdbarch_register_sim_regno (gdbarch, regno))

>>     {

>>     case LEGACY_SIM_REGNO_IGNORE:

>> @@ -447,10 +450,9 @@ gdbsim_fetch_register (struct target_ops *ops,

>>       {

>> 	/* For moment treat a `does not exist' register the same way

>> 	   as an ``unavailable'' register.  */

>> -	gdb_byte buf[MAX_REGISTER_SIZE];

>> 	int nr_bytes;

>> 

>> -	memset (buf, 0, MAX_REGISTER_SIZE);

>> +	memset (buf, 0, regsize);

> 

> memset to std::vector?


Doh!

> 

>> 	regcache_raw_supply (regcache, regno, buf);

>> 	break;

>>       }

>> @@ -458,18 +460,17 @@ gdbsim_fetch_register (struct target_ops *ops,

>>     default:

>>       {

>> 	static int warn_user = 1;

>> -	gdb_byte buf[MAX_REGISTER_SIZE];

>> 	int nr_bytes;

>> 

>> 	gdb_assert (regno >= 0 && regno < gdbarch_num_regs (gdbarch));

>> -	memset (buf, 0, MAX_REGISTER_SIZE);

>> +	memset (buf, 0, regsize);

>> 	nr_bytes = sim_fetch_register (sim_data->gdbsim_desc,

>> 				       gdbarch_register_sim_regno

>> 					 (gdbarch, regno),

>> 				       buf,

> 

> 'buf' is a std::vector, so how does it compile?


Looks like remote-sim.c doesn’t get complied in if you don’t specify a sim at config time.
Hacking the makefile, I can get this to build, but I don’t have a sim, so can’t link the code.
Fixed up.


> 

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

>> index c4cec910c44cf91cc7f36b7f2d87cde3f46de41e..157a1b1789d2a248c11dcc4efebd8ce54da82045 100644

>> --- a/gdb/remote.c

>> +++ b/gdb/remote.c

>> @@ -7757,9 +7757,10 @@ remote_fetch_registers (struct target_ops *ops,

>> static void

>> remote_prepare_to_store (struct target_ops *self, struct regcache *regcache)

>> {

>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);

>>   struct remote_arch_state *rsa = get_remote_arch_state ();

>>   int i;

>> -  gdb_byte buf[MAX_REGISTER_SIZE];

>> +  std::vector<gdb_byte> buf (max_register_size (gdbarch));

>> 

>>   /* Make sure the entire registers array is valid.  */

>>   switch (packet_support (PACKET_P))

>> @@ -7769,7 +7770,7 @@ remote_prepare_to_store (struct target_ops *self, struct regcache *regcache)

>>       /* Make sure all the necessary registers are cached.  */

>>       for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++)

>> 	if (rsa->regs[i].in_g_packet)

>> -	  regcache_raw_read (regcache, rsa->regs[i].regnum, buf);

>> +	  regcache_raw_read (regcache, rsa->regs[i].regnum, buf.data ());

> 

> Use regcache_raw_update.


Ok.

> 

>>       break;

>>     case PACKET_ENABLE:

>>       break;

>> diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c

>> index a09a3ab9a8bc56f367e3ba1537f5674f0a7f491f..5e7443a97624fbeae5b65cb534401e6371755423 100644

>> --- a/gdb/sol-thread.c

>> +++ b/gdb/sol-thread.c

>> @@ -514,6 +514,7 @@ sol_thread_store_registers (struct target_ops *ops,

>>   td_err_e val;

>>   prgregset_t gregset;

>>   prfpregset_t fpregset;

>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);

>> 

>>   if (!ptid_tid_p (inferior_ptid))

>>     {

>> @@ -535,10 +536,10 @@ sol_thread_store_registers (struct target_ops *ops,

>>   if (regnum != -1)

>>     {

>>       /* Not writing all the registers.  */

>> -      char old_value[MAX_REGISTER_SIZE];

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

>> 

>>       /* Save new register value.  */

>> -      regcache_raw_collect (regcache, regnum, old_value);

>> +      regcache_raw_collect (regcache, regnum, old_value.data ());

>> 

>>       val = p_td_thr_getgregs (&thandle, gregset);

>>       if (val != TD_OK)

>> @@ -550,7 +551,7 @@ sol_thread_store_registers (struct target_ops *ops,

>> 	       td_err_string (val));

>> 

>>       /* Restore new register value.  */

>> -      regcache_raw_supply (regcache, regnum, old_value);

>> +      regcache_raw_supply (regcache, regnum, old_value.data ());

> 

> I don't understand why we collect all registers and restore them later.

> Will take a deeper look.


Looks like the collect and supply are entirely redundant!
p_td_thr_getgregs and p_td_thr_getfpregs are simple writing into the newly created
gregset/fpregset

The function should be doing:

* If regnum is a real register number then: read all registers from OS into gregset/fpregset,
  copy regnum from recache into struct, write struct back to OS
* If regnum is -1 then: copy all registers from regcache into gregset/fpregset, write struct to OS.
 
I’ll remove the bogus code.

> 

>>     }

>> 

>>   fill_gregset (regcache, (gdb_gregset_t *) &gregset, regnum);

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

>> index e00e2972cf20bc63917af19f86bf57f1c6b0b5b0..9b84435fe73b9af9b13f01c819f77f83bbb7117b 100644

>> --- a/gdb/stack.c

>> +++ b/gdb/stack.c

>> @@ -1667,16 +1667,16 @@ frame_info (char *addr_exp, int from_tty)

>> 	  {

>> 	    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

>> 	    int sp_size = register_size (gdbarch, gdbarch_sp_regnum (gdbarch));

>> -	    gdb_byte value[MAX_REGISTER_SIZE];

>> +	    std::vector<gdb_byte> value (sp_size);

>> 	    CORE_ADDR sp;

>> 

>> 	    frame_register_unwind (fi, gdbarch_sp_regnum (gdbarch),

>> 				   &optimized, &unavailable, &lval, &addr,

>> -				   &realnum, value);

>> +				   &realnum, value.data ());

> 

> We can use frame_unwind_register_value.

> 


Ok.

>> 	    /* NOTE: cagney/2003-05-22: This is assuming that the

>>                stack pointer was packed as an unsigned integer.  That

>>                may or may not be valid.  */

>> -	    sp = extract_unsigned_integer (value, sp_size, byte_order);

>> +	    sp = extract_unsigned_integer (value.data (), sp_size, byte_order);

>> 	    printf_filtered (" Previous frame's sp is ");

>> 	    fputs_filtered (paddress (gdbarch, sp), gdb_stdout);

>> 	    printf_filtered ("\n");

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

>> index 3c409f0f619141205dfdcbbf8e46a277585ed683..52bb59255fcbdc74a674e08f6e168be5b6294819 100644

>> --- a/gdb/target.c

>> +++ b/gdb/target.c

>> @@ -3565,17 +3565,18 @@ debug_print_register (const char * func,

>>     {

>>       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

>>       int i, size = register_size (gdbarch, regno);

>> -      gdb_byte buf[MAX_REGISTER_SIZE];

>> +      std::vector<gdb_byte> buf (size);

>> 

>> -      regcache_raw_collect (regcache, regno, buf);

>> +      regcache_raw_collect (regcache, regno, buf.data ());

>>       fprintf_unfiltered (gdb_stdlog, " = ");

>>       for (i = 0; i < size; i++)

>> 	{

>> -	  fprintf_unfiltered (gdb_stdlog, "%02x", buf[i]);

>> +	  fprintf_unfiltered (gdb_stdlog, "%02x", buf.data ()[i]);

>> 	}

>>       if (size <= sizeof (LONGEST))

>> 	{

>> -	  ULONGEST val = extract_unsigned_integer (buf, size, byte_order);

>> +	  ULONGEST val = extract_unsigned_integer (buf.data (), size,

>> +						   byte_order);

>> 

>> 	  fprintf_unfiltered (gdb_stdlog, " %s %s",

>> 			      core_addr_to_string_nz (val), plongest (val));

>> 

>> 

> 

> We can move debug_print_register to regcache.c, so that we can access

> register_buffer, and don't need 'buf' at all.

> 


Ok.

> -- 

> Yao (齐尧)



New version of the patch with all the changes above.

Alan.


2017-02-07  Alan Hayward  <alan.hayward@arm.com>

	* frame.c (frame_unwind_register_signed): Use LONGEST for size.
	(frame_unwind_register_unsigned): Use ULONGEST for size.
	(get_frame_register_bytes): Unwind register.
	(put_frame_register_bytes): Likewise.
	* i386-tdep.c (i386_pseudo_register_read_into_value): Use
	I386_MAX_REGISTER_SIZE.
	(i386_pseudo_register_write): Likewise.
	(i386_process_record): Likewise.
	* i387-tdep.c (i387_supply_xsave): Likewise.
	* m68k-linux-nat.c (fetch_register): Use M68K_MAX_REGISTER_SIZE.
	(store_register): Likewise.
	* mi/mi-main.c (register_changed_p): Use std::vector.
	* record-full.c (record_full_exec_insn): Likewise.
	(record_full_core_open_1): Use max_register_size ().
	(record_full_core_fetch_registers): Likewise.
	(record_full_core_store_registers): Likewise.
	(init_record_full_core_ops): Likewise.
	* remote-sim.c (gdbsim_fetch_register): Likewise.
	(gdbsim_store_register): Likewise.
	* regcache.c (struct regcache_descr): Add max_register_size.
	(max_register_size): New.
	(init_regcache_descr): Find max register size.
	(regcache_save): Use std::vector.
	(regcache_restore): Likewise.
	(regcache_raw_update): New function.
	(regcache_raw_read): Move checks to regcache_raw_update.
	(regcache_debug_print_register): New function.
	(regcache_dump): Call regcache_raw_update. Use std::vector.
	* regcache.h (max_register_size): New.
	(regcache_raw_update): New declaration.
	(regcache_debug_print_register): New declaration.
	* remote.c (remote_prepare_to_store): Call regcache_raw_update.
	* sol-thread.c (sol_thread_store_registers): Remove superfluous code.
	* stack.c (frame_info): Unwind sp register.
	* target.c (debug_print_register): Move to regcache.c
	(target_fetch_registers): Call regcache_debug_print_register.
	(target_store_registers): Likewise.

Comments

Yao Qi Feb. 8, 2017, 10:47 a.m. UTC | #1
On 17-02-07 16:33:19, Alan Hayward wrote:

Hi Alan,
We end up having multiple different ways removing MAX_REGISTER_SIZE, and
each change is quite independent.  I'll split it in my review, and you can
to post a patch set in the next version.

> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 8a4d59f6fdae8ec785462d0ceedcd6501b955cf0..081a16c6896ce7aee4db3b0be45fbbdd2c23dbdb 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -3250,7 +3250,7 @@ i386_pseudo_register_read_into_value (struct gdbarch *gdbarch,
>  				      int regnum,
>  				      struct value *result_value)
>  {
> -  gdb_byte raw_buf[MAX_REGISTER_SIZE];
> +  gdb_byte raw_buf[I386_MAX_REGISTER_SIZE];
>    enum register_status status;
>    gdb_byte *buf = value_contents_raw (result_value);
> 
> @@ -3455,7 +3455,7 @@ void
>  i386_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
>  			    int regnum, const gdb_byte *buf)
>  {
> -  gdb_byte raw_buf[MAX_REGISTER_SIZE];
> +  gdb_byte raw_buf[I386_MAX_REGISTER_SIZE];
> 
>    if (i386_mmx_regnum_p (gdbarch, regnum))
>      {
> @@ -5037,7 +5037,7 @@ i386_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
>    uint32_t opcode;
>    uint8_t opcode8;
>    ULONGEST addr;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> +  gdb_byte buf[I386_MAX_REGISTER_SIZE];
>    struct i386_record_s ir;
>    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>    uint8_t rex_w = -1;
> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
> index adbe72133089bc371108d5dd79bf8d8e61ba259c..fcd5ad248d6b737b9f27e294ce166a118e4bdcad 100644
> --- a/gdb/i387-tdep.c
> +++ b/gdb/i387-tdep.c
> @@ -899,7 +899,7 @@ i387_supply_xsave (struct regcache *regcache, int regnum,
>    const gdb_byte *regs = (const gdb_byte *) xsave;
>    int i;
>    unsigned int clear_bv;
> -  static const gdb_byte zero[MAX_REGISTER_SIZE] = { 0 };
> +  static const gdb_byte zero[I386_MAX_REGISTER_SIZE] = { 0 };
>    enum
>      {
>        none = 0x0,
> diff --git a/gdb/m68k-linux-nat.c b/gdb/m68k-linux-nat.c
> index 6944c74eb198381135fda3ddf01b9da3a63e62d5..e5182caf39197f759c85c2321e4d66c428f5911e 100644
> --- a/gdb/m68k-linux-nat.c
> +++ b/gdb/m68k-linux-nat.c
> @@ -105,7 +105,7 @@ fetch_register (struct regcache *regcache, int regno)
>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    long regaddr, val;
>    int i;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> +  gdb_byte buf[M68K_MAX_REGISTER_SIZE];

Nit, we can even reduce the size of 'buf' to sizeof (long), because the
code read/write register by PTRACE_PEEKUSER/PTRACE_POKEUSER which is
word-wide operation.

>    int tid;
> 
>    /* Overload thread id onto process id.  */
> @@ -160,7 +160,7 @@ store_register (const struct regcache *regcache, int regno)
>    long regaddr, val;
>    int i;
>    int tid;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> +  gdb_byte buf[M68K_MAX_REGISTER_SIZE];
> 
>    /* Overload thread id onto process id.  */
>    tid = ptid_get_lwp (inferior_ptid);

This part is OK.
Yao Qi Feb. 8, 2017, 12:06 p.m. UTC | #2
On Tue, Feb 7, 2017 at 4:33 PM, Alan Hayward <Alan.Hayward@arm.com> wrote:

> diff --git a/gdb/stack.c b/gdb/stack.c
> index e00e2972cf20bc63917af19f86bf57f1c6b0b5b0..7ba7d68bde8d83ea1e700faa466c6951979e0f76 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -1650,33 +1650,35 @@ frame_info (char *addr_exp, int from_tty)
>      int count;
>      int i;
>      int need_nl = 1;
> +    int sp_regnum = gdbarch_sp_regnum (gdbarch);
>
>      /* The sp is special; what's displayed isn't the save address, but
>         the value of the previous frame's sp.  This is a legacy thing,
>         at one stage the frame cached the previous frame's SP instead
>         of its address, hence it was easiest to just display the cached
>         value.  */
> -    if (gdbarch_sp_regnum (gdbarch) >= 0)
> +    if (sp_regnum >= 0)
>        {
>         /* Find out the location of the saved stack pointer with out
>             actually evaluating it.  */
> -       frame_register_unwind (fi, gdbarch_sp_regnum (gdbarch),
> -                              &optimized, &unavailable, &lval, &addr,
> -                              &realnum, NULL);
> +       frame_register_unwind (fi, sp_regnum, &optimized, &unavailable, &lval,
> +                              &addr, &realnum, NULL);
>         if (!optimized && !unavailable && lval == not_lval)
>           {
>             enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -           int sp_size = register_size (gdbarch, gdbarch_sp_regnum (gdbarch));
> -           gdb_byte value[MAX_REGISTER_SIZE];
> +           int sp_size = register_size (gdbarch, sp_regnum);
>             CORE_ADDR sp;
> +           struct value *value = frame_unwind_register_value (fi, sp_regnum);
>

Why don't you hoist frame_unwind_register_value above?, so the
frame_register_unwind call is no longer needed,

  struct value *value = frame_unwind_register_value (fi, sp_regnum);

  gdb_assert (value != NULL);

  if (!value_optimized_out (value) && value_entirely_available (value))
     {
       if (VALUE_LVAL (value) == not_lval)
         {
            sp = extract_unsigned_integer (value_contents_all (value),
                                          sp_size, byte_order);
         }
       else if (VALUE_LVAL (value) == lval_memory)
         {
            // use value_address (value);
         }
       else if (VALUE_LVAL (value) == lval_register)
         {
            // use VALUE_REGNUM (value);
         }
     }
   /* else keep quiet.  */

   release_value (value);
   value_free (value);

> -           frame_register_unwind (fi, gdbarch_sp_regnum (gdbarch),
> -                                  &optimized, &unavailable, &lval, &addr,
> -                                  &realnum, value);
> +           gdb_assert (value != NULL);
>             /* NOTE: cagney/2003-05-22: This is assuming that the
>                 stack pointer was packed as an unsigned integer.  That
>                 may or may not be valid.  */
> -           sp = extract_unsigned_integer (value, sp_size, byte_order);
> +           sp = extract_unsigned_integer (value_contents_all (value), sp_size,
> +                                          byte_order);
> +           release_value (value);
> +           value_free (value);
> +
>             printf_filtered (" Previous frame's sp is ");
>             fputs_filtered (paddress (gdbarch, sp), gdb_stdout);
>             printf_filtered ("\n");
Yao Qi Feb. 8, 2017, 12:24 p.m. UTC | #3
Alan Hayward <Alan.Hayward@arm.com> writes:

> diff --git a/gdb/stack.c b/gdb/stack.c
> index e00e2972cf20bc63917af19f86bf57f1c6b0b5b0..7ba7d68bde8d83ea1e700faa466c6951979e0f76 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -1650,33 +1650,35 @@ frame_info (char *addr_exp, int from_tty)
>      int count;
>      int i;
>      int need_nl = 1;
> +    int sp_regnum = gdbarch_sp_regnum (gdbarch);
>
>      /* The sp is special; what's displayed isn't the save address, but
>         the value of the previous frame's sp.  This is a legacy thing,
>         at one stage the frame cached the previous frame's SP instead
>         of its address, hence it was easiest to just display the cached
>         value.  */
> -    if (gdbarch_sp_regnum (gdbarch) >= 0)
> +    if (sp_regnum >= 0)
>        {
>  	/* Find out the location of the saved stack pointer with out
>             actually evaluating it.  */
> -	frame_register_unwind (fi, gdbarch_sp_regnum (gdbarch),
> -			       &optimized, &unavailable, &lval, &addr,
> -			       &realnum, NULL);
> +	frame_register_unwind (fi, sp_regnum, &optimized, &unavailable, &lval,
> +			       &addr, &realnum, NULL);
>  	if (!optimized && !unavailable && lval == not_lval)
>  	  {
>  	    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -	    int sp_size = register_size (gdbarch, gdbarch_sp_regnum (gdbarch));
> -	    gdb_byte value[MAX_REGISTER_SIZE];
> +	    int sp_size = register_size (gdbarch, sp_regnum);
>  	    CORE_ADDR sp;
> +	    struct value *value = frame_unwind_register_value (fi, sp_regnum);
>
> -	    frame_register_unwind (fi, gdbarch_sp_regnum (gdbarch),
> -				   &optimized, &unavailable, &lval, &addr,
> -				   &realnum, value);
> +	    gdb_assert (value != NULL);

Why don't you hoist frame_unwind_register_value above?, so the
frame_register_unwind call is no longer needed,


  struct value *value = frame_unwind_register_value (fi, sp_regnum);

  gdb_assert (value != NULL);

  if (!value_optimized_out (value) && value_entirely_available (value))
     {
       if (VALUE_LVAL (value) == not_lval)
         {
            sp = extract_unsigned_integer (value_contents_all (value),
                                          sp_size, byte_order);
         }
       else if (VALUE_LVAL (value) == lval_memory)
         {
            // use value_address (value);
         }
       else if (VALUE_LVAL (value) == lval_register)
         {
            // use VALUE_REGNUM (value);
         }
     }
   /* else keep quiet.  */

   release_value (value);
   value_free (value);

>  	    /* NOTE: cagney/2003-05-22: This is assuming that the
>                 stack pointer was packed as an unsigned integer.  That
>                 may or may not be valid.  */
> -	    sp = extract_unsigned_integer (value, sp_size, byte_order);
> +	    sp = extract_unsigned_integer (value_contents_all (value), sp_size,
> +					   byte_order);
> +	    release_value (value);
> +	    value_free (value);
> +
>  	    printf_filtered (" Previous frame's sp is ");
>  	    fputs_filtered (paddress (gdbarch, sp), gdb_stdout);
>  	    printf_filtered ("\n");
> @@ -1702,7 +1704,7 @@ frame_info (char *addr_exp, int from_tty)
>      numregs = gdbarch_num_regs (gdbarch)
>  	      + gdbarch_num_pseudo_regs (gdbarch);
>      for (i = 0; i < numregs; i++)
> -      if (i != gdbarch_sp_regnum (gdbarch)
> +      if (i != sp_regnum
>  	  && gdbarch_register_reggroup_p (gdbarch, i, all_reggroup))
>  	{
>  	  /* Find out the location of the saved register without
Yao Qi Feb. 8, 2017, 3:31 p.m. UTC | #4
Alan Hayward <Alan.Hayward@arm.com> writes:

> +/* Make certain that the register cache is up-to-date with respect to the
> +   current thread.  */

Only register REGNUM is up-to-date.  REGCACHE has ptid, which may not be
the current thread.

/* Make certain that the register REGNUM in REGCACHE is up-to-date.  */

> +void regcache_raw_update (struct regcache *regcache, int regnum);
> +
> +enum register_status regcache_raw_read (struct regcache *regcache,
> +					int rawnum, gdb_byte *buf);

We've already had the declaration.

>  /* Transfer a raw register [0..NUM_REGS) between core-gdb and the
>     regcache.  The read variants return the status of the register.  */
Yao Qi Feb. 8, 2017, 5:09 p.m. UTC | #5
Alan Hayward <Alan.Hayward@arm.com> writes:

> @@ -1135,8 +1135,8 @@ register_changed_p (int regnum, struct regcache *prev_regs,
>  		    struct regcache *this_regs)
>  {
>    struct gdbarch *gdbarch = get_regcache_arch (this_regs);
> -  gdb_byte prev_buffer[MAX_REGISTER_SIZE];
> -  gdb_byte this_buffer[MAX_REGISTER_SIZE];
> +  std::vector<gdb_byte> prev_buffer (register_size (gdbarch, regnum));
> +  std::vector<gdb_byte> this_buffer (register_size (gdbarch, regnum));
>    enum register_status prev_status;
>    enum register_status this_status;
>

This function should be moved to regcache.c, because it is about
comparing bytes of a certain register in both regcaches.  Then, wen can
compare raw registers from register_buffer, and pseudo registers from
the values.

> @@ -1146,13 +1146,13 @@ register_changed_p (int regnum, struct regcache *prev_regs,
>      return 1;
>
>    /* Get register contents and compare.  */
> -  prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer);
> -  this_status = regcache_cooked_read (this_regs, regnum, this_buffer);
> +  prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer.data ());
> +  this_status = regcache_cooked_read (this_regs, regnum, this_buffer.data ());
>
>    if (this_status != prev_status)
>      return 1;
>    else if (this_status == REG_VALID)
> -    return memcmp (prev_buffer, this_buffer,
> +    return memcmp (prev_buffer.data (), this_buffer.data (),
>  		   register_size (gdbarch, regnum)) != 0;
>    else
>      return 0;
Yao Qi Feb. 8, 2017, 5:36 p.m. UTC | #6
Alan Hayward <Alan.Hayward@arm.com> writes:

> @@ -1279,7 +1335,7 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
>    int footnote_register_offset = 0;
>    int footnote_register_type_name_null = 0;
>    long register_offset = 0;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> +  std::vector<gdb_byte> buf (max_register_size (gdbarch));
>
>  #if 0
>    fprintf_unfiltered (file, "nr_raw_registers %d\n",
> @@ -1406,8 +1462,8 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
>  	    fprintf_unfiltered (file, "<unavailable>");
>  	  else
>  	    {
> -	      regcache_raw_read (regcache, regnum, buf);
> -	      print_hex_chars (file, buf,
> +	      regcache_raw_update (regcache, regnum);
> +	      print_hex_chars (file, register_buffer (regcache, regnum),
>  			       regcache->descr->sizeof_register[regnum],
>  			       gdbarch_byte_order (gdbarch));
>  	    }
> @@ -1422,13 +1478,13 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
>  	    {
>  	      enum register_status status;
>
> -	      status = regcache_cooked_read (regcache, regnum, buf);
> +	      status = regcache_cooked_read (regcache, regnum, buf.data ());

Can we use regcache_cooked_read_value so that we don't need buf at all.

>  	      if (status == REG_UNKNOWN)
>  		fprintf_unfiltered (file, "<invalid>");
>  	      else if (status == REG_UNAVAILABLE)
>  		fprintf_unfiltered (file, "<unavailable>");
>  	      else
> -		print_hex_chars (file, buf,
> +		print_hex_chars (file, buf.data (),
>  				 regcache->descr->sizeof_register[regnum],
>  				 gdbarch_byte_order (gdbarch));
>  	    }
Alan Hayward Feb. 9, 2017, 1:26 p.m. UTC | #7
> On 8 Feb 2017, at 17:09, Yao Qi <qiyaoltc@gmail.com> wrote:

> 

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

> 

>> @@ -1135,8 +1135,8 @@ register_changed_p (int regnum, struct regcache *prev_regs,

>> 		    struct regcache *this_regs)

>> {

>>   struct gdbarch *gdbarch = get_regcache_arch (this_regs);

>> -  gdb_byte prev_buffer[MAX_REGISTER_SIZE];

>> -  gdb_byte this_buffer[MAX_REGISTER_SIZE];

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

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

>>   enum register_status prev_status;

>>   enum register_status this_status;

>> 

> 

> This function should be moved to regcache.c, because it is about

> comparing bytes of a certain register in both regcaches.  Then, wen can

> compare raw registers from register_buffer, and pseudo registers from

> the values.


Trying to remove the buffer results in quite a lot of code, as you can’t assume
the register has the same state in both prev_regs and this_regs.
For pseudo cases, it will still result in buffer usage.
(Attempted code pasted below).

I feel this is adding a whole lot of complexity and mostly duplicates code from
regcache_cooked_read.

Unless you really feel this case is going to hit performance, then I think the
existing code is much simpler and easier to read.

(Happy to move the function to regcache and change result to a bool).



Maybe a long term solution would be an alternative additional version of
regcache_cooked_read:

enum register_status
regcache_cooked_read (struct regcache *regcache, int regnum, gdb_byte **buf, 
		      bool *requires_deallocation)

The function returns you a ptr in **buf. If *requires_deallocation is true then you
need to free buf once you have finished with it.

Or maybe a version of regcache_cooked_read which returns a struct *value
(assuming it’s possible to set the buffer in a struct value to be an existing pointer).

This could then be used throughout the codebase, preventing lots of small memcpy.
However, that would then open a rabbit hole of changes to common functions, which is
all far out of the scope of these patches.

It’s possible that all that is not possible because we don’t want to expose the
internal regcache pointers in case the values move.



This was my work in progress:


bool
regcache_register_changed_p (int regnum, struct regcache *prev_regs,
			     struct regcache *this_regs)
{
  struct gdbarch *this_gdbarch = get_regcache_arch (this_regs);
  struct gdbarch *prev_gdbarch;
  gdb_byte *prev_buffer = NULL;
  gdb_byte *this_buffer = NULL;
  enum register_status prev_status;
  enum register_status this_status;
  gdb_assert (regnum >= 0);
  gdb_assert (regnum < this_regs->descr->nr_cooked_registers);

  /* if there are no previous registers consider all registers as changed.  */
  if (!prev_regs)
    return true;
  gdb_assert (regnum < regcache->descr->nr_cooked_registers);

  prev_gdbarch = get_regcache_arch (prev_regs);

  /* If arches don't match then consider all registers as changed.  */
  if (prev_gdbarch != gdbarch)
    return true;

  if (regnum < regcache->descr->nr_raw_registers)
  {
    prev_status = prev_regs->register_status[regnum];
    this_status = this_status->register_status[regnum];
    prev_buffer = register_buffer (prev_regs, regnum);
    this_buffer = register_buffer (this_regs, regnum);
  }
  else
  {
    if (prev_regs->readonly_p
	&& prev_regs->register_status[regnum] != REG_UNKNOWN)
      {
      	prev_status = prev_regs->register_status[regnum];
      	prev_buffer = register_buffer (prev_regs, regnum);
      }
     else if (gdbarch_pseudo_register_read_value_p (prev_regs->descr->gdbarch))
      {
	struct value *mark, *computed;

	mark = value_mark ();

	computed = gdbarch_pseudo_register_read_value (prev_gdbarch,
						       prev_regs, regnum);
	if (value_entirely_available (computed))
	  {
	    prev_buffer = value_contents_raw (computed);
	    prev_status = REG_VALID;
	  }
	else
	  prev_status = REG_UNAVAILABLE;
	}
      }
     else
      {
      	prev_buffer = xmalloc (register_size (prev_gdbarch, regnum));
      	gdbarch_pseudo_register_read (prev_gdbarch, prev_regcache, regnum, prev_buffer);
      }

      ....duplicate above code for this_regcache....
      Possibly put above code into a static function (but then you still have to deal
      with allocation and frees spanning the functions)

  }


  if (this_status != prev_status)
    ret = true;
  else if (this_status == REG_VALID)
    ret = memcmp (prev_buffer, this_buffer,
		  register_size (gdbarch, regnum)) != 0;
  else
    ret = false;

  ...code to free any buf or mark for created for either prev and this...

  return ret;
}




> 

>> @@ -1146,13 +1146,13 @@ register_changed_p (int regnum, struct regcache *prev_regs,

>>     return 1;

>> 

>>   /* Get register contents and compare.  */

>> -  prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer);

>> -  this_status = regcache_cooked_read (this_regs, regnum, this_buffer);

>> +  prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer.data ());

>> +  this_status = regcache_cooked_read (this_regs, regnum, this_buffer.data ());

>> 

>>   if (this_status != prev_status)

>>     return 1;

>>   else if (this_status == REG_VALID)

>> -    return memcmp (prev_buffer, this_buffer,

>> +    return memcmp (prev_buffer.data (), this_buffer.data (),

>> 		   register_size (gdbarch, regnum)) != 0;

>>   else

>>     return 0;

> 

> -- 

> Yao (齐尧)
Alan Hayward Feb. 13, 2017, 11:59 a.m. UTC | #8
> On 8 Feb 2017, at 17:36, Yao Qi <qiyaoltc@gmail.com> wrote:

> 

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

> 

>> @@ -1279,7 +1335,7 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,

>>   int footnote_register_offset = 0;

>>   int footnote_register_type_name_null = 0;

>>   long register_offset = 0;

>> -  gdb_byte buf[MAX_REGISTER_SIZE];

>> +  std::vector<gdb_byte> buf (max_register_size (gdbarch));

>> 

>> #if 0

>>   fprintf_unfiltered (file, "nr_raw_registers %d\n",

>> @@ -1406,8 +1462,8 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,

>> 	    fprintf_unfiltered (file, "<unavailable>");

>> 	  else

>> 	    {

>> -	      regcache_raw_read (regcache, regnum, buf);

>> -	      print_hex_chars (file, buf,

>> +	      regcache_raw_update (regcache, regnum);

>> +	      print_hex_chars (file, register_buffer (regcache, regnum),

>> 			       regcache->descr->sizeof_register[regnum],

>> 			       gdbarch_byte_order (gdbarch));

>> 	    }

>> @@ -1422,13 +1478,13 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,

>> 	    {

>> 	      enum register_status status;

>> 

>> -	      status = regcache_cooked_read (regcache, regnum, buf);

>> +	      status = regcache_cooked_read (regcache, regnum, buf.data ());

> 

> Can we use regcache_cooked_read_value so that we don't need buf at all.


Yes, that would work.
However, this cooked read is in the middle of a for() loop of every register value.

With the patch currently, we have the allocation of buf once, and then re-use it for each
iteration.

Switching to regcache_cooked_read_value() would result in a call to allocate_value()
(within the regcache_cooked_read_value code) every iteration, which would then be freed
after printing it.


> 

>> 	      if (status == REG_UNKNOWN)

>> 		fprintf_unfiltered (file, "<invalid>");

>> 	      else if (status == REG_UNAVAILABLE)

>> 		fprintf_unfiltered (file, "<unavailable>");

>> 	      else

>> -		print_hex_chars (file, buf,

>> +		print_hex_chars (file, buf.data (),

>> 				 regcache->descr->sizeof_register[regnum],

>> 				 gdbarch_byte_order (gdbarch));

>> 	    }

> 

> -- 

> Yao (齐尧)
diff mbox

Patch

diff --git a/gdb/frame.c b/gdb/frame.c
index d98003dee7c34a63bd25356e6674721664a4b2f3..22cfdea4bcb20582229ffc360ead060c43d7cd81 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1252,7 +1252,11 @@  frame_unwind_register_signed (struct frame_info *frame, int regnum)
   struct gdbarch *gdbarch = frame_unwind_arch (frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int size = register_size (gdbarch, regnum);
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  gdb_byte buf[sizeof (LONGEST)];
+
+  if (size > (int) sizeof (LONGEST))
+    error (_("Cannot unwind integers more than %d bytes."),
+	   (int) sizeof (LONGEST));

   frame_unwind_register (frame, regnum, buf);
   return extract_signed_integer (buf, size, byte_order);
@@ -1270,7 +1274,11 @@  frame_unwind_register_unsigned (struct frame_info *frame, int regnum)
   struct gdbarch *gdbarch = frame_unwind_arch (frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int size = register_size (gdbarch, regnum);
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  gdb_byte buf[sizeof (ULONGEST)];
+
+  if (size > (int) sizeof (ULONGEST))
+    error (_("Cannot unwind integers more than %d bytes."),
+	   (int) sizeof (ULONGEST));

   frame_unwind_register (frame, regnum, buf);
   return extract_unsigned_integer (buf, size, byte_order);
@@ -1410,16 +1418,21 @@  get_frame_register_bytes (struct frame_info *frame, int regnum,
 	}
       else
 	{
-	  gdb_byte buf[MAX_REGISTER_SIZE];
-	  enum lval_type lval;
-	  CORE_ADDR addr;
-	  int realnum;
+	  struct value *value = frame_unwind_register_value (frame->next,
+							     regnum);
+	  gdb_assert (value != NULL);
+	  *optimizedp = value_optimized_out (value);
+	  *unavailablep = !value_entirely_available (value);

-	  frame_register (frame, regnum, optimizedp, unavailablep,
-			  &lval, &addr, &realnum, buf);
 	  if (*optimizedp || *unavailablep)
-	    return 0;
-	  memcpy (myaddr, buf + offset, curr_len);
+	    {
+	      release_value (value);
+	      value_free (value);
+	      return 0;
+	    }
+	  memcpy (myaddr, value_contents_all (value) + offset, curr_len);
+	  release_value (value);
+	  value_free (value);
 	}

       myaddr += curr_len;
@@ -1460,11 +1473,15 @@  put_frame_register_bytes (struct frame_info *frame, int regnum,
 	}
       else
 	{
-	  gdb_byte buf[MAX_REGISTER_SIZE];
-
-	  deprecated_frame_register_read (frame, regnum, buf);
-	  memcpy (buf + offset, myaddr, curr_len);
-	  put_frame_register (frame, regnum, buf);
+	  struct value *value = frame_unwind_register_value (frame->next,
+							     regnum);
+	  gdb_assert (value != NULL);
+
+	  memcpy ((char *) value_contents_all (value) + offset, myaddr,
+		  curr_len);
+	  put_frame_register (frame, regnum, value_contents_all (value));
+	  release_value (value);
+	  value_free (value);
 	}

       myaddr += curr_len;
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 8a4d59f6fdae8ec785462d0ceedcd6501b955cf0..081a16c6896ce7aee4db3b0be45fbbdd2c23dbdb 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -3250,7 +3250,7 @@  i386_pseudo_register_read_into_value (struct gdbarch *gdbarch,
 				      int regnum,
 				      struct value *result_value)
 {
-  gdb_byte raw_buf[MAX_REGISTER_SIZE];
+  gdb_byte raw_buf[I386_MAX_REGISTER_SIZE];
   enum register_status status;
   gdb_byte *buf = value_contents_raw (result_value);

@@ -3455,7 +3455,7 @@  void
 i386_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
 			    int regnum, const gdb_byte *buf)
 {
-  gdb_byte raw_buf[MAX_REGISTER_SIZE];
+  gdb_byte raw_buf[I386_MAX_REGISTER_SIZE];

   if (i386_mmx_regnum_p (gdbarch, regnum))
     {
@@ -5037,7 +5037,7 @@  i386_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
   uint32_t opcode;
   uint8_t opcode8;
   ULONGEST addr;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  gdb_byte buf[I386_MAX_REGISTER_SIZE];
   struct i386_record_s ir;
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   uint8_t rex_w = -1;
diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index adbe72133089bc371108d5dd79bf8d8e61ba259c..fcd5ad248d6b737b9f27e294ce166a118e4bdcad 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -899,7 +899,7 @@  i387_supply_xsave (struct regcache *regcache, int regnum,
   const gdb_byte *regs = (const gdb_byte *) xsave;
   int i;
   unsigned int clear_bv;
-  static const gdb_byte zero[MAX_REGISTER_SIZE] = { 0 };
+  static const gdb_byte zero[I386_MAX_REGISTER_SIZE] = { 0 };
   enum
     {
       none = 0x0,
diff --git a/gdb/m68k-linux-nat.c b/gdb/m68k-linux-nat.c
index 6944c74eb198381135fda3ddf01b9da3a63e62d5..e5182caf39197f759c85c2321e4d66c428f5911e 100644
--- a/gdb/m68k-linux-nat.c
+++ b/gdb/m68k-linux-nat.c
@@ -105,7 +105,7 @@  fetch_register (struct regcache *regcache, int regno)
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   long regaddr, val;
   int i;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  gdb_byte buf[M68K_MAX_REGISTER_SIZE];
   int tid;

   /* Overload thread id onto process id.  */
@@ -160,7 +160,7 @@  store_register (const struct regcache *regcache, int regno)
   long regaddr, val;
   int i;
   int tid;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  gdb_byte buf[M68K_MAX_REGISTER_SIZE];

   /* Overload thread id onto process id.  */
   tid = ptid_get_lwp (inferior_ptid);
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 57c23ebf5d6b2d3b398aa40ebd9b3cb70c56125c..11702c6c06c5cd791e03d99d56ef93d2c234862b 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1135,8 +1135,8 @@  register_changed_p (int regnum, struct regcache *prev_regs,
 		    struct regcache *this_regs)
 {
   struct gdbarch *gdbarch = get_regcache_arch (this_regs);
-  gdb_byte prev_buffer[MAX_REGISTER_SIZE];
-  gdb_byte this_buffer[MAX_REGISTER_SIZE];
+  std::vector<gdb_byte> prev_buffer (register_size (gdbarch, regnum));
+  std::vector<gdb_byte> this_buffer (register_size (gdbarch, regnum));
   enum register_status prev_status;
   enum register_status this_status;

@@ -1146,13 +1146,13 @@  register_changed_p (int regnum, struct regcache *prev_regs,
     return 1;

   /* Get register contents and compare.  */
-  prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer);
-  this_status = regcache_cooked_read (this_regs, regnum, this_buffer);
+  prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer.data ());
+  this_status = regcache_cooked_read (this_regs, regnum, this_buffer.data ());

   if (this_status != prev_status)
     return 1;
   else if (this_status == REG_VALID)
-    return memcmp (prev_buffer, this_buffer,
+    return memcmp (prev_buffer.data (), this_buffer.data (),
 		   register_size (gdbarch, regnum)) != 0;
   else
     return 0;
diff --git a/gdb/record-full.c b/gdb/record-full.c
index fdd613b6e41bbfcd8644b02ccfeb98b53b518bff..ada86cf6726176d93f61d85ef67290bca46c5b1a 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -698,7 +698,7 @@  record_full_exec_insn (struct regcache *regcache,
     {
     case record_full_reg: /* reg */
       {
-        gdb_byte reg[MAX_REGISTER_SIZE];
+	std::vector<gdb_byte> reg (register_size (gdbarch, entry->u.reg.num));

         if (record_debug > 1)
           fprintf_unfiltered (gdb_stdlog,
@@ -707,10 +707,10 @@  record_full_exec_insn (struct regcache *regcache,
                               host_address_to_string (entry),
                               entry->u.reg.num);

-        regcache_cooked_read (regcache, entry->u.reg.num, reg);
-        regcache_cooked_write (regcache, entry->u.reg.num,
+	regcache_cooked_read (regcache, entry->u.reg.num, reg.data ());
+	regcache_cooked_write (regcache, entry->u.reg.num,
 			       record_full_get_loc (entry));
-        memcpy (record_full_get_loc (entry), reg, entry->u.reg.len);
+	memcpy (record_full_get_loc (entry), reg.data (), entry->u.reg.len);
       }
       break;

@@ -792,15 +792,17 @@  static void
 record_full_core_open_1 (const char *name, int from_tty)
 {
   struct regcache *regcache = get_current_regcache ();
-  int regnum = gdbarch_num_regs (get_regcache_arch (regcache));
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  int regnum = gdbarch_num_regs (gdbarch);
+  int regmaxsize = max_register_size (gdbarch);
   int i;

   /* Get record_full_core_regbuf.  */
   target_fetch_registers (regcache, -1);
-  record_full_core_regbuf = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE * regnum);
+  record_full_core_regbuf = (gdb_byte *) xmalloc (regmaxsize * regnum);
   for (i = 0; i < regnum; i ++)
     regcache_raw_collect (regcache, i,
-			  record_full_core_regbuf + MAX_REGISTER_SIZE * i);
+			  record_full_core_regbuf + regmaxsize * i);

   /* Get record_full_core_start and record_full_core_end.  */
   if (build_section_table (core_bfd, &record_full_core_start,
@@ -2038,6 +2040,9 @@  record_full_core_fetch_registers (struct target_ops *ops,
 				  struct regcache *regcache,
 				  int regno)
 {
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  int regmaxsize = max_register_size (gdbarch);
+
   if (regno < 0)
     {
       int num = gdbarch_num_regs (get_regcache_arch (regcache));
@@ -2045,11 +2050,11 @@  record_full_core_fetch_registers (struct target_ops *ops,

       for (i = 0; i < num; i ++)
         regcache_raw_supply (regcache, i,
-                             record_full_core_regbuf + MAX_REGISTER_SIZE * i);
+			     record_full_core_regbuf + regmaxsize * i);
     }
   else
     regcache_raw_supply (regcache, regno,
-                         record_full_core_regbuf + MAX_REGISTER_SIZE * regno);
+			 record_full_core_regbuf + regmaxsize * regno);
 }

 /* "to_prepare_to_store" method for prec over corefile.  */
@@ -2067,9 +2072,12 @@  record_full_core_store_registers (struct target_ops *ops,
                              struct regcache *regcache,
                              int regno)
 {
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  int regmaxsize = max_register_size (gdbarch);
+
   if (record_full_gdb_operation_disable)
     regcache_raw_collect (regcache, regno,
-                          record_full_core_regbuf + MAX_REGISTER_SIZE * regno);
+			  record_full_core_regbuf + regmaxsize * regno);
   else
     error (_("You can't do that without a process to debug."));
 }
@@ -2253,25 +2261,6 @@  init_record_full_core_ops (void)
 }

 /* Record log save-file format
-   Version 1 (never released)
-
-   Header:
-     4 bytes: magic number htonl(0x20090829).
-       NOTE: be sure to change whenever this file format changes!
-
-   Records:
-     record_full_end:
-       1 byte:  record type (record_full_end, see enum record_full_type).
-     record_full_reg:
-       1 byte:  record type (record_full_reg, see enum record_full_type).
-       8 bytes: register id (network byte order).
-       MAX_REGISTER_SIZE bytes: register value.
-     record_full_mem:
-       1 byte:  record type (record_full_mem, see enum record_full_type).
-       8 bytes: memory length (network byte order).
-       8 bytes: memory address (network byte order).
-       n bytes: memory value (n == memory length).
-
    Version 2
      4 bytes: magic number netorder32(0x20091016).
        NOTE: be sure to change whenever this file format changes!
diff --git a/gdb/regcache.h b/gdb/regcache.h
index e5a7cf553279b8cc0d546ec1b8274cbf97e246d5..f8d8f2f84887cb9bb2541f68367f5269b75deb56 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -50,6 +50,12 @@  extern struct address_space *get_regcache_aspace (const struct regcache *);
 enum register_status regcache_register_status (const struct regcache *regcache,
 					       int regnum);

+/* Make certain that the register cache is up-to-date with respect to the
+   current thread.  */
+void regcache_raw_update (struct regcache *regcache, int regnum);
+
+enum register_status regcache_raw_read (struct regcache *regcache,
+					int rawnum, gdb_byte *buf);
 /* Transfer a raw register [0..NUM_REGS) between core-gdb and the
    regcache.  The read variants return the status of the register.  */

@@ -202,6 +208,8 @@  extern struct type *register_type (struct gdbarch *gdbarch, int regnum);

 extern int register_size (struct gdbarch *gdbarch, int regnum);

+/* Return the size of the largest register.  */
+extern long max_register_size (struct gdbarch *gdbarch);

 /* Save/restore a register cache.  The set of registers saved /
    restored into the DST regcache determined by the save_reggroup /
@@ -228,4 +236,8 @@  extern void regcache_cpy (struct regcache *dest, struct regcache *src);
 extern void registers_changed (void);
 extern void registers_changed_ptid (ptid_t);

+extern void regcache_debug_print_register (const char *func,
+					   struct regcache *regcache,
+					   int regno);
+
 #endif /* REGCACHE_H */
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 9d28aa2c2114e0f1c52758bb2fbe9669a329c13e..7d693ac616c3c8e692d369d748937ac4662c849a 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -73,6 +73,9 @@  struct regcache_descr

   /* Cached table containing the type of each register.  */
   struct type **register_type;
+
+  /* Size of the largest register.  */
+  long max_register_size;
 };

 static void *
@@ -126,6 +129,8 @@  init_regcache_descr (struct gdbarch *gdbarch)
 	descr->register_offset[i] = offset;
 	offset += descr->sizeof_register[i];
 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
+	descr->max_register_size = std::max (descr->max_register_size,
+					     descr->sizeof_register[i]);
       }
     /* Set the real size of the raw register cache buffer.  */
     descr->sizeof_raw_registers = offset;
@@ -136,6 +141,8 @@  init_regcache_descr (struct gdbarch *gdbarch)
 	descr->register_offset[i] = offset;
 	offset += descr->sizeof_register[i];
 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
+	descr->max_register_size = std::max (descr->max_register_size,
+					     descr->sizeof_register[i]);
       }
     /* Set the real size of the readonly register cache buffer.  */
     descr->sizeof_cooked_registers = offset;
@@ -187,6 +194,13 @@  regcache_register_size (const struct regcache *regcache, int n)
   return register_size (get_regcache_arch (regcache), n);
 }

+long
+max_register_size (struct gdbarch *gdbarch)
+{
+  struct regcache_descr *descr = regcache_descr (gdbarch);
+  return descr->max_register_size;
+}
+
 /* The register cache for storing raw register values.  */

 struct regcache
@@ -327,7 +341,6 @@  regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
 	       void *src)
 {
   struct gdbarch *gdbarch = dst->descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
   int regnum;

   /* The DST should be `read-only', if it wasn't then the save would
@@ -346,12 +359,10 @@  regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
     {
       if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
 	{
+	  gdb_byte *buf = register_buffer (dst, regnum);
 	  enum register_status status = cooked_read (src, regnum, buf);

-	  if (status == REG_VALID)
-	    memcpy (register_buffer (dst, regnum), buf,
-		    register_size (gdbarch, regnum));
-	  else
+	  if (status != REG_VALID)
 	    {
 	      gdb_assert (status != REG_UNKNOWN);

@@ -369,7 +380,7 @@  regcache_restore (struct regcache *dst,
 		  void *cooked_read_context)
 {
   struct gdbarch *gdbarch = dst->descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  std::vector<gdb_byte> buf (max_register_size (gdbarch));
   int regnum;

   /* The dst had better not be read-only.  If it is, the `restore'
@@ -385,9 +396,9 @@  regcache_restore (struct regcache *dst,
 	{
 	  enum register_status status;

-	  status = cooked_read (cooked_read_context, regnum, buf);
+	  status = cooked_read (cooked_read_context, regnum, buf.data ());
 	  if (status == REG_VALID)
-	    regcache_cooked_write (dst, regnum, buf);
+	    regcache_cooked_write (dst, regnum, buf.data ());
 	}
     }
 }
@@ -642,15 +653,17 @@  registers_changed (void)
   alloca (0);
 }

-enum register_status
-regcache_raw_read (struct regcache *regcache, int regnum, gdb_byte *buf)
+void
+regcache_raw_update (struct regcache *regcache, int regnum)
 {
-  gdb_assert (regcache != NULL && buf != NULL);
-  gdb_assert (regnum >= 0 && regnum < regcache->descr->nr_raw_registers);
   /* Make certain that the register cache is up-to-date with respect
      to the current thread.  This switching shouldn't be necessary
      only there is still only one target side register cache.  Sigh!
      On the bright side, at least there is a regcache object.  */
+
+  gdb_assert (regcache != NULL);
+  gdb_assert (regnum >= 0 && regnum < regcache->descr->nr_raw_registers);
+
   if (!regcache->readonly_p
       && regcache_register_status (regcache, regnum) == REG_UNKNOWN)
     {
@@ -666,6 +679,13 @@  regcache_raw_read (struct regcache *regcache, int regnum, gdb_byte *buf)
       if (regcache->register_status[regnum] == REG_UNKNOWN)
 	regcache->register_status[regnum] = REG_UNAVAILABLE;
     }
+}
+
+enum register_status
+regcache_raw_read (struct regcache *regcache, int regnum, gdb_byte *buf)
+{
+  gdb_assert (buf != NULL);
+  regcache_raw_update (regcache, regnum);

   if (regcache->register_status[regnum] != REG_VALID)
     memset (buf, 0, regcache->descr->sizeof_register[regnum]);
@@ -1250,6 +1270,42 @@  regcache_write_pc (struct regcache *regcache, CORE_ADDR pc)
   reinit_frame_cache ();
 }

+void
+regcache_debug_print_register (const char *func, struct regcache *regcache,
+			       int regno)
+{
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+
+  fprintf_unfiltered (gdb_stdlog, "%s ", func);
+  if (regno >= 0 && regno < gdbarch_num_regs (gdbarch)
+      && gdbarch_register_name (gdbarch, regno) != NULL
+      && gdbarch_register_name (gdbarch, regno)[0] != '\0')
+    fprintf_unfiltered (gdb_stdlog, "(%s)",
+			gdbarch_register_name (gdbarch, regno));
+  else
+    fprintf_unfiltered (gdb_stdlog, "(%d)", regno);
+  if (regno >= 0 && regno < gdbarch_num_regs (gdbarch))
+    {
+      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+      int i, size = register_size (gdbarch, regno);
+      gdb_byte *buf = register_buffer (regcache, regno);
+
+      regcache_raw_collect (regcache, regno, buf);
+      fprintf_unfiltered (gdb_stdlog, " = ");
+      for (i = 0; i < size; i++)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "%02x", buf[i]);
+	}
+      if (size <= sizeof (LONGEST))
+	{
+	  ULONGEST val = extract_unsigned_integer (buf, size, byte_order);
+
+	  fprintf_unfiltered (gdb_stdlog, " %s %s",
+			      core_addr_to_string_nz (val), plongest (val));
+	}
+    }
+  fprintf_unfiltered (gdb_stdlog, "\n");
+}

 static void
 reg_flush_command (char *command, int from_tty)
@@ -1279,7 +1335,7 @@  regcache_dump (struct regcache *regcache, struct ui_file *file,
   int footnote_register_offset = 0;
   int footnote_register_type_name_null = 0;
   long register_offset = 0;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  std::vector<gdb_byte> buf (max_register_size (gdbarch));

 #if 0
   fprintf_unfiltered (file, "nr_raw_registers %d\n",
@@ -1406,8 +1462,8 @@  regcache_dump (struct regcache *regcache, struct ui_file *file,
 	    fprintf_unfiltered (file, "<unavailable>");
 	  else
 	    {
-	      regcache_raw_read (regcache, regnum, buf);
-	      print_hex_chars (file, buf,
+	      regcache_raw_update (regcache, regnum);
+	      print_hex_chars (file, register_buffer (regcache, regnum),
 			       regcache->descr->sizeof_register[regnum],
 			       gdbarch_byte_order (gdbarch));
 	    }
@@ -1422,13 +1478,13 @@  regcache_dump (struct regcache *regcache, struct ui_file *file,
 	    {
 	      enum register_status status;

-	      status = regcache_cooked_read (regcache, regnum, buf);
+	      status = regcache_cooked_read (regcache, regnum, buf.data ());
 	      if (status == REG_UNKNOWN)
 		fprintf_unfiltered (file, "<invalid>");
 	      else if (status == REG_UNAVAILABLE)
 		fprintf_unfiltered (file, "<unavailable>");
 	      else
-		print_hex_chars (file, buf,
+		print_hex_chars (file, buf.data (),
 				 regcache->descr->sizeof_register[regnum],
 				 gdbarch_byte_order (gdbarch));
 	    }
diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index b0c68c617e365c0ea18ac2eca525598b688ffbb5..0dd901377e53eafc27e4fb39a1453380779ae361 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -439,6 +439,9 @@  gdbsim_fetch_register (struct target_ops *ops,
       return;
     }

+  int regsize = register_size (gdbarch, regno);
+  std::vector<gdb_byte> buf (regsize);
+
   switch (gdbarch_register_sim_regno (gdbarch, regno))
     {
     case LEGACY_SIM_REGNO_IGNORE:
@@ -447,38 +450,32 @@  gdbsim_fetch_register (struct target_ops *ops,
       {
 	/* For moment treat a `does not exist' register the same way
 	   as an ``unavailable'' register.  */
-	gdb_byte buf[MAX_REGISTER_SIZE];
 	int nr_bytes;

-	memset (buf, 0, MAX_REGISTER_SIZE);
-	regcache_raw_supply (regcache, regno, buf);
+	regcache_raw_supply (regcache, regno, buf.data ());
 	break;
       }

     default:
       {
 	static int warn_user = 1;
-	gdb_byte buf[MAX_REGISTER_SIZE];
 	int nr_bytes;

 	gdb_assert (regno >= 0 && regno < gdbarch_num_regs (gdbarch));
-	memset (buf, 0, MAX_REGISTER_SIZE);
 	nr_bytes = sim_fetch_register (sim_data->gdbsim_desc,
 				       gdbarch_register_sim_regno
 					 (gdbarch, regno),
-				       buf,
-				       register_size (gdbarch, regno));
+				       buf.data (), regsize);
 	if (nr_bytes > 0
-	    && nr_bytes != register_size (gdbarch, regno) && warn_user)
+	    && nr_bytes != regsize && warn_user)
 	  {
 	    fprintf_unfiltered (gdb_stderr,
 				"Size of register %s (%d/%d) "
 				"incorrect (%d instead of %d))",
 				gdbarch_register_name (gdbarch, regno),
 				regno,
-				gdbarch_register_sim_regno
-				  (gdbarch, regno),
-				nr_bytes, register_size (gdbarch, regno));
+				gdbarch_register_sim_regno (gdbarch, regno),
+				nr_bytes, regsize);
 	    warn_user = 0;
 	  }
 	/* FIXME: cagney/2002-05-27: Should check `nr_bytes == 0'
@@ -486,13 +483,13 @@  gdbsim_fetch_register (struct target_ops *ops,
 	   which registers are fetchable.  */
 	/* Else if (nr_bytes < 0): an old simulator, that doesn't
 	   think to return the register size.  Just assume all is ok.  */
-	regcache_raw_supply (regcache, regno, buf);
+	regcache_raw_supply (regcache, regno, buf.data ());
 	if (remote_debug)
 	  {
 	    fprintf_unfiltered (gdb_stdlog,
 				"gdbsim_fetch_register: %d", regno);
 	    /* FIXME: We could print something more intelligible.  */
-	    dump_mem (buf, register_size (gdbarch, regno));
+	    dump_mem (buf.data (), regsize);
 	  }
 	break;
       }
@@ -516,15 +513,16 @@  gdbsim_store_register (struct target_ops *ops,
     }
   else if (gdbarch_register_sim_regno (gdbarch, regno) >= 0)
     {
-      gdb_byte tmp[MAX_REGISTER_SIZE];
+      int regsize = register_size (gdbarch, regno);
+      std::vector<gdb_byte> tmp (regsize);
       int nr_bytes;

-      regcache_cooked_read (regcache, regno, tmp);
+      regcache_cooked_read (regcache, regno, tmp.data ());
       nr_bytes = sim_store_register (sim_data->gdbsim_desc,
 				     gdbarch_register_sim_regno
 				       (gdbarch, regno),
-				     tmp, register_size (gdbarch, regno));
-      if (nr_bytes > 0 && nr_bytes != register_size (gdbarch, regno))
+				     tmp.data (), regsize);
+      if (nr_bytes > 0 && nr_bytes != regsize)
 	internal_error (__FILE__, __LINE__,
 			_("Register size different to expected"));
       if (nr_bytes < 0)
@@ -538,7 +536,7 @@  gdbsim_store_register (struct target_ops *ops,
 	{
 	  fprintf_unfiltered (gdb_stdlog, "gdbsim_store_register: %d", regno);
 	  /* FIXME: We could print something more intelligible.  */
-	  dump_mem (tmp, register_size (gdbarch, regno));
+	  dump_mem (tmp.data (), regsize);
 	}
     }
 }
diff --git a/gdb/remote.c b/gdb/remote.c
index c4cec910c44cf91cc7f36b7f2d87cde3f46de41e..0eeba218ea4846466746817e8e79a5bbe84fba95 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7759,7 +7759,6 @@  remote_prepare_to_store (struct target_ops *self, struct regcache *regcache)
 {
   struct remote_arch_state *rsa = get_remote_arch_state ();
   int i;
-  gdb_byte buf[MAX_REGISTER_SIZE];

   /* Make sure the entire registers array is valid.  */
   switch (packet_support (PACKET_P))
@@ -7769,7 +7768,7 @@  remote_prepare_to_store (struct target_ops *self, struct regcache *regcache)
       /* Make sure all the necessary registers are cached.  */
       for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++)
 	if (rsa->regs[i].in_g_packet)
-	  regcache_raw_read (regcache, rsa->regs[i].regnum, buf);
+	  regcache_raw_update (regcache, rsa->regs[i].regnum);
       break;
     case PACKET_ENABLE:
       break;
diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
index a09a3ab9a8bc56f367e3ba1537f5674f0a7f491f..06b146c314a10be0e360a7242bab229ced1c00b1 100644
--- a/gdb/sol-thread.c
+++ b/gdb/sol-thread.c
@@ -534,12 +534,6 @@  sol_thread_store_registers (struct target_ops *ops,

   if (regnum != -1)
     {
-      /* Not writing all the registers.  */
-      char old_value[MAX_REGISTER_SIZE];
-
-      /* Save new register value.  */
-      regcache_raw_collect (regcache, regnum, old_value);
-
       val = p_td_thr_getgregs (&thandle, gregset);
       if (val != TD_OK)
 	error (_("sol_thread_store_registers: td_thr_getgregs %s"),
@@ -548,9 +542,6 @@  sol_thread_store_registers (struct target_ops *ops,
       if (val != TD_OK)
 	error (_("sol_thread_store_registers: td_thr_getfpregs %s"),
 	       td_err_string (val));
-
-      /* Restore new register value.  */
-      regcache_raw_supply (regcache, regnum, old_value);
     }

   fill_gregset (regcache, (gdb_gregset_t *) &gregset, regnum);
diff --git a/gdb/stack.c b/gdb/stack.c
index e00e2972cf20bc63917af19f86bf57f1c6b0b5b0..7ba7d68bde8d83ea1e700faa466c6951979e0f76 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1650,33 +1650,35 @@  frame_info (char *addr_exp, int from_tty)
     int count;
     int i;
     int need_nl = 1;
+    int sp_regnum = gdbarch_sp_regnum (gdbarch);

     /* The sp is special; what's displayed isn't the save address, but
        the value of the previous frame's sp.  This is a legacy thing,
        at one stage the frame cached the previous frame's SP instead
        of its address, hence it was easiest to just display the cached
        value.  */
-    if (gdbarch_sp_regnum (gdbarch) >= 0)
+    if (sp_regnum >= 0)
       {
 	/* Find out the location of the saved stack pointer with out
            actually evaluating it.  */
-	frame_register_unwind (fi, gdbarch_sp_regnum (gdbarch),
-			       &optimized, &unavailable, &lval, &addr,
-			       &realnum, NULL);
+	frame_register_unwind (fi, sp_regnum, &optimized, &unavailable, &lval,
+			       &addr, &realnum, NULL);
 	if (!optimized && !unavailable && lval == not_lval)
 	  {
 	    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-	    int sp_size = register_size (gdbarch, gdbarch_sp_regnum (gdbarch));
-	    gdb_byte value[MAX_REGISTER_SIZE];
+	    int sp_size = register_size (gdbarch, sp_regnum);
 	    CORE_ADDR sp;
+	    struct value *value = frame_unwind_register_value (fi, sp_regnum);

-	    frame_register_unwind (fi, gdbarch_sp_regnum (gdbarch),
-				   &optimized, &unavailable, &lval, &addr,
-				   &realnum, value);
+	    gdb_assert (value != NULL);
 	    /* NOTE: cagney/2003-05-22: This is assuming that the
                stack pointer was packed as an unsigned integer.  That
                may or may not be valid.  */
-	    sp = extract_unsigned_integer (value, sp_size, byte_order);
+	    sp = extract_unsigned_integer (value_contents_all (value), sp_size,
+					   byte_order);
+	    release_value (value);
+	    value_free (value);
+
 	    printf_filtered (" Previous frame's sp is ");
 	    fputs_filtered (paddress (gdbarch, sp), gdb_stdout);
 	    printf_filtered ("\n");
@@ -1702,7 +1704,7 @@  frame_info (char *addr_exp, int from_tty)
     numregs = gdbarch_num_regs (gdbarch)
 	      + gdbarch_num_pseudo_regs (gdbarch);
     for (i = 0; i < numregs; i++)
-      if (i != gdbarch_sp_regnum (gdbarch)
+      if (i != sp_regnum
 	  && gdbarch_register_reggroup_p (gdbarch, i, all_reggroup))
 	{
 	  /* Find out the location of the saved register without
diff --git a/gdb/target.c b/gdb/target.c
index 3c409f0f619141205dfdcbbf8e46a277585ed683..ae1321c57983fe3446148090e1d0a4aa823a3b19 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3547,49 +3547,12 @@  target_options_to_string (int target_options)
   return ret;
 }

-static void
-debug_print_register (const char * func,
-		      struct regcache *regcache, int regno)
-{
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
-  fprintf_unfiltered (gdb_stdlog, "%s ", func);
-  if (regno >= 0 && regno < gdbarch_num_regs (gdbarch)
-      && gdbarch_register_name (gdbarch, regno) != NULL
-      && gdbarch_register_name (gdbarch, regno)[0] != '\0')
-    fprintf_unfiltered (gdb_stdlog, "(%s)",
-			gdbarch_register_name (gdbarch, regno));
-  else
-    fprintf_unfiltered (gdb_stdlog, "(%d)", regno);
-  if (regno >= 0 && regno < gdbarch_num_regs (gdbarch))
-    {
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      int i, size = register_size (gdbarch, regno);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-
-      regcache_raw_collect (regcache, regno, buf);
-      fprintf_unfiltered (gdb_stdlog, " = ");
-      for (i = 0; i < size; i++)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "%02x", buf[i]);
-	}
-      if (size <= sizeof (LONGEST))
-	{
-	  ULONGEST val = extract_unsigned_integer (buf, size, byte_order);
-
-	  fprintf_unfiltered (gdb_stdlog, " %s %s",
-			      core_addr_to_string_nz (val), plongest (val));
-	}
-    }
-  fprintf_unfiltered (gdb_stdlog, "\n");
-}
-
 void
 target_fetch_registers (struct regcache *regcache, int regno)
 {
   current_target.to_fetch_registers (&current_target, regcache, regno);
   if (targetdebug)
-    debug_print_register ("target_fetch_registers", regcache, regno);
+    regcache_debug_print_register ("target_fetch_registers", regcache, regno);
 }

 void
@@ -3601,7 +3564,8 @@  target_store_registers (struct regcache *regcache, int regno)
   current_target.to_store_registers (&current_target, regcache, regno);
   if (targetdebug)
     {
-      debug_print_register ("target_store_registers", regcache, regno);
+      regcache_debug_print_register ("target_store_registers", regcache,
+				     regno);
     }
 }