[3/7] : Regcache: Remove xmalloc/xfree methods

  Regcache is a class. There is no need for explicit xmalloc
and xfree methods, it is much simpler to use new and delete.


Tested on a --enable-targets=all build with board files unix,
native-gdbserver and unittest.exp.


2017-08-16  Alan Hayward  <alan.hayward@arm.com>

	* frame.c (frame_save_as_regcache): Use new.
	(frame_pop): Update name.
	* infrun.c (discard_infcall_suspend_state): Use delete.
	* jit.c (jit_dealloc_cache): Use delete.
	(jit_frame_sniffer): Use new.
	* linux-fork.c (free_fork): Use delete.
	(fork_save_infrun_state): Likewise.
	* mi/mi-main.c (mi_cmd_data_list_changed_registers): Update name.
	* ppc-linux-tdep.c (ppu2spu_sniffer): Use new.
	(ppu2spu_dealloc_cache): Use delete.
	* regcache.c (regcache_xmalloc): Remove.
	(regcache_xfree): Remove.
	(do_regcache_delete): Update name.
	(make_cleanup_regcache_delete): Likewise.
	* regcache.h: Remove functions and rename.
	* spu-tdep.c (spu2ppu_dealloc_cache): Use delete.
  

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

> Regcache is a class. There is no need for explicit xmalloc
> and xfree methods, it is much simpler to use new and delete.
>

new/delete isn't simpler than xmalloc/xfree, IMO.  We still need to take
care of releasing resources.

>
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 30e4aeab7e2901074c289ac4d96ebda885805a29..7fd4b07a2e95f28b2eb6a18dea4d2071f0ece4e2 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1021,13 +1021,12 @@ struct regcache *
> frame_save_as_regcache (struct frame_info *this_frame)
> {
>   struct address_space *aspace = get_frame_address_space (this_frame);
> -  struct regcache *regcache = regcache_xmalloc (get_frame_arch (this_frame),
> -						aspace);
> -  struct cleanup *cleanups = make_cleanup_regcache_xfree (regcache);
> +  regcache *backup = new regcache (get_frame_arch (this_frame), aspace);
> +  struct cleanup *cleanups = make_cleanup_regcache_delete (backup);
>

It makes no sense to change make_cleanup_regcache_xfree to
make_cleanup_regcache_delete.  We still have to use cleanup.

> -  regcache_save (regcache, do_frame_register_read, this_frame);
> +  regcache_save (backup, do_frame_register_read, this_frame);
>   discard_cleanups (cleanups);
> -  return regcache;
> +  return backup;
> }
>
> void
> @@ -1063,7 +1062,7 @@ frame_pop (struct frame_info *this_frame)
>      trying to extract the old values from the current regcache while
>      at the same time writing new values into that same cache.  */
>   scratch = frame_save_as_regcache (prev_frame);
> -  cleanups = make_cleanup_regcache_xfree (scratch);
> +  cleanups = make_cleanup_regcache_delete (scratch);

scratch is only used within this function, so we can change it to a
local object (instead of a pointer), and call regcache_save here.  Then,
we can remove the cleanups.

> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 0bf587ffe702c68f538fe2877cce6114e64ee1bd..1edcf752c82230fc65669f675e10735ac8e4f654 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1051,7 +1051,7 @@ mi_cmd_data_list_changed_registers (const char *command, char **argv, int argc)
>
>   prev_regs = this_regs;
>   this_regs = frame_save_as_regcache (get_selected_frame (NULL));
> -  cleanup = make_cleanup_regcache_xfree (prev_regs);
> +  cleanup = make_cleanup_regcache_delete (prev_regs);

Why don't you do this? then, we don't need this make_cleanup_regcache_xfree.

std::unique_ptr<regcache> prev_regs (this_regs);

