On 02/27/2019 08:18 PM, Tom Tromey wrote:
> This C++ifies the remote notification code -- replacing function
> pointers with virtual methods and using unique_ptr. This allows for
> the removal of some cleanups.
>
> gdb/ChangeLog
> 2019-02-27 Tom Tromey <tom@tromey.com>
>
> * remote.c (struct stop_reply_deleter): Remove.
> (stop_reply_up): Update.
> (struct stop_reply): Derive from notif_event. Don't typedef.
> <regcache>: Now a std::vector.
> (stop_reply_xfree): Remove.
> (stop_reply::~stop_reply): Rename from stop_reply_dtr.
> (remote_notif_stop_alloc_reply): Return a unique_ptr. Use new.
> (remote_target::discard_pending_stop_replies): Use delete.
> (remote_target::remote_parse_stop_reply): Update.
> (remote_target::process_stop_reply): Update.
> * remote-notif.h (struct notif_event): Add virtual destructor.
> Remove "dtr" member.
> (struct notif_client) <alloc_event>: Return a unique_ptr.
> (notif_event_xfree): Don't declare.
> * remote-notif.c (remote_notif_ack, remote_notif_parse): Update.
> (notif_event_xfree, do_notif_event_xfree): Remove.
> (remote_notif_state_xfree): Update.
> ---
> gdb/ChangeLog | 20 ++++++++++++
> gdb/remote-notif.c | 42 ++++--------------------
> gdb/remote-notif.h | 11 +++----
> gdb/remote.c | 80 +++++++++++++---------------------------------
> 4 files changed, 54 insertions(+), 99 deletions(-)
>
> diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
> index ae9a94d9c94..5a70ca128d5 100644
> --- a/gdb/remote-notif.c
> +++ b/gdb/remote-notif.c
> @@ -52,8 +52,6 @@ static struct notif_client *notifs[] =
>
> gdb_static_assert (ARRAY_SIZE (notifs) == REMOTE_NOTIF_LAST);
>
> -static void do_notif_event_xfree (void *arg);
> -
> /* Parse the BUF for the expected notification NC, and send packet to
> acknowledge. */
>
> @@ -61,18 +59,14 @@ void
> remote_notif_ack (remote_target *remote,
> struct notif_client *nc, const char *buf)
> {
> - struct notif_event *event = nc->alloc_event ();
> - struct cleanup *old_chain
> - = make_cleanup (do_notif_event_xfree, event);
> + std::unique_ptr<struct notif_event> event = nc->alloc_event ();
"std::unique_ptr<struct notif_event>" appears in a number of places
in the patch. Did you consider adding a "notif_event_up" typedef ?
> -typedef std::unique_ptr<stop_reply, stop_reply_deleter> stop_reply_up;
> +typedef std::unique_ptr<struct stop_reply> stop_reply_up;
Odd that you added the "struct". I tend to remove it when touching
code instead. :-)
Anyway, patch is OK.
Thanks,
Pedro Alves
@@ -52,8 +52,6 @@ static struct notif_client *notifs[] =
gdb_static_assert (ARRAY_SIZE (notifs) == REMOTE_NOTIF_LAST);
-static void do_notif_event_xfree (void *arg);
-
/* Parse the BUF for the expected notification NC, and send packet to
acknowledge. */
@@ -61,18 +59,14 @@ void
remote_notif_ack (remote_target *remote,
struct notif_client *nc, const char *buf)
{
- struct notif_event *event = nc->alloc_event ();
- struct cleanup *old_chain
- = make_cleanup (do_notif_event_xfree, event);
+ std::unique_ptr<struct notif_event> event = nc->alloc_event ();
if (notif_debug)
fprintf_unfiltered (gdb_stdlog, "notif: ack '%s'\n",
nc->ack_command);
- nc->parse (remote, nc, buf, event);
- nc->ack (remote, nc, buf, event);
-
- discard_cleanups (old_chain);
+ nc->parse (remote, nc, buf, event.get ());
+ nc->ack (remote, nc, buf, event.release ());
}
/* Parse the BUF for the expected notification NC. */
@@ -81,17 +75,14 @@ struct notif_event *
remote_notif_parse (remote_target *remote,
struct notif_client *nc, const char *buf)
{
- struct notif_event *event = nc->alloc_event ();
- struct cleanup *old_chain
- = make_cleanup (do_notif_event_xfree, event);
+ std::unique_ptr<struct notif_event> event = nc->alloc_event ();
if (notif_debug)
fprintf_unfiltered (gdb_stdlog, "notif: parse '%s'\n", nc->name);
- nc->parse (remote, nc, buf, event);
+ nc->parse (remote, nc, buf, event.get ());
- discard_cleanups (old_chain);
- return event;
+ return event.release ();
}
DEFINE_QUEUE_P (notif_client_p);
@@ -216,25 +207,6 @@ handle_notification (struct remote_notif_state *state, const char *buf)
}
}
-/* Invoke destructor of EVENT and xfree it. */
-
-void
-notif_event_xfree (struct notif_event *event)
-{
- if (event != NULL && event->dtr != NULL)
- event->dtr (event);
-
- xfree (event);
-}
-
-/* Cleanup wrapper. */
-
-static void
-do_notif_event_xfree (void *arg)
-{
- notif_event_xfree ((struct notif_event *) arg);
-}
-
/* Return an allocated remote_notif_state. */
struct remote_notif_state *
@@ -269,7 +241,7 @@ remote_notif_state_xfree (struct remote_notif_state *state)
delete_async_event_handler (&state->get_pending_events_token);
for (i = 0; i < REMOTE_NOTIF_LAST; i++)
- notif_event_xfree (state->pending_event[i]);
+ delete state->pending_event[i];
xfree (state);
}
@@ -20,15 +20,16 @@
#ifndef REMOTE_NOTIF_H
#define REMOTE_NOTIF_H
+#include <memory>
#include "common/queue.h"
/* An event of a type of async remote notification. */
struct notif_event
{
- /* Destructor. Release everything from SELF, but not SELF
- itself. */
- void (*dtr) (struct notif_event *self);
+ virtual ~notif_event ()
+ {
+ }
};
/* ID of the notif_client. */
@@ -70,7 +71,7 @@ typedef struct notif_client
struct notif_client *self);
/* Allocate an event. */
- struct notif_event *(*alloc_event) (void);
+ std::unique_ptr<struct notif_event> (*alloc_event) ();
/* Id of this notif_client. */
const enum REMOTE_NOTIF_ID id;
@@ -112,8 +113,6 @@ struct notif_event *remote_notif_parse (remote_target *remote,
notif_client *nc,
const char *buf);
-void notif_event_xfree (struct notif_event *event);
-
void handle_notification (struct remote_notif_state *notif_state,
const char *buf);
@@ -96,17 +96,7 @@ struct protocol_feature;
struct packet_reg;
struct stop_reply;
-static void stop_reply_xfree (struct stop_reply *);
-
-struct stop_reply_deleter
-{
- void operator() (stop_reply *r) const
- {
- stop_reply_xfree (r);
- }
-};
-
-typedef std::unique_ptr<stop_reply, stop_reply_deleter> stop_reply_up;
+typedef std::unique_ptr<struct stop_reply> stop_reply_up;
/* Generic configuration support for packets the stub optionally
supports. Allows the user to specify the use of the packet as well
@@ -6815,11 +6805,9 @@ remote_console_output (const char *msg)
gdb_flush (gdb_stdtarg);
}
-DEF_VEC_O(cached_reg_t);
-
-typedef struct stop_reply
+struct stop_reply : public notif_event
{
- struct notif_event base;
+ ~stop_reply ();
/* The identifier of the thread about this event */
ptid_t ptid;
@@ -6838,20 +6826,14 @@ typedef struct stop_reply
efficient for those targets that provide critical registers as
part of their normal status mechanism (as another roundtrip to
fetch them is avoided). */
- VEC(cached_reg_t) *regcache;
+ std::vector<cached_reg_t> regcache;
enum target_stop_reason stop_reason;
CORE_ADDR watch_data_address;
int core;
-} *stop_reply_p;
-
-static void
-stop_reply_xfree (struct stop_reply *r)
-{
- notif_event_xfree ((struct notif_event *) r);
-}
+};
/* Return the length of the stop reply queue. */
@@ -6903,30 +6885,16 @@ remote_notif_stop_can_get_pending_events (remote_target *remote,
return 0;
}
-static void
-stop_reply_dtr (struct notif_event *event)
+stop_reply::~stop_reply ()
{
- struct stop_reply *r = (struct stop_reply *) event;
- cached_reg_t *reg;
- int ix;
-
- for (ix = 0;
- VEC_iterate (cached_reg_t, r->regcache, ix, reg);
- ix++)
- xfree (reg->data);
-
- VEC_free (cached_reg_t, r->regcache);
+ for (cached_reg_t ® : regcache)
+ xfree (reg.data);
}
-static struct notif_event *
-remote_notif_stop_alloc_reply (void)
+static std::unique_ptr<struct notif_event>
+remote_notif_stop_alloc_reply ()
{
- /* We cast to a pointer to the "base class". */
- struct notif_event *r = (struct notif_event *) XNEW (struct stop_reply);
-
- r->dtr = stop_reply_dtr;
-
- return r;
+ return std::unique_ptr<struct notif_event> (new struct stop_reply ());
}
/* A client of notification Stop. */
@@ -7069,7 +7037,7 @@ remote_target::discard_pending_stop_replies (struct inferior *inf)
/* Discard the in-flight notification. */
if (reply != NULL && reply->ptid.pid () == inf->pid)
{
- stop_reply_xfree (reply);
+ delete reply;
rns->pending_event[notif_client_stop.id] = NULL;
}
@@ -7213,7 +7181,7 @@ remote_target::remote_parse_stop_reply (const char *buf, stop_reply *event)
event->ws.kind = TARGET_WAITKIND_IGNORE;
event->ws.value.integer = 0;
event->stop_reason = TARGET_STOPPED_BY_NO_REASON;
- event->regcache = NULL;
+ event->regcache.clear ();
event->core = -1;
switch (buf[0])
@@ -7449,7 +7417,7 @@ Packet: '%s'\n"),
if (fieldsize < register_size (event->arch, reg->regnum))
warning (_("Remote reply is too short: %s"), buf);
- VEC_safe_push (cached_reg_t, event->regcache, &cached_reg);
+ event->regcache.push_back (cached_reg);
}
else
{
@@ -7665,22 +7633,18 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
&& status->kind != TARGET_WAITKIND_NO_RESUMED)
{
/* Expedited registers. */
- if (stop_reply->regcache)
+ if (!stop_reply->regcache.empty ())
{
struct regcache *regcache
= get_thread_arch_regcache (ptid, stop_reply->arch);
- cached_reg_t *reg;
- int ix;
- for (ix = 0;
- VEC_iterate (cached_reg_t, stop_reply->regcache, ix, reg);
- ix++)
- {
- regcache->raw_supply (reg->num, reg->data);
- xfree (reg->data);
- }
+ for (cached_reg_t ® : stop_reply->regcache)
+ {
+ regcache->raw_supply (reg.num, reg.data);
+ xfree (reg.data);
+ }
- VEC_free (cached_reg_t, stop_reply->regcache);
+ stop_reply->regcache.clear ();
}
remote_notice_new_inferior (ptid, 0);
@@ -7691,7 +7655,7 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
remote_thr->vcont_resumed = 0;
}
- stop_reply_xfree (stop_reply);
+ delete stop_reply;
return ptid;
}