diff mbox series

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

Message ID 08f2de94-a450-4bd3-9c3f-2f3916a7cb02@palves.net
State New
Headers
Series [pushed] Fix bug in previous remote unique_ptr change (Re: [PATCH 0/2] Use unique_ptr more in the remote target) |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 warning Patch is already merged
linaro-tcwg-bot/tcwg_gdb_build--master-arm warning Patch is already merged

Commit Message

Pedro Alves Dec. 20, 2023, 8:27 p.m. UTC 
  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(-)


base-commit: e21633812306a23d454e1a63fa57a5b689cddcbb

Comments

Tom Tromey Dec. 20, 2023, 11:30 p.m. UTC | #1 
>>>>> "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
diff mbox series

Patch

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)