From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 24AC13858D1E for ; Mon, 9 Oct 2023 09:11:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 24AC13858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1696842687; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=aZo70swDDcGBhxK8xBE35ydS7z2wsq1kuCh5HT2yf/I=; b=SMcQKYyUsspoLkBWJ9oNu3G6N9TI6x8VNlEPxapK0AF70FtAS5LZdMDnrfsdB/Zw8kCUg9 IJLgUh5YUAZU38bz6lpWi6RlE9vrTQNzJYqneT6gOD2PSg4dyWe+1sziR5wBa6JHbTFL2q e4Qh7S36EcwB5QsLUTL13CDWRLOnaXU= Received: from mail-ej1-f72.google.com (mail-ej1-f72.google.com [209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-412-1JoN8djaNvyHobOAnPhqSQ-1; Mon, 09 Oct 2023 05:11:06 -0400 X-MC-Unique: 1JoN8djaNvyHobOAnPhqSQ-1 Received: by mail-ej1-f72.google.com with SMTP id a640c23a62f3a-99c8bbc902eso349892366b.1 for ; Mon, 09 Oct 2023 02:11:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696842665; x=1697447465; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=aZo70swDDcGBhxK8xBE35ydS7z2wsq1kuCh5HT2yf/I=; b=mMk1SLpoGW9I5YU1kdJUv41dye+o75hxgT/XM9yViWTgooE0BMeY5H0WHqaXAF2O1B 23ueZrem0pKI1vvTjrVTmXMTnBPKptVgK0zXGY4iw402KviT1J2v8WBiVAF+m8rhdIcW WOWicS2ICN6Krs/HIdhowBlqDRh+nrDTWRy4LUyjZCgLNezJefNlxIrRxWplCth61n1U vV2lnhHEkABOPjsGWP56lUUHNSjoXUboJzOQxZY4BZO9ZGOx+j1gIRl9KJo3OPz1gIor FDUFFVgmOLi0UtrpiWB1BLQT52Qshvns50N1NioL0Ik5MAjs48WKSkMetkNA+Pvh65Vw 3Frg== X-Gm-Message-State: AOJu0YxoB/gx0TCacR3OLjSxPkbkWWSHkgHEIcugwKGoJCve0bosRFFv wkcSj5vUSreIJlb9Af1Q2i0S+PvCgarxxABZZF4zyyn66f0f85rICgt9OgYXmoO6BzaHIvvZXvA 79my8wweUqlY/LR18GkLf8w== X-Received: by 2002:a17:907:78c5:b0:9b2:ffca:3890 with SMTP id kv5-20020a17090778c500b009b2ffca3890mr13176669ejc.19.1696842665335; Mon, 09 Oct 2023 02:11:05 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEP1Ornaxtnx13C5JOhktFdYlcz/qeszKulh3+g5WDUGbSrLbTi3TdMcObm0E5itrIW+tJb9g== X-Received: by 2002:a17:907:78c5:b0:9b2:ffca:3890 with SMTP id kv5-20020a17090778c500b009b2ffca3890mr13176651ejc.19.1696842664800; Mon, 09 Oct 2023 02:11:04 -0700 (PDT) Received: from localhost ([31.111.84.209]) by smtp.gmail.com with ESMTPSA id s10-20020a170906354a00b00997cce73cc7sm6467347eja.29.2023.10.09.02.11.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 09 Oct 2023 02:11:04 -0700 (PDT) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Cc: Simon Marchi Subject: Re: [PATCH 1/3] gdb: make remote_state's async token private In-Reply-To: <20231004020701.260411-2-simon.marchi@polymtl.ca> References: <20231004020701.260411-1-simon.marchi@polymtl.ca> <20231004020701.260411-2-simon.marchi@polymtl.ca> Date: Mon, 09 Oct 2023 10:11:03 +0100 Message-ID: <87fs2kduu0.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP 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: Simon Marchi via Gdb-patches writes: > From: Simon Marchi > > Make remote_async_inferior_event_token private (rename to > m_async_event_handler_token) and add methods for the various operations > we do on it. This will help by: > > - allowing to break on those methods when debugging > - allowing to add assertions in the methods Looks good. Couple of minor nits. > > Change-Id: Ia3b8a2bc48ad4849dbbe83442c3f83920f03334d > --- > gdb/remote.c | 70 ++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 43 insertions(+), 27 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index 9bb4f1de5982..2deba06d7d47 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -413,6 +413,31 @@ class remote_state > /* Get the remote arch state for GDBARCH. */ > struct remote_arch_state *get_remote_arch_state (struct gdbarch *gdbarch); > > + void create_async_event_handler () > + { Maybe add: gdb_assert (m_async_event_handler_token == nullptr); If this was ever not true then we'd leak a token, right? > + m_async_event_handler_token > + = ::create_async_event_handler ([] (gdb_client_data data) > + { Spaces before tabs here. > + inferior_event_handler (INF_REG_EVENT); > + }, > + nullptr, "remote"); > + } > + > + void mark_async_event_handler () > + { ::mark_async_event_handler (m_async_event_handler_token); } > + > + void clear_async_event_handler () > + { ::clear_async_event_handler (m_async_event_handler_token); } > + > + bool async_event_handler_marked () const > + { return ::async_event_handler_marked (m_async_event_handler_token); } > + > + void delete_async_event_handler () > + { > + if (m_async_event_handler_token != nullptr) > + ::delete_async_event_handler (&m_async_event_handler_token); > + } > + > public: /* data */ > > /* A buffer to use for incoming packets, and its current size. The > @@ -540,10 +565,6 @@ class remote_state > immediately, so queue is not needed for them. */ > std::vector stop_reply_queue; > > - /* Asynchronous signal handle registered as event loop source for > - when we have pending events ready to be passed to the core. */ > - struct async_event_handler *remote_async_inferior_event_token = nullptr; > - > /* FIXME: cagney/1999-09-23: Even though getpkt was called with > ``forever'' still use the normal timeout mechanism. This is > currently used by the ASYNC code to guarentee that target reads > @@ -554,6 +575,10 @@ class remote_state > bool wait_forever_enabled_p = true; > > private: > + /* Asynchronous signal handle registered as event loop source for > + when we have pending events ready to be passed to the core. */ > + async_event_handler *m_async_event_handler_token = nullptr; > + Trailing white-space here. Thanks, Andrew > /* Mapping of remote protocol data for each gdbarch. Usually there > is only one entry here, though we may see more with stubs that > support multi-process. */ > @@ -1384,8 +1409,6 @@ static void show_remote_protocol_packet_cmd (struct ui_file *file, > > static ptid_t read_ptid (const char *buf, const char **obuf); > > -static void remote_async_inferior_event_handler (gdb_client_data); > - > static bool remote_read_description_p (struct target_ops *target); > > static void remote_console_output (const char *msg); > @@ -4389,8 +4412,7 @@ remote_target::~remote_target () > everything of this target. */ > discard_pending_stop_replies_in_queue (); > > - if (rs->remote_async_inferior_event_token) > - delete_async_event_handler (&rs->remote_async_inferior_event_token); > + rs->delete_async_event_handler (); > > delete rs->notif_state; > } > @@ -5989,9 +6011,8 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p) > current_inferior ()->push_target (std::move (target_holder)); > > /* Register extra event sources in the event loop. */ > - rs->remote_async_inferior_event_token > - = create_async_event_handler (remote_async_inferior_event_handler, nullptr, > - "remote"); > + rs->create_async_event_handler (); > + > rs->notif_state = remote_notif_state_allocate (remote); > > /* Reset the target state; these things will be queried either by > @@ -7086,7 +7107,7 @@ remote_target::has_pending_events () > { > remote_state *rs = get_remote_state (); > > - if (async_event_handler_marked (rs->remote_async_inferior_event_token)) > + if (rs->async_event_handler_marked ()) > return true; > > /* Note that BUFCNT can be negative, indicating sticky > @@ -7420,7 +7441,7 @@ remote_notif_stop_can_get_pending_events (remote_target *remote, > may exit and we have no chance to process them back in > remote_wait_ns. */ > remote_state *rs = remote->get_remote_state (); > - mark_async_event_handler (rs->remote_async_inferior_event_token); > + rs->mark_async_event_handler (); > return 0; > } > > @@ -7628,7 +7649,7 @@ remote_target::queued_stop_reply (ptid_t ptid) > if (!rs->stop_reply_queue.empty () && target_can_async_p ()) > { > /* There's still at least an event left. */ > - mark_async_event_handler (rs->remote_async_inferior_event_token); > + rs->mark_async_event_handler (); > } > > return r; > @@ -7655,7 +7676,7 @@ remote_target::push_stop_reply (struct stop_reply *new_event) > enabled, and there are events in this queue, we will mark the event > token at that point, see remote_target::async. */ > if (target_is_async_p ()) > - mark_async_event_handler (rs->remote_async_inferior_event_token); > + rs->mark_async_event_handler (); > } > > /* Returns true if we have a stop reply for PTID. */ > @@ -8515,10 +8536,9 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status, > we'll mark it again at the end if needed. If the target is not in > async mode then the async token should not be marked. */ > if (target_is_async_p ()) > - clear_async_event_handler (rs->remote_async_inferior_event_token); > + rs->clear_async_event_handler (); > else > - gdb_assert (!async_event_handler_marked > - (rs->remote_async_inferior_event_token)); > + gdb_assert (!rs->async_event_handler_marked ()); > > ptid_t event_ptid; > > @@ -8533,7 +8553,7 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status, > notifications, then tell the event loop to call us again. */ > if (!rs->stop_reply_queue.empty () > || rs->notif_state->pending_event[notif_client_stop.id] != nullptr) > - mark_async_event_handler (rs->remote_async_inferior_event_token); > + rs->mark_async_event_handler (); > } > > return event_ptid; > @@ -14859,12 +14879,6 @@ remote_async_serial_handler (struct serial *scb, void *context) > inferior_event_handler (INF_REG_EVENT); > } > > -static void > -remote_async_inferior_event_handler (gdb_client_data data) > -{ > - inferior_event_handler (INF_REG_EVENT); > -} > - > int > remote_target::async_wait_fd () > { > @@ -14884,7 +14898,8 @@ remote_target::async (bool enable) > /* If there are pending events in the stop reply queue tell the > event loop to process them. */ > if (!rs->stop_reply_queue.empty ()) > - mark_async_event_handler (rs->remote_async_inferior_event_token); > + rs->mark_async_event_handler (); > + > /* For simplicity, below we clear the pending events token > without remembering whether it is marked, so here we always > mark it. If there's actually no pending notification to > @@ -14899,7 +14914,8 @@ remote_target::async (bool enable) > /* If the core is disabling async, it doesn't want to be > disturbed with target events. Clear all async event sources > too. */ > - clear_async_event_handler (rs->remote_async_inferior_event_token); > + rs->clear_async_event_handler (); > + > if (target_is_non_stop_p ()) > clear_async_event_handler (rs->notif_state->get_pending_events_token); > } > -- > 2.42.0