* [PATCH 1/2] Make cached_reg_t own its data
2023-12-15 18:12 [PATCH 0/2] Use unique_ptr more in the remote target Pedro Alves
@ 2023-12-15 18:12 ` Pedro Alves
2023-12-20 23:48 ` Simon Marchi
2023-12-15 18:12 ` [PATCH 2/2] Complete use of unique_ptr with notif_event and stop_reply Pedro Alves
2023-12-15 19:10 ` [PATCH 0/2] Use unique_ptr more in the remote target Tom Tromey
2 siblings, 1 reply; 8+ messages in thread
From: Pedro Alves @ 2023-12-15 18:12 UTC (permalink / raw)
To: gdb-patches
struct cached_reg_t own its data buffer, but currently that is managed
manually. Convert it to use a unique_xmalloc_ptr.
Change-Id: I05a107098b717299e76de76aaba00d7fbaeac77b
---
gdb/python/py-unwind.c | 13 ++++++-------
gdb/regcache.h | 5 ++++-
gdb/remote.c | 24 ++++++------------------
3 files changed, 16 insertions(+), 26 deletions(-)
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index f1162f22290..31b74c67310 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -785,7 +785,7 @@ pyuw_prev_register (frame_info_ptr this_frame, void **cache_ptr,
for (; reg_info < reg_info_end; ++reg_info)
{
if (regnum == reg_info->num)
- return frame_unwind_got_bytes (this_frame, regnum, reg_info->data);
+ return frame_unwind_got_bytes (this_frame, regnum, reg_info->data.get ());
}
return frame_unwind_got_optimized (this_frame, regnum);
@@ -903,15 +903,14 @@ pyuw_sniffer (const struct frame_unwind *self, frame_info_ptr this_frame,
struct value *value = value_object_to_value (reg->value.get ());
size_t data_size = register_size (gdbarch, reg->number);
- cached_frame->reg[i].num = reg->number;
-
/* `value' validation was done before, just assert. */
gdb_assert (value != NULL);
gdb_assert (data_size == value->type ()->length ());
- cached_frame->reg[i].data = (gdb_byte *) xmalloc (data_size);
- memcpy (cached_frame->reg[i].data,
- value->contents ().data (), data_size);
+ cached_reg_t *cached = new (&cached_frame->reg[i]) cached_reg_t ();
+ cached->num = reg->number;
+ cached->data.reset ((gdb_byte *) xmalloc (data_size));
+ memcpy (cached->data.get (), value->contents ().data (), data_size);
}
}
@@ -928,7 +927,7 @@ pyuw_dealloc_cache (frame_info *this_frame, void *cache)
cached_frame_info *cached_frame = (cached_frame_info *) cache;
for (int i = 0; i < cached_frame->reg_count; i++)
- xfree (cached_frame->reg[i].data);
+ cached_frame->reg[i].~cached_reg_t ();
xfree (cache);
}
diff --git a/gdb/regcache.h b/gdb/regcache.h
index d90f74bfbb0..85890b66cbc 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -176,7 +176,10 @@ using register_read_ftype
struct cached_reg_t
{
int num;
- gdb_byte *data;
+ gdb::unique_xmalloc_ptr<gdb_byte> data;
+
+ cached_reg_t () = default;
+ cached_reg_t (cached_reg_t &&rhs) = default;
};
/* Buffer of registers. */
diff --git a/gdb/remote.c b/gdb/remote.c
index 84daa8567b6..7611396c949 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1353,8 +1353,6 @@ class extended_remote_target final : public remote_target
struct stop_reply : public notif_event
{
- ~stop_reply ();
-
/* The identifier of the thread about this event */
ptid_t ptid;
@@ -7604,12 +7602,6 @@ remote_notif_stop_can_get_pending_events (remote_target *remote,
return 0;
}
-stop_reply::~stop_reply ()
-{
- for (cached_reg_t ® : regcache)
- xfree (reg.data);
-}
-
static notif_event_up
remote_notif_stop_alloc_reply ()
{
@@ -8094,17 +8086,18 @@ Packet: '%s'\n"),
hex_string (pnum), p, buf);
cached_reg.num = reg->regnum;
- cached_reg.data = (gdb_byte *)
- xmalloc (register_size (event->arch, reg->regnum));
+ cached_reg.data.reset ((gdb_byte *)
+ xmalloc (register_size (event->arch,
+ reg->regnum)));
p = p1 + 1;
- fieldsize = hex2bin (p, cached_reg.data,
+ fieldsize = hex2bin (p, cached_reg.data.get (),
register_size (event->arch, reg->regnum));
p += 2 * fieldsize;
if (fieldsize < register_size (event->arch, reg->regnum))
warning (_("Remote reply is too short: %s"), buf);
- event->regcache.push_back (cached_reg);
+ event->regcache.push_back (std::move (cached_reg));
}
else
{
@@ -8436,12 +8429,7 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
stop_reply->arch);
for (cached_reg_t ® : stop_reply->regcache)
- {
- regcache->raw_supply (reg.num, reg.data);
- xfree (reg.data);
- }
-
- stop_reply->regcache.clear ();
+ regcache->raw_supply (reg.num, reg.data.get ());
}
remote_thread_info *remote_thr = get_remote_thread_info (this, ptid);
--
2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 2/2] Complete use of unique_ptr with notif_event and stop_reply
2023-12-15 18:12 [PATCH 0/2] Use unique_ptr more in the remote target Pedro Alves
2023-12-15 18:12 ` [PATCH 1/2] Make cached_reg_t own its data Pedro Alves
@ 2023-12-15 18:12 ` Pedro Alves
2023-12-15 19:10 ` [PATCH 0/2] Use unique_ptr more in the remote target Tom Tromey
2 siblings, 0 replies; 8+ messages in thread
From: Pedro Alves @ 2023-12-15 18:12 UTC (permalink / raw)
To: gdb-patches
We already use unique_ptr with notif_event and stop_reply in some
places around the remote target, but not fully. There are several
code paths that still use raw pointers. This commit makes all of the
ownership of these objects tracked by unique pointers, making the
lifetime flow much more obvious, IMHO.
I notice that it fixes a leak -- in remote_notif_stop_ack, We weren't
destroying the stop_reply object if it was of TARGET_WAITKIND_IGNORE
kind.
Change-Id: Id81daf39653d8792c8795b2a145772176bfae77c
---
gdb/remote-notif.c | 15 +++------
gdb/remote-notif.h | 10 +++---
gdb/remote.c | 84 ++++++++++++++++++++++++----------------------
3 files changed, 54 insertions(+), 55 deletions(-)
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index 997b6e7ae26..e932d6ab46a 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -67,12 +67,12 @@ remote_notif_ack (remote_target *remote,
nc->ack_command);
nc->parse (remote, nc, buf, event.get ());
- nc->ack (remote, nc, buf, event.release ());
+ nc->ack (remote, nc, buf, std::move (event));
}
/* Parse the BUF for the expected notification NC. */
-struct notif_event *
+notif_event_up
remote_notif_parse (remote_target *remote,
const notif_client *nc, const char *buf)
{
@@ -83,7 +83,7 @@ remote_notif_parse (remote_target *remote,
nc->parse (remote, nc, buf, event.get ());
- return event.release ();
+ return event;
}
/* Process notifications in STATE's notification queue one by one.
@@ -150,12 +150,12 @@ handle_notification (struct remote_notif_state *state, const char *buf)
}
else
{
- struct notif_event *event
+ notif_event_up event
= remote_notif_parse (state->remote, nc, buf + strlen (nc->name) + 1);
/* Be careful to only set it after parsing, since an error
may be thrown then. */
- state->pending_event[nc->id] = event;
+ state->pending_event[nc->id] = std::move (event);
/* Notify the event loop there's a stop reply to acknowledge
and that there may be more events to fetch. */
@@ -230,14 +230,9 @@ remote_notif_state_allocate (remote_target *remote)
remote_notif_state::~remote_notif_state ()
{
- int i;
-
/* Unregister async_event_handler for notification. */
if (get_pending_events_token != NULL)
delete_async_event_handler (&get_pending_events_token);
-
- for (i = 0; i < REMOTE_NOTIF_LAST; i++)
- delete pending_event[i];
}
void _initialize_notif ();
diff --git a/gdb/remote-notif.h b/gdb/remote-notif.h
index c95e1f6ff14..3b09589d495 100644
--- a/gdb/remote-notif.h
+++ b/gdb/remote-notif.h
@@ -67,7 +67,7 @@ struct notif_client
something wrong, throw an exception. */
void (*ack) (remote_target *remote,
const notif_client *self, const char *buf,
- struct notif_event *event);
+ notif_event_up event);
/* Check this notification client can get pending events in
'remote_notif_process'. */
@@ -111,14 +111,14 @@ struct remote_notif_state
this notification (which is done by
remote.c:remote_notif_pending_replies). */
- struct notif_event *pending_event[REMOTE_NOTIF_LAST] {};
+ notif_event_up pending_event[REMOTE_NOTIF_LAST];
};
void remote_notif_ack (remote_target *remote, const notif_client *nc,
const char *buf);
-struct notif_event *remote_notif_parse (remote_target *remote,
- const notif_client *nc,
- const char *buf);
+notif_event_up remote_notif_parse (remote_target *remote,
+ const notif_client *nc,
+ const char *buf);
void handle_notification (struct remote_notif_state *notif_state,
const char *buf);
diff --git a/gdb/remote.c b/gdb/remote.c
index 7611396c949..f3823bb9c76 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1110,7 +1110,7 @@ class remote_target : public process_stratum_target
ptid_t wait_as (ptid_t ptid, target_waitstatus *status,
target_wait_flags options);
- ptid_t process_stop_reply (struct stop_reply *stop_reply,
+ ptid_t process_stop_reply (stop_reply_up stop_reply,
target_waitstatus *status);
ptid_t select_thread_for_ambiguous_stop_reply
@@ -1137,8 +1137,8 @@ class remote_target : public process_stratum_target
(bool *may_global_wildcard_vcont);
void discard_pending_stop_replies_in_queue ();
- struct stop_reply *remote_notif_remove_queued_reply (ptid_t ptid);
- struct stop_reply *queued_stop_reply (ptid_t ptid);
+ stop_reply_up remote_notif_remove_queued_reply (ptid_t ptid);
+ stop_reply_up queued_stop_reply (ptid_t ptid);
int peek_stop_reply (ptid_t ptid);
void remote_parse_stop_reply (const char *buf, stop_reply *event);
@@ -1305,7 +1305,7 @@ class remote_target : public process_stratum_target
ULONGEST *xfered_len,
const unsigned int which_packet);
- void push_stop_reply (struct stop_reply *new_event);
+ void push_stop_reply (stop_reply_up new_event);
bool vcont_r_supported ();
@@ -4973,6 +4973,16 @@ struct scoped_mark_target_starting
scoped_restore_tmpl<bool> m_restore_starting_up;
};
+/* Transfer ownership of the stop_reply owned by EVENT to a
+ stop_reply_up object. */
+
+static stop_reply_up
+as_stop_reply_up (notif_event_up event)
+{
+ auto *stop_reply = static_cast<struct stop_reply *> (event.release ());
+ return stop_reply_up (stop_reply);
+}
+
/* Helper for remote_target::start_remote, start the remote connection and
sync state. Return true if everything goes OK, otherwise, return false.
This function exists so that the scoped_restore created within it will
@@ -5214,9 +5224,9 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
/* Use the previously fetched status. */
gdb_assert (wait_status != NULL);
- struct notif_event *reply
+ notif_event_up reply
= remote_notif_parse (this, ¬if_client_stop, wait_status);
- push_stop_reply ((struct stop_reply *) reply);
+ push_stop_reply (as_stop_reply_up (std::move (reply)));
::start_remote (from_tty); /* Initialize gdb process mechanisms. */
}
@@ -6551,10 +6561,9 @@ extended_remote_target::attach (const char *args, int from_tty)
/* Use the previously fetched status. */
gdb_assert (wait_status != NULL);
- struct notif_event *reply
- = remote_notif_parse (this, ¬if_client_stop, wait_status);
-
- push_stop_reply ((struct stop_reply *) reply);
+ notif_event_up reply
+ = remote_notif_parse (this, ¬if_client_stop, wait_status);
+ push_stop_reply (as_stop_reply_up (std::move (reply)));
}
else
{
@@ -7335,7 +7344,7 @@ remote_target::remote_stop_ns (ptid_t ptid)
= remote_thr->resumed_pending_vcont_info ();
gdb_assert (info.sig == GDB_SIGNAL_0);
- stop_reply *sr = new stop_reply ();
+ stop_reply_up sr = std::make_unique<stop_reply> ();
sr->ptid = tp->ptid;
sr->rs = rs;
sr->ws.set_stopped (GDB_SIGNAL_0);
@@ -7343,7 +7352,7 @@ remote_target::remote_stop_ns (ptid_t ptid)
sr->stop_reason = TARGET_STOPPED_BY_NO_REASON;
sr->watch_data_address = 0;
sr->core = 0;
- this->push_stop_reply (sr);
+ this->push_stop_reply (std::move (sr));
/* Pretend that this thread was actually resumed on the
remote target, then stopped. If we leave it in the
@@ -7574,9 +7583,9 @@ remote_notif_stop_parse (remote_target *remote,
static void
remote_notif_stop_ack (remote_target *remote,
const notif_client *self, const char *buf,
- struct notif_event *event)
+ notif_event_up event)
{
- struct stop_reply *stop_reply = (struct stop_reply *) event;
+ stop_reply_up stop_reply = as_stop_reply_up (std::move (event));
/* acknowledge */
putpkt (remote, self->ack_command);
@@ -7585,7 +7594,7 @@ remote_notif_stop_ack (remote_target *remote,
the notification. It was left in the queue because we need to
acknowledge it and pull the rest of the notifications out. */
if (stop_reply->ws.kind () != TARGET_WAITKIND_IGNORE)
- remote->push_stop_reply (stop_reply);
+ remote->push_stop_reply (std::move (stop_reply));
}
static int
@@ -7696,7 +7705,6 @@ remote_target::check_pending_events_prevent_wildcard_vcont
void
remote_target::discard_pending_stop_replies (struct inferior *inf)
{
- struct stop_reply *reply;
struct remote_state *rs = get_remote_state ();
struct remote_notif_state *rns = rs->notif_state;
@@ -7705,7 +7713,8 @@ remote_target::discard_pending_stop_replies (struct inferior *inf)
if (rs->remote_desc == NULL)
return;
- reply = (struct stop_reply *) rns->pending_event[notif_client_stop.id];
+ stop_reply_up reply
+ = as_stop_reply_up (std::move (rns->pending_event[notif_client_stop.id]));
/* Discard the in-flight notification. */
if (reply != NULL && reply->ptid.pid () == inf->pid)
@@ -7757,7 +7766,7 @@ remote_target::discard_pending_stop_replies_in_queue ()
/* Remove the first reply in 'stop_reply_queue' which matches
PTID. */
-struct stop_reply *
+stop_reply_up
remote_target::remote_notif_remove_queued_reply (ptid_t ptid)
{
remote_state *rs = get_remote_state ();
@@ -7768,12 +7777,10 @@ remote_target::remote_notif_remove_queued_reply (ptid_t ptid)
{
return event->ptid.matches (ptid);
});
- struct stop_reply *result;
- if (iter == rs->stop_reply_queue.end ())
- result = nullptr;
- else
+ stop_reply_up result;
+ if (iter != rs->stop_reply_queue.end ())
{
- result = iter->release ();
+ result = std::move (*iter);
rs->stop_reply_queue.erase (iter);
}
@@ -7790,11 +7797,11 @@ remote_target::remote_notif_remove_queued_reply (ptid_t ptid)
found. If there are still queued events left to process, tell the
event loop to get back to target_wait soon. */
-struct stop_reply *
+stop_reply_up
remote_target::queued_stop_reply (ptid_t ptid)
{
remote_state *rs = get_remote_state ();
- struct stop_reply *r = remote_notif_remove_queued_reply (ptid);
+ stop_reply_up r = remote_notif_remove_queued_reply (ptid);
if (!rs->stop_reply_queue.empty () && target_can_async_p ())
{
@@ -7810,10 +7817,10 @@ remote_target::queued_stop_reply (ptid_t ptid)
core side, tell the event loop to get back to target_wait soon. */
void
-remote_target::push_stop_reply (struct stop_reply *new_event)
+remote_target::push_stop_reply (stop_reply_up new_event)
{
remote_state *rs = get_remote_state ();
- rs->stop_reply_queue.push_back (stop_reply_up (new_event));
+ rs->stop_reply_queue.push_back (std::move (new_event));
if (notif_debug)
gdb_printf (gdb_stdlog,
@@ -8253,8 +8260,7 @@ remote_target::remote_notif_get_pending_events (const notif_client *nc)
/* acknowledge */
nc->ack (this, nc, rs->buf.data (),
- rs->notif_state->pending_event[nc->id]);
- rs->notif_state->pending_event[nc->id] = NULL;
+ std::move (rs->notif_state->pending_event[nc->id]));
while (1)
{
@@ -8398,7 +8404,7 @@ remote_target::select_thread_for_ambiguous_stop_reply
destroys STOP_REPLY. */
ptid_t
-remote_target::process_stop_reply (struct stop_reply *stop_reply,
+remote_target::process_stop_reply (stop_reply_up stop_reply,
struct target_waitstatus *status)
{
*status = stop_reply->ws;
@@ -8452,7 +8458,6 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
}
}
- delete stop_reply;
return ptid;
}
@@ -8463,7 +8468,6 @@ remote_target::wait_ns (ptid_t ptid, struct target_waitstatus *status,
target_wait_flags options)
{
struct remote_state *rs = get_remote_state ();
- struct stop_reply *stop_reply;
int ret;
bool is_notif = false;
@@ -8496,9 +8500,9 @@ remote_target::wait_ns (ptid_t ptid, struct target_waitstatus *status,
remote_notif_get_pending_events (¬if_client_stop);
/* If indeed we noticed a stop reply, we're done. */
- stop_reply = queued_stop_reply (ptid);
+ stop_reply_up stop_reply = queued_stop_reply (ptid);
if (stop_reply != NULL)
- return process_stop_reply (stop_reply, status);
+ return process_stop_reply (std::move (stop_reply), status);
/* Still no event. If we're just polling for an event, then
return to the event loop. */
@@ -8534,7 +8538,7 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
struct remote_state *rs = get_remote_state ();
ptid_t event_ptid = null_ptid;
char *buf;
- struct stop_reply *stop_reply;
+ stop_reply_up stop_reply;
again:
@@ -8546,7 +8550,7 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
/* None of the paths that push a stop reply onto the queue should
have set the waiting_for_stop_reply flag. */
gdb_assert (!rs->waiting_for_stop_reply);
- event_ptid = process_stop_reply (stop_reply, status);
+ event_ptid = process_stop_reply (std::move (stop_reply), status);
}
else
{
@@ -8609,11 +8613,11 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
rs->waiting_for_stop_reply = 0;
stop_reply
- = (struct stop_reply *) remote_notif_parse (this,
- ¬if_client_stop,
- rs->buf.data ());
+ = as_stop_reply_up (remote_notif_parse (this,
+ ¬if_client_stop,
+ rs->buf.data ()));
- event_ptid = process_stop_reply (stop_reply, status);
+ event_ptid = process_stop_reply (std::move (stop_reply), status);
break;
}
case 'O': /* Console output. */
--
2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread