From: Yao Qi <qiyaoltc@gmail.com>
To: Alan Hayward <Alan.Hayward@arm.com>
Cc: "gdb-patches\@sourceware.org" <gdb-patches@sourceware.org>,
nd <nd@arm.com>
Subject: Re: [PATCH 3/7]: Regcache: Remove xmalloc/xfree methods
Date: Wed, 23 Aug 2017 09:33:00 -0000 [thread overview]
Message-ID: <86ziaqr5mi.fsf@gmail.com> (raw)
In-Reply-To: <08C93960-8ED8-431A-B786-3455FF149B77@arm.com> (Alan Hayward's message of "Thu, 17 Aug 2017 08:47:54 +0000")
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.
--
Yao (齐尧)
next prev parent reply other threads:[~2017-08-23 9:33 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-17 8:48 Alan Hayward
2017-08-23 9:33 ` Yao Qi [this message]
2017-08-23 13:42 ` Alan Hayward
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86ziaqr5mi.fsf@gmail.com \
--to=qiyaoltc@gmail.com \
--cc=Alan.Hayward@arm.com \
--cc=gdb-patches@sourceware.org \
--cc=nd@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).