>
>   /* Note that the test for a valid register must include checking the
>      gdbarch_register_name because gdbarch_num_regs may be allocated
> diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
> index 324b29d90c1266a73f172da20a6015174189625f..42aff2cd1bf3834268b0b58dcf00dac1c52add96 100644
> --- a/gdb/ppc-linux-tdep.c
> +++ b/gdb/ppc-linux-tdep.c
> @@ -1364,13 +1364,13 @@ ppu2spu_sniffer (const struct frame_unwind *self,
> 	    = FRAME_OBSTACK_CALLOC (1, struct ppu2spu_cache);
>
> 	  struct address_space *aspace = get_frame_address_space (this_frame);
> -	  struct regcache *regcache = regcache_xmalloc (data.gdbarch, aspace);
> -	  struct cleanup *cleanups = make_cleanup_regcache_xfree (regcache);
> -	  regcache_save (regcache, ppu2spu_unwind_register, &data);
> +	  regcache *backup = new regcache (data.gdbarch, aspace);
> +	  struct cleanup *cleanups = make_cleanup_regcache_delete (backup);
> +	  regcache_save (backup, ppu2spu_unwind_register, &data);
> 	  discard_cleanups (cleanups);

We can use std::unique_ptr<regcache> too here.  After call
regcache_save, call .release () to return the raw pointer to cache->regcache.

>
> 	  cache->frame_id = frame_id_build (base, func);
> -	  cache->regcache = regcache;
> +	  cache->regcache = backup;
> 	  *this_prologue_cache = cache;
> 	  return 1;
> 	}
> @@ -1383,7 +1383,7 @@ static void
> ppu2spu_dealloc_cache (struct frame_info *self, void *this_cache)
> {
>   struct ppu2spu_cache *cache = (struct ppu2spu_cache *) this_cache;
> -  regcache_xfree (cache->regcache);
> +  delete cache->regcache;
> }
>
> static const struct frame_unwind ppu2spu_unwind = {
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index aeac54e8d7b91e2bcf6a3e1ce8e781c3f994306b..00b87db7d145205b74859c08ca5a9d852441a4aa 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -36,10 +36,7 @@ extern target_regcache *get_thread_arch_aspace_regcache (ptid_t,
> 							 struct gdbarch *,
> 							 struct address_space *);
>
> -void regcache_xfree (struct regcache *regcache);
> -struct cleanup *make_cleanup_regcache_xfree (struct regcache *regcache);
> -struct regcache *regcache_xmalloc (struct gdbarch *gdbarch,
> -				   struct address_space *aspace);
> +struct cleanup *make_cleanup_regcache_delete (regcache *regcache);
>
> /* Return REGCACHE's ptid.  */
>
> @@ -261,12 +258,7 @@ public:
>   regcache (const regcache &) = delete;
>   void operator= (const regcache &) = delete;
>
> -  /* class regcache is only extended in unit test, so only mark it
> -     virtual when selftest is enabled.  */
> -#if GDB_SELF_TEST
> -  virtual
> -#endif
> -  ~regcache ()
> +  virtual ~regcache ()

This is not related to this patch.  It should be in patch #1.
  

> On 23 Aug 2017, at 10:32, Yao Qi <qiyaoltc@gmail.com> wrote:

> 

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

> 

>> Regcache is a class. There is no need for explicit xmalloc

>> and xfree methods, it is much simpler to use new and delete.

>> 

> 

> new/delete isn't simpler than xmalloc/xfree, IMO.  We still need to take

> care of releasing resources.


Agreed, but I wanted to only have one way of doing the same thing.
We have to have a constructor/destructor.
regcache_xmalloc and regcache_xfree are (almost) one liners that call new/delete.

> 

>> 

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

>> index 30e4aeab7e2901074c289ac4d96ebda885805a29..7fd4b07a2e95f28b2eb6a18dea4d2071f0ece4e2 100644

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

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

>> @@ -1021,13 +1021,12 @@ struct regcache *

>> frame_save_as_regcache (struct frame_info *this_frame)

>> {

>>  struct address_space *aspace = get_frame_address_space (this_frame);

>> -  struct regcache *regcache = regcache_xmalloc (get_frame_arch (this_frame),

>> -						aspace);

>> -  struct cleanup *cleanups = make_cleanup_regcache_xfree (regcache);

>> +  regcache *backup = new regcache (get_frame_arch (this_frame), aspace);

>> +  struct cleanup *cleanups = make_cleanup_regcache_delete (backup);

>> 

> 

> It makes no sense to change make_cleanup_regcache_xfree to

> make_cleanup_regcache_delete.  We still have to use cleanup.


I made this change to keep the consistent with the other changes in this patch.
Using new together with something_xfree() sounds wrong.

If I make all the changes below, then I think we can remove this function.

> 

>> -  regcache_save (regcache, do_frame_register_read, this_frame);

>> +  regcache_save (backup, do_frame_register_read, this_frame);

>>  discard_cleanups (cleanups);

>> -  return regcache;

>> +  return backup;

>> }

>> 

>> void

>> @@ -1063,7 +1062,7 @@ frame_pop (struct frame_info *this_frame)

>>     trying to extract the old values from the current regcache while

>>     at the same time writing new values into that same cache.  */

>>  scratch = frame_save_as_regcache (prev_frame);

>> -  cleanups = make_cleanup_regcache_xfree (scratch);

