This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/7]: Regcache: Remove xmalloc/xfree methods
- From: Alan Hayward <Alan dot Hayward at arm dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, nd <nd at arm dot com>
- Date: Wed, 23 Aug 2017 13:42:48 +0000
- Subject: Re: [PATCH 3/7]: Regcache: Remove xmalloc/xfree methods
- Authentication-results: sourceware.org; auth=none
- Authentication-results: spf=none (sender IP is ) smtp.mailfrom=Alan dot Hayward at arm dot com;
- Nodisclaimer: True
- References: <08C93960-8ED8-431A-B786-3455FF149B77@arm.com> <86ziaqr5mi.fsf@gmail.com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
> 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 (齐尧)