From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.43]) by sourceware.org (Postfix) with ESMTPS id 75BE7384CBAF for ; Fri, 15 Dec 2023 18:13:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 75BE7384CBAF Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 75BE7384CBAF Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.221.43 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702663996; cv=none; b=dy7NlqtmKTPMPzePDSpYpMkODpSrmyl9W04jxdOwFVflkPdZjNb71TiwYgttucEPUo3HG3HvOYBOz4XDc1F02EGreSKS5ajZH8C1IpxnPo/iKOniYjdtGTSL0xQE1DR7OUNFlDQo47B+QZ5T7pktCZgkxE7MpYyB11EFWpJz9SQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702663996; c=relaxed/simple; bh=Jli5PJnJ0iI/9g/yfa+xF/7O3y1mNdyN4R1DhvqYCXk=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=RL+AGkdiveHQUfz4eV0Lalr5zjAfKOsO/OgPW+3vwc6Eh/b546e7CDiruqTGwmQGwfmqcVCZ5Przy2fd6HVB5N4LW7ADd0Ww17HMcEB+7D+APvhKIvfKkS1MvSfyv33vwT91Q8aANoLSpjP23PYb0tkxkFOJVk1btBhQ4QNBL1M= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wr1-f43.google.com with SMTP id ffacd0b85a97d-3363ebb277bso622407f8f.2 for ; Fri, 15 Dec 2023 10:13:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702663992; x=1703268792; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=MgDgSKPpwM98Z+uladhDrgloP9AXRVyCcWyaDRWiR+Q=; b=qVpJ7qfAfypJDSrVa95USBum1nbfHJeR6x6ujXgCEfjVMnQLwYsqsXYR+wfg0Ya+Nr bmPVjFP7fvEUunljcXMUw+dmtlKG8UlTVwHK9qH7KCZyGf8iZIhJVJUGdmDGSaDy/ia4 usg5JJZjqrnV0mKMH7gbalV1+VhyNxNVkGoKUr3V9Ug2CLLFnyFV5lt1I0yLgtVrYoAe BLUe1QFnQPg2aW7cWhiHsFPM0vPgHDsSNbCrWCcp8Na7+NkNRtWCaB7GfVlWVp2X1YYp E24Q2LQYD/qqaDhwqhA6yqw5ABDUf0juIM5jJcV+O/iNUqW7osWbq0VVLsySPpQnysps PhIw== X-Gm-Message-State: AOJu0YyVprzFm3ItNSaomBPyAMGTcvN49WGLO23nfNkuSY4FNV6QNVAC BDYhwGXFbfZehkZDecu7twOrPCP2BBggxw== X-Google-Smtp-Source: AGHT+IEEqL9CgQKvUS79InPcYNjj0U9loy/++tUJlPM4UBpeFHQXPEP7Vwsjy9qYgTTZVgCZxg/bSA== X-Received: by 2002:adf:da4c:0:b0:332:fe4b:ad5b with SMTP id r12-20020adfda4c000000b00332fe4bad5bmr6044469wrl.46.1702663991568; Fri, 15 Dec 2023 10:13:11 -0800 (PST) Received: from localhost ([2001:8a0:f923:4f00:43f0:3b0b:6626:3025]) by smtp.gmail.com with UTF8SMTPSA id df5-20020a5d5b85000000b003364a0e6983sm4546090wrb.62.2023.12.15.10.13.10 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 15 Dec 2023 10:13:11 -0800 (PST) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 2/2] Complete use of unique_ptr with notif_event and stop_reply Date: Fri, 15 Dec 2023 18:12:57 +0000 Message-ID: <20231215181257.1196830-3-pedro@palves.net> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20231215181257.1196830-1-pedro@palves.net> References: <20231215181257.1196830-1-pedro@palves.net> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 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 (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 (); 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