>> +  cleanups = make_cleanup_regcache_delete (scratch);

> 

> scratch is only used within this function, so we can change it to a

> local object (instead of a pointer), and call regcache_save here.  Then,

> we can remove the cleanups.


Ok.

> 

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

>> index 0bf587ffe702c68f538fe2877cce6114e64ee1bd..1edcf752c82230fc65669f675e10735ac8e4f654 100644

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

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

>> @@ -1051,7 +1051,7 @@ mi_cmd_data_list_changed_registers (const char *command, char **argv, int argc)

>> 

>>  prev_regs = this_regs;

>>  this_regs = frame_save_as_regcache (get_selected_frame (NULL));

>> -  cleanup = make_cleanup_regcache_xfree (prev_regs);

>> +  cleanup = make_cleanup_regcache_delete (prev_regs);

> 

> Why don't you do this? then, we don't need this make_cleanup_regcache_xfree.

> 

> std::unique_ptr<regcache> prev_regs (this_regs);


This is new to me. Happy to use it.

> 

>> 

>>  /* Note that the test for a valid register must include checking the

>>     gdbarch_register_name because gdbarch_num_regs may be allocated

>> diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c

>> index 324b29d90c1266a73f172da20a6015174189625f..42aff2cd1bf3834268b0b58dcf00dac1c52add96 100644

>> --- a/gdb/ppc-linux-tdep.c

>> +++ b/gdb/ppc-linux-tdep.c

>> @@ -1364,13 +1364,13 @@ ppu2spu_sniffer (const struct frame_unwind *self,

>> 	    = FRAME_OBSTACK_CALLOC (1, struct ppu2spu_cache);

>> 

>> 	  struct address_space *aspace = get_frame_address_space (this_frame);

>> -	  struct regcache *regcache = regcache_xmalloc (data.gdbarch, aspace);

>> -	  struct cleanup *cleanups = make_cleanup_regcache_xfree (regcache);

>> -	  regcache_save (regcache, ppu2spu_unwind_register, &data);

>> +	  regcache *backup = new regcache (data.gdbarch, aspace);

>> +	  struct cleanup *cleanups = make_cleanup_regcache_delete (backup);

>> +	  regcache_save (backup, ppu2spu_unwind_register, &data);

>> 	  discard_cleanups (cleanups);

> 

> We can use std::unique_ptr<regcache> too here.  After call

> regcache_save, call .release () to return the raw pointer to cache->regcache.


Ok.

> 

>> 

>> 	  cache->frame_id = frame_id_build (base, func);

>> -	  cache->regcache = regcache;

>> +	  cache->regcache = backup;

>> 	  *this_prologue_cache = cache;

>> 	  return 1;

>> 	}

>> @@ -1383,7 +1383,7 @@ static void

>> ppu2spu_dealloc_cache (struct frame_info *self, void *this_cache)

>> {

>>  struct ppu2spu_cache *cache = (struct ppu2spu_cache *) this_cache;

>> -  regcache_xfree (cache->regcache);

>> +  delete cache->regcache;

>> }

>> 

>> static const struct frame_unwind ppu2spu_unwind = {

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

>> index aeac54e8d7b91e2bcf6a3e1ce8e781c3f994306b..00b87db7d145205b74859c08ca5a9d852441a4aa 100644

>> --- a/gdb/regcache.h

>> +++ b/gdb/regcache.h

>> @@ -36,10 +36,7 @@ extern target_regcache *get_thread_arch_aspace_regcache (ptid_t,

>> 							 struct gdbarch *,

>> 							 struct address_space *);

>> 

>> -void regcache_xfree (struct regcache *regcache);

>> -struct cleanup *make_cleanup_regcache_xfree (struct regcache *regcache);

>> -struct regcache *regcache_xmalloc (struct gdbarch *gdbarch,

>> -				   struct address_space *aspace);

>> +struct cleanup *make_cleanup_regcache_delete (regcache *regcache);

>> 

>> /* Return REGCACHE's ptid.  */

>> 

>> @@ -261,12 +258,7 @@ public:

>>  regcache (const regcache &) = delete;

>>  void operator= (const regcache &) = delete;

>> 

>> -  /* class regcache is only extended in unit test, so only mark it

>> -     virtual when selftest is enabled.  */

>> -#if GDB_SELF_TEST

>> -  virtual

>> -#endif

>> -  ~regcache ()

>> +  virtual ~regcache ()

> 

> This is not related to this patch.  It should be in patch #1.


Ok.

> 

> -- 

> Yao (齐尧)