From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x431.google.com (mail-wr1-x431.google.com [IPv6:2a00:1450:4864:20::431]) by sourceware.org (Postfix) with ESMTPS id 161853858401 for ; Tue, 24 Aug 2021 16:14:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 161853858401 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=embecosm.com Received: by mail-wr1-x431.google.com with SMTP id i6so8374763wrv.2 for ; Tue, 24 Aug 2021 09:14:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=TKnMOd4sVOVew/E4KLOaR13fhOhihlg2kNQn8yYD37U=; b=T+36PNEYjAtdhRYedc4b6cj/mRoU5REGeVvzd8N5DQcRX+oD7+K1xYX7jG/2GVlIl0 yo3qR3rIgb4Aw8mla96L95ChfHqp6zKpYfmPOWM9hnF0zt+4fM0uUuvysBsqps0c2/KX W4W1U5dN3R+3U0bBA/0Fb0yuJyV8mf2XbVasn99CvS9cruf/elPQqMqvCTzxKt6faA8D VAAAR64pWeTDRUefFM3Kir8V1y4Ebez1p7SS0JgOZSgDEvnHUuj7K2XQPLcXLhLpnAzr RuTsat9hkk2aG5ow6mQINbZWkqNYZ1+vHN4ubhiC+673Fs+SyZhupFGZcV3fZtNH9+um 01wg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=TKnMOd4sVOVew/E4KLOaR13fhOhihlg2kNQn8yYD37U=; b=Vsv7fxXOz/6ffJqkkfZUrphymsi06EjMhyI5tFd1FO0CCLNHP26nXg/zzZm3XNlH3v lsvPC2g+QqwGiUHfvafFB/ZOSbCpIflPTMg26ithwBQnYpYYH4sm06huqdQWSUoaIWwt KBnUdnOaOvmJLivi2UxwUdRldWy3m+2d3XnENYAgZ31mbM0dU9fpSQJv9+kuaqF8ST0e MWNHrfhWk1rlYqNCgRvniVbsFovQU1hdwqfdXdYHpmmvKDQaI2igcfGwVi5J7No9RYSg gV9gi7q2aZ6xefN14sQJDoIROVJVZ143PRHzzAic+rnRi6YCPANMB4QH9Zp5M154IPcf gdLA== X-Gm-Message-State: AOAM532w7u7agLP7PwPOaU8hHOUwm+ZkkV9nsZkTtvOaJeoLO4kXyyFw zDT0/0vobJLPPeR5UpSQQW8R1g== X-Google-Smtp-Source: ABdhPJwrpnx3oXiBAe4Or8NO1nOrMo00LsB7GzeAuTeflqx9PbHsqEDNKRyaKtblVQsuRRWNLN7g4A== X-Received: by 2002:a5d:658e:: with SMTP id q14mr20396886wru.142.1629821653153; Tue, 24 Aug 2021 09:14:13 -0700 (PDT) Received: from localhost (host86-188-49-44.range86-188.btcentralplus.com. [86.188.49.44]) by smtp.gmail.com with ESMTPSA id w29sm18757309wra.88.2021.08.24.09.14.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 24 Aug 2021 09:14:12 -0700 (PDT) Date: Tue, 24 Aug 2021 17:14:11 +0100 From: Andrew Burgess To: Patrick Monnerat Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Replace deprecated_target_wait_hook by an observer Message-ID: <20210824161411.GE2581@embecosm.com> References: <20210822164256.144875-1-patrick@monnerat.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210822164256.144875-1-patrick@monnerat.net> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 17:12:16 up 7 days, 5:08, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Aug 2021 16:14:24 -0000 * Patrick Monnerat via Gdb-patches [2021-08-22 18:42:56 +0200]: > Commit b60cea7 (Make target_wait options use enum flags) broke > deprecated_target_wait_hook usage: there's a commit comment telling > this hook has not been converted. > > Rather than trying to mend it, this patch replaces the hook by a > target_wait observer: > > waiting_for_target (bool entering, ptid_t ptid) > > Upon target_wait entry, it is notified with entering=TRUE and ptid passed > to target_wait. Upon exit, it is notified again with entering=FALSE and > ptid = event ptid returned by target_wait. > > This change benefits to Insight (out-of-tree): there's no real use of the > late hook in gdb itself. > --- > gdb/infrun.c | 15 +++------------ > gdb/infrun.h | 5 ++--- > gdb/interps.c | 1 - > gdb/observable.c | 1 + > gdb/observable.h | 3 +++ > gdb/target.c | 7 ++++++- > gdb/top.c | 7 ------- > 7 files changed, 15 insertions(+), 24 deletions(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 5ee650fa464..695e3b0a586 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -366,7 +366,7 @@ show_stop_on_solib_events (struct ui_file *file, int from_tty, > static bool stop_print_frame; > > /* This is a cached copy of the target/ptid/waitstatus of the last > - event returned by target_wait()/deprecated_target_wait_hook(). > + event returned by target_wait(). > This information is returned by get_last_target_status(). */ > static process_stratum_target *target_last_proc_target; > static ptid_t target_last_wait_ptid; > @@ -3478,7 +3478,6 @@ static ptid_t > do_target_wait_1 (inferior *inf, ptid_t ptid, > target_waitstatus *status, target_wait_flags options) > { > - ptid_t event_ptid; > struct thread_info *tp; > > /* We know that we are looking for an event in the target of inferior > @@ -3594,12 +3593,7 @@ do_target_wait_1 (inferior *inf, ptid_t ptid, > if (!target_can_async_p ()) > options &= ~TARGET_WNOHANG; > > - if (deprecated_target_wait_hook) > - event_ptid = deprecated_target_wait_hook (ptid, status, options); > - else > - event_ptid = target_wait (ptid, status, options); > - > - return event_ptid; > + return target_wait (ptid, status, options); > } > > /* Wrapper for target_wait that first checks whether threads have > @@ -4558,10 +4552,7 @@ poll_one_curr_target (struct target_waitstatus *ws) > don't get any event. */ > target_dcache_invalidate (); > > - if (deprecated_target_wait_hook) > - event_ptid = deprecated_target_wait_hook (minus_one_ptid, ws, TARGET_WNOHANG); > - else > - event_ptid = target_wait (minus_one_ptid, ws, TARGET_WNOHANG); > + event_ptid = target_wait (minus_one_ptid, ws, TARGET_WNOHANG); > > if (debug_infrun) > print_target_wait_results (minus_one_ptid, event_ptid, ws); > diff --git a/gdb/infrun.h b/gdb/infrun.h > index 5a577365f94..a14e20b5f94 100644 > --- a/gdb/infrun.h > +++ b/gdb/infrun.h > @@ -124,9 +124,8 @@ extern process_stratum_target *user_visible_resume_target (ptid_t resume_ptid); > extern int normal_stop (void); > > /* Return the cached copy of the last target/ptid/waitstatus returned > - by target_wait()/deprecated_target_wait_hook(). The data is > - actually cached by handle_inferior_event(), which gets called > - immediately after target_wait()/deprecated_target_wait_hook(). */ > + by target_wait(). The data is actually cached by handle_inferior_event(), > + which gets called immediately after target_wait(). */ > extern void get_last_target_status (process_stratum_target **target, > ptid_t *ptid, > struct target_waitstatus *status); > diff --git a/gdb/interps.c b/gdb/interps.c > index ec19822b571..55bd10467c3 100644 > --- a/gdb/interps.c > +++ b/gdb/interps.c > @@ -356,7 +356,6 @@ clear_interpreter_hooks (void) > deprecated_readline_hook = 0; > deprecated_readline_end_hook = 0; > deprecated_context_hook = 0; > - deprecated_target_wait_hook = 0; > deprecated_call_command_hook = 0; > deprecated_error_begin_hook = 0; > } > diff --git a/gdb/observable.c b/gdb/observable.c > index 51f5edb0a1f..08d45c77ea7 100644 > --- a/gdb/observable.c > +++ b/gdb/observable.c > @@ -77,6 +77,7 @@ DEFINE_OBSERVABLE (register_changed); > DEFINE_OBSERVABLE (user_selected_context_changed); > DEFINE_OBSERVABLE (source_styling_changed); > DEFINE_OBSERVABLE (current_source_symtab_and_line_changed); > +DEFINE_OBSERVABLE (waiting_for_target); Given we already have events 'target_changed' and 'target_resumed', I wonder if it would be more consistent to name this event 'target_wait'? > > } /* namespace observers */ > } /* namespace gdb */ > diff --git a/gdb/observable.h b/gdb/observable.h > index 915770ff363..d61b5468c9e 100644 > --- a/gdb/observable.h > +++ b/gdb/observable.h > @@ -251,6 +251,9 @@ extern observable<> source_styling_changed; > > extern observable<> current_source_symtab_and_line_changed; > > +/* About to enter target_wait () or leave it. */ > +extern observable waiting_for_target; > + > } /* namespace observers */ > > } /* namespace gdb */ > diff --git a/gdb/target.c b/gdb/target.c > index ae2d659583e..df7c64e204f 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -26,6 +26,7 @@ > #include "symtab.h" > #include "inferior.h" > #include "infrun.h" > +#include "observable.h" > #include "bfd.h" > #include "symfile.h" > #include "objfiles.h" > @@ -2599,13 +2600,17 @@ target_wait (ptid_t ptid, struct target_waitstatus *status, > { > target_ops *target = current_inferior ()->top_target (); > process_stratum_target *proc_target = current_inferior ()->process_target (); > + ptid_t event_ptid; > > gdb_assert (!proc_target->commit_resumed_state); > > if (!target->can_async_p ()) > gdb_assert ((options & TARGET_WNOHANG) == 0); > > - return target->wait (ptid, status, options); > + gdb::observers::waiting_for_target.notify (true, ptid); > + event_ptid = target->wait (ptid, status, options); > + gdb::observers::waiting_for_target.notify (false, event_ptid); > + return event_ptid; I would be tempted to wrap this notification inside an RAII class, then we will be guaranteed to send the second notification, even in the event that the wait call exits via an exception. Thanks, Andrew > } > > /* See target.h. */ > diff --git a/gdb/top.c b/gdb/top.c > index 0c49f4f9eb4..368166720ac 100644 > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -249,13 +249,6 @@ void (*deprecated_readline_end_hook) (void); > void (*deprecated_attach_hook) (void); > void (*deprecated_detach_hook) (void); > > -/* Called when going to wait for the target. Usually allows the GUI > - to run while waiting for target events. */ > - > -ptid_t (*deprecated_target_wait_hook) (ptid_t ptid, > - struct target_waitstatus *status, > - int options); > - > /* Used by UI as a wrapper around command execution. May do various > things like enabling/disabling buttons, etc... */ > > -- > 2.31.1 >