[PATCH 0/2] Use unique_ptr more in the remote target

[PATCH 0/2] Use unique_ptr more in the remote target

[Pedro Alves]

A couple cleanup patches improving the ownership tracking in the
remote target.

Pedro Alves (2):
  Make cached_reg_t own its data
  Complete use of unique_ptr with notif_event and stop_reply

 gdb/python/py-unwind.c |  13 +++--
 gdb/regcache.h         |   5 +-
 gdb/remote-notif.c     |  15 ++----
 gdb/remote-notif.h     |  10 ++--
 gdb/remote.c           | 108 +++++++++++++++++++----------------------
 5 files changed, 70 insertions(+), 81 deletions(-)


base-commit: c5a473d789d815c8f03f3ef9fc6df5ccfc40a468
-- 
2.43.0

[PATCH 1/2] Make cached_reg_t own its data

[Pedro Alves]

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 &reg : 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 &reg : 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

[PATCH 2/2] Complete use of unique_ptr with notif_event and stop_reply

[Pedro Alves]

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, &notif_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, &notif_client_stop, wait_status);
-
-      push_stop_reply ((struct stop_reply *) reply);
+      notif_event_up reply
+	= remote_notif_parse (this, &notif_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 (&notif_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,
-							  &notif_client_stop,
-							  rs->buf.data ());
+	      = as_stop_reply_up (remote_notif_parse (this,
+						      &notif_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

Re: [PATCH 0/2] Use unique_ptr more in the remote target

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> A couple cleanup patches improving the ownership tracking in the
Pedro> remote target.

Thanks.  These look good to me.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

[pushed] Fix bug in previous remote unique_ptr change (Re: [PATCH 0/2] Use unique_ptr more in the remote target)

[Pedro Alves]

On 2023-12-15 19:10, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> A couple cleanup patches improving the ownership tracking in the
> Pedro> remote target.
> 
> Thanks.  These look good to me.
> Approved-By: Tom Tromey <tom@tromey.com>

Thanks.  I pushed it, and then immediately afterwards, while looking at the patch
again I noticed a problem... :-/  I've pushed this follow up patch as well.

From aed77b16f17fc3ed9c952af632674dc25d0dfdb5 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 20 Dec 2023 20:11:23 +0000
Subject: [PATCH] Fix bug in previous remote unique_ptr change

By inspection, I noticed that the previous patch went too far, here:

 @@ -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)

That is always moving the stop reply from pending_event, when we only
really want to peek into it.  The code further below that even says:

  /* Discard the in-flight notification.  */
  if (reply != NULL && reply->ptid.pid () == inf->pid)
    {
      /* Leave the notification pending, since the server expects that
	 we acknowledge it with vStopped.  But clear its contents, so
	 that later on when we acknowledge it, we also discard it.  */

This commit reverts that hunk back, adjusted to use unique_ptr::get().

Change-Id: Ifc809d1a8225150a4656889f056d51267100ee24
---
 gdb/remote.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index f3823bb9c76..dcc1a0d0639 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7713,8 +7713,9 @@ remote_target::discard_pending_stop_replies (struct inferior *inf)
   if (rs->remote_desc == NULL)
     return;
 
-  stop_reply_up reply
-    = as_stop_reply_up (std::move (rns->pending_event[notif_client_stop.id]));
+  struct notif_event *notif_event
+    = rns->pending_event[notif_client_stop.id].get ();
+  auto *reply = static_cast<stop_reply *> (notif_event);
 
   /* Discard the in-flight notification.  */
   if (reply != NULL && reply->ptid.pid () == inf->pid)

base-commit: e21633812306a23d454e1a63fa57a5b689cddcbb
-- 
2.43.0

Re: [pushed] Fix bug in previous remote unique_ptr change (Re: [PATCH 0/2] Use unique_ptr more in the remote target)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> -  stop_reply_up reply
Pedro> -    = as_stop_reply_up (std::move (rns->pending_event[notif_client_stop.id]));
Pedro> +  struct notif_event *notif_event
Pedro> +    = rns->pending_event[notif_client_stop.id].get ();
Pedro> +  auto *reply = static_cast<stop_reply *> (notif_event);
 
I wonder if you want checked_static_cast here.

Tom

Re: [PATCH 1/2] Make cached_reg_t own its data

[Simon Marchi]

On 2023-12-15 13:12, Pedro Alves wrote:
> 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

Hi Pedro,

When building with clang 17:

  CXX    python/py-unwind.o
/home/smarchi/src/binutils-gdb/gdb/python/py-unwind.c:126:16: error: flexible array member 'reg' of type 'cached_reg_t[]' with non-trivial destruction
  126 |   cached_reg_t reg[];
      |                ^


Simon

Re: [PATCH 1/2] Make cached_reg_t own its data

[Pedro Alves]

On 2023-12-20 23:48, Simon Marchi wrote:
> 
> 
> On 2023-12-15 13:12, Pedro Alves wrote:
>> 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
> 
> Hi Pedro,
> 
> When building with clang 17:
> 
>   CXX    python/py-unwind.o
> /home/smarchi/src/binutils-gdb/gdb/python/py-unwind.c:126:16: error: flexible array member 'reg' of type 'cached_reg_t[]' with non-trivial destruction
>   126 |   cached_reg_t reg[];
>       |                ^
> 

Bah.  Really Clang?  Not even a warning instead of an error?

I'm going to apply this to unbreak the build ASAP, but maybe we should really get
rid of this flexible array member and C++-fy the whole of struct cached_frame_info.

From bfcfa995f9461726d57f0d9a2879ba4352547870 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 21 Dec 2023 10:43:20 +0000
Subject: [PATCH] Fix Clang build issue with flexible array member and
 non-trivial dtor

Commit d5cebea18e7a ("Make cached_reg_t own its data") added a
destructor to cached_reg_t.

That caused a build problem with Clang, which errors out like so:

 > CXX    python/py-unwind.o
 > gdb/python/py-unwind.c:126:16: error: flexible array member 'reg' of type 'cached_reg_t[]' with non-trivial destruction
 >   126 |   cached_reg_t reg[];
 >       |                ^

This is is not really a problem for our code, which allocates the
whole structure with xmalloc, and then initializes the array elements
with in-place new, and then takes care to call the destructor
manually.  Like, commit d5cebea18e7a did:

 @@ -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 ();

Maybe we should get rid of the flexible array member and use a bog
standard std::vector.  I doubt this would cause any visible
performance issue.

Meanwhile, to unbreak the build, this commit switches from C99-style
flexible array member to 0-length array.  It behaves the same, and
Clang doesn't complain.  I got the idea from here:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70932#c11

GCC 9, our oldest support version, already supported this:

  https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Zero-Length.html

but the extension is actually much older than that.  Note that
C99-style flexible array members are not standard C++ either.

Change-Id: I37dda18f367e238a41d610619935b2a0f2acacce
---
 gdb/python/py-unwind.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 31b74c67310..8fed55beadc 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -123,7 +123,15 @@ struct cached_frame_info
   /* Length of the `reg' array below.  */
   int reg_count;
 
-  cached_reg_t reg[];
+  /* Flexible array member.  Note: use a zero-sized array rather than
+     an actual C99-style flexible array member (unsized array),
+     because the latter would cause an error with Clang:
+
+       error: flexible array member 'reg' of type 'cached_reg_t[]' with non-trivial destruction
+
+     Note we manually call the destructor of each array element in
+     pyuw_dealloc_cache.  */
+  cached_reg_t reg[0];
 };
 
 extern PyTypeObject pending_frame_object_type

base-commit: 3a4ee6286814e850f66d84b6b8b18cd053649d35
-- 
2.43.0