From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id MhfsGZyg+F+INQAAWB0awg (envelope-from ) for ; Fri, 08 Jan 2021 13:12:44 -0500 Received: by simark.ca (Postfix, from userid 112) id 5CD401E99A; Fri, 8 Jan 2021 13:12:44 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.4 required=5.0 tests=DKIM_SIGNED,MAILING_LIST_MULTI, RDNS_NONE,T_DKIM_INVALID,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id A20E31E4F4 for ; Fri, 8 Jan 2021 13:12:43 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 1EFE838438A3; Fri, 8 Jan 2021 18:12:43 +0000 (GMT) Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) by sourceware.org (Postfix) with ESMTPS id 865DA38438A3 for ; Fri, 8 Jan 2021 18:12:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 865DA38438A3 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wm1-x333.google.com with SMTP id v14so8527073wml.1 for ; Fri, 08 Jan 2021 10:12:40 -0800 (PST) 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=/Iu7wO5DfyKnPNxvJ+ssaKjGocPWqjNYcAflxYGp4uo=; b=Doj5l23C9Fe905bdQ9JsWHTgltEH5EwLkoAiPMW5fY1y5z45bwHj2fPKBOLhp2kYFa SrHRTskoOfBlZvQ/zwWNo/FqwyXY9Wn2a+RrgL2pQmBkLZLWfj2iXIALwBSYnNhc57C5 mdYfABxPvUDJjq2k/g6NigHyBE76YUM5PCrB/x575796iVw5be++gObmWd0rgBsAE2xu 7Sel5964s0QmJVZxf3MPu7XzlXZTHm0JQ2Lizk8bujX8702uxFclrU4kHoZ0LIPkJHHx 5ixzpfHv4tjdONGopd+4VTz/ii2mcG1TLSd9/GVpRTHgieveWYk73BYB42UZXis5a3Ds aD7g== 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=/Iu7wO5DfyKnPNxvJ+ssaKjGocPWqjNYcAflxYGp4uo=; b=qCtIfuC1ss0I9uLFLsXWZq8fs7SmH41RHixAgZ/5k50aDFAkZnveN33E4FCAZOnBBC 5C/Yb7U8Hm834WsE/ub5OPltg1V1D0IZj3QRe+IehqcBqK3NlDmeCSIGfZFzpegdqqJQ gLM/E+n5oUyZH7faL6CLQwFQzZspSaU+O506qKzjR6XG5ud/If4SEaMLKLjKnoFUogvs tifKP3nvIXNZBsByNYWbG2LE2JJ02y+9mwneWds/JkW59usAfUoubH509d/j+fZUmwjK KYbrbxP5guu4hxlH6l+O7nPcNynf+PRF3ebN1rR/E9xijVQ6ojiI6OGWCLcafiKVcLTb J6ww== X-Gm-Message-State: AOAM530XEo0W6eLc+jAEikDNV/acIJdpqkrs/CmfAFbCvegkMGLrRJLC FxvJya6QSBI3IIxMDXuyrRm5qdodjyaAUg== X-Google-Smtp-Source: ABdhPJyOAHgGQFMr6Jx4hhvxUxHCbrMbIQwwDIU608ZfOOtTaev5WEWdqW1xKWzm6HSh/g6/OpReGg== X-Received: by 2002:a1c:2091:: with SMTP id g139mr4197478wmg.133.1610129559491; Fri, 08 Jan 2021 10:12:39 -0800 (PST) Received: from localhost (host86-166-129-230.range86-166.btcentralplus.com. [86.166.129.230]) by smtp.gmail.com with ESMTPSA id c81sm13216580wmd.6.2021.01.08.10.12.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 08 Jan 2021 10:12:38 -0800 (PST) Date: Fri, 8 Jan 2021 18:12:37 +0000 From: Andrew Burgess To: Simon Marchi Subject: Re: [PATCH v3 3/5] gdb: move commit_resume to process_stratum_target Message-ID: <20210108181237.GB2945@embecosm.com> References: <20210108041734.3873826-1-simon.marchi@polymtl.ca> <20210108041734.3873826-4-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210108041734.3873826-4-simon.marchi@polymtl.ca> X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 18:02:55 up 30 days, 22:47, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] 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: , Cc: Simon Marchi , gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" * Simon Marchi [2021-01-07 23:17:32 -0500]: > From: Simon Marchi > > The following patch will change the commit_resume target method to > something stateful. Because it would be difficult to track a state > replicated in the various targets of a target stack, and since for the > foreseeable future, only process stratum targets are going to use this > concept, this patch makes the commit resume concept specific to process > stratum targets. > > So, move the method to process_stratum_target, and move helper functions > to process-stratum-target.h. > > gdb/ChangeLog: > > * target.h (struct target_ops) : New. > (target_commit_resume): Remove. > (make_scoped_defer_target_commit_resume): Remove. > * target.c (defer_target_commit_resume): Remove. > (target_commit_resume): Remove. > (make_scoped_defer_target_commit_resume): Remove. > * process-stratum-target.h (class process_stratum_target) > : New. > (maybe_commit_resume_all_process_targets): New. > (make_scoped_defer_process_target_commit_resume): New. > * process-stratum-target.c (defer_process_target_commit_resume): > New. > (maybe_commit_resume_process_target): New. > (make_scoped_defer_process_target_commit_resume): New. > * infrun.c (do_target_resume): Adjust. > (commit_resume_all_targets): Rename into... > (maybe_commit_resume_all_process_targets): ... this, adjust. > (proceed): Adjust. > * record-full.c (record_full_wait_1): Adjust. > * target-delegates.c: Re-generate. > > Change-Id: Ifc957817ac5b2303e22760ce3d14740b9598f02c > --- > gdb/infrun.c | 28 +++++++++------------------- > gdb/process-stratum-target.c | 23 +++++++++++++++++++++++ > gdb/process-stratum-target.h | 29 +++++++++++++++++++++++++++++ > gdb/record-full.c | 8 ++++---- > gdb/target-delegates.c | 22 ---------------------- > gdb/target.c | 22 ---------------------- > gdb/target.h | 20 -------------------- > 7 files changed, 65 insertions(+), 87 deletions(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 45bedf896419..1a27af51b7e9 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -2172,7 +2172,7 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig) > > target_resume (resume_ptid, step, sig); > > - target_commit_resume (); > + maybe_commit_resume_process_target (tp->inf->process_target ()); > > if (target_can_async_p ()) > target_async (1); > @@ -2760,28 +2760,17 @@ schedlock_applies (struct thread_info *tp) > execution_direction))); > } > > -/* Calls target_commit_resume on all targets. */ > +/* Calls maybe_commit_resume_process_target on all process targets. */ > > static void > -commit_resume_all_targets () > +maybe_commit_resume_all_process_targets () > { > scoped_restore_current_thread restore_thread; > > - /* Map between process_target and a representative inferior. This > - is to avoid committing a resume in the same target more than > - once. Resumptions must be idempotent, so this is an > - optimization. */ > - std::unordered_map conn_inf; > - > - for (inferior *inf : all_non_exited_inferiors ()) > - if (inf->has_execution ()) > - conn_inf[inf->process_target ()] = inf; > - > - for (const auto &ci : conn_inf) > + for (process_stratum_target *target : all_non_exited_process_targets ()) > { > - inferior *inf = ci.second; > - switch_to_inferior_no_thread (inf); > - target_commit_resume (); > + switch_to_target_no_thread (target); > + maybe_commit_resume_process_target (target); > } > } > > @@ -3005,7 +2994,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) > cur_thr->prev_pc = regcache_read_pc_protected (regcache); > > { > - scoped_restore save_defer_tc = make_scoped_defer_target_commit_resume (); > + scoped_restore save_defer_tc > + = make_scoped_defer_process_target_commit_resume (); > > started = start_step_over (); > > @@ -3075,7 +3065,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) > } > } > > - commit_resume_all_targets (); > + maybe_commit_resume_all_process_targets (); > > finish_state.release (); > > diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c > index 719167803fff..1436a550ac04 100644 > --- a/gdb/process-stratum-target.c > +++ b/gdb/process-stratum-target.c > @@ -108,3 +108,26 @@ switch_to_target_no_thread (process_stratum_target *target) > break; > } > } > + > +/* If true, `maybe_commit_resume_process_target` is a no-op. */ > + > +static bool defer_process_target_commit_resume; > + > +/* See target.h. */ Should be 'process-stratum-target.h' now. Thanks, Andrew > + > +void > +maybe_commit_resume_process_target (process_stratum_target *proc_target) > +{ > + if (defer_process_target_commit_resume) > + return; > + > + proc_target->commit_resume (); > +} > + > +/* See process-stratum-target.h. */ > + > +scoped_restore_tmpl > +make_scoped_defer_process_target_commit_resume () > +{ > + return make_scoped_restore (&defer_process_target_commit_resume, true); > +} > diff --git a/gdb/process-stratum-target.h b/gdb/process-stratum-target.h > index b513c26ffc2a..c8060c46be93 100644 > --- a/gdb/process-stratum-target.h > +++ b/gdb/process-stratum-target.h > @@ -63,6 +63,20 @@ class process_stratum_target : public target_ops > bool has_registers () override; > bool has_execution (inferior *inf) override; > > + /* Commit a series of resumption requests previously prepared with > + resume calls. > + > + GDB always calls `commit_resume` on the process stratum target after > + calling `resume` on a target stack. A process stratum target may thus use > + this method in coordination with its `resume` method to batch resumption > + requests. In that case, the target doesn't actually resume in its > + `resume` implementation. Instead, it takes note of resumption intent in > + `resume`, and defers the actual resumption `commit_resume`. > + > + E.g., the remote target uses this to coalesce multiple resumption requests > + in a single vCont packet. */ > + virtual void commit_resume () {} > + > /* True if any thread is, or may be executing. We need to track > this separately because until we fully sync the thread list, we > won't know whether the target is fully stopped, even if we see > @@ -92,4 +106,19 @@ extern std::set all_non_exited_process_targets (); > > extern void switch_to_target_no_thread (process_stratum_target *target); > > +/* Commit a series of resumption requests previously prepared with > + target_resume calls. > + > + This function is a no-op if commit resumes are deferred (see > + `make_scoped_defer_process_target_commit_resume`). */ > + > +extern void maybe_commit_resume_process_target > + (process_stratum_target *target); > + > +/* Setup to defer `commit_resume` calls, and re-set to the previous status on > + destruction. */ > + > +extern scoped_restore_tmpl > + make_scoped_defer_process_target_commit_resume (); > + > #endif /* !defined (PROCESS_STRATUM_TARGET_H) */ > diff --git a/gdb/record-full.c b/gdb/record-full.c > index 22eaaa4bb1bc..56ab29479874 100644 > --- a/gdb/record-full.c > +++ b/gdb/record-full.c > @@ -1242,11 +1242,11 @@ record_full_wait_1 (struct target_ops *ops, > break; > } > > + process_stratum_target *proc_target > + = current_inferior ()->process_target (); > + > if (gdbarch_software_single_step_p (gdbarch)) > { > - process_stratum_target *proc_target > - = current_inferior ()->process_target (); > - > /* Try to insert the software single step breakpoint. > If insert success, set step to 0. */ > set_executing (proc_target, inferior_ptid, false); > @@ -1263,7 +1263,7 @@ record_full_wait_1 (struct target_ops *ops, > "issuing one more step in the " > "target beneath\n"); > ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0); > - ops->beneath ()->commit_resume (); > + proc_target->commit_resume (); > continue; > } > } > diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c > index 437b19b8581c..8b933fdf82eb 100644 > --- a/gdb/target-delegates.c > +++ b/gdb/target-delegates.c > @@ -14,7 +14,6 @@ struct dummy_target : public target_ops > void detach (inferior *arg0, int arg1) override; > void disconnect (const char *arg0, int arg1) override; > void resume (ptid_t arg0, int arg1, enum gdb_signal arg2) override; > - void commit_resume () override; > ptid_t wait (ptid_t arg0, struct target_waitstatus *arg1, target_wait_flags arg2) override; > void fetch_registers (struct regcache *arg0, int arg1) override; > void store_registers (struct regcache *arg0, int arg1) override; > @@ -185,7 +184,6 @@ struct debug_target : public target_ops > void detach (inferior *arg0, int arg1) override; > void disconnect (const char *arg0, int arg1) override; > void resume (ptid_t arg0, int arg1, enum gdb_signal arg2) override; > - void commit_resume () override; > ptid_t wait (ptid_t arg0, struct target_waitstatus *arg1, target_wait_flags arg2) override; > void fetch_registers (struct regcache *arg0, int arg1) override; > void store_registers (struct regcache *arg0, int arg1) override; > @@ -440,26 +438,6 @@ debug_target::resume (ptid_t arg0, int arg1, enum gdb_signal arg2) > fputs_unfiltered (")\n", gdb_stdlog); > } > > -void > -target_ops::commit_resume () > -{ > - this->beneath ()->commit_resume (); > -} > - > -void > -dummy_target::commit_resume () > -{ > -} > - > -void > -debug_target::commit_resume () > -{ > - fprintf_unfiltered (gdb_stdlog, "-> %s->commit_resume (...)\n", this->beneath ()->shortname ()); > - this->beneath ()->commit_resume (); > - fprintf_unfiltered (gdb_stdlog, "<- %s->commit_resume (", this->beneath ()->shortname ()); > - fputs_unfiltered (")\n", gdb_stdlog); > -} > - > ptid_t > target_ops::wait (ptid_t arg0, struct target_waitstatus *arg1, target_wait_flags arg2) > { > diff --git a/gdb/target.c b/gdb/target.c > index 3a03a0ad530e..3a5270e5a416 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -2062,28 +2062,6 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal) > clear_inline_frame_state (curr_target, ptid); > } > > -/* If true, target_commit_resume is a nop. */ > -static int defer_target_commit_resume; > - > -/* See target.h. */ > - > -void > -target_commit_resume (void) > -{ > - if (defer_target_commit_resume) > - return; > - > - current_top_target ()->commit_resume (); > -} > - > -/* See target.h. */ > - > -scoped_restore_tmpl > -make_scoped_defer_target_commit_resume () > -{ > - return make_scoped_restore (&defer_target_commit_resume, 1); > -} > - > void > target_pass_signals (gdb::array_view pass_signals) > { > diff --git a/gdb/target.h b/gdb/target.h > index e1a1d7a9226b..a252c29eafb4 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -478,8 +478,6 @@ struct target_ops > int TARGET_DEBUG_PRINTER (target_debug_print_step), > enum gdb_signal) > TARGET_DEFAULT_NORETURN (noprocess ()); > - virtual void commit_resume () > - TARGET_DEFAULT_IGNORE (); > /* See target_wait's description. Note that implementations of > this method must not assume that inferior_ptid on entry is > pointing at the thread or inferior that ends up reporting an > @@ -1431,24 +1429,6 @@ extern void target_disconnect (const char *, int); > target_commit_resume below. */ > extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal); > > -/* Commit a series of resumption requests previously prepared with > - target_resume calls. > - > - GDB always calls target_commit_resume after calling target_resume > - one or more times. A target may thus use this method in > - coordination with the target_resume method to batch target-side > - resumption requests. In that case, the target doesn't actually > - resume in its target_resume implementation. Instead, it prepares > - the resumption in target_resume, and defers the actual resumption > - to target_commit_resume. E.g., the remote target uses this to > - coalesce multiple resumption requests in a single vCont packet. */ > -extern void target_commit_resume (); > - > -/* Setup to defer target_commit_resume calls, and reactivate > - target_commit_resume on destruction, if it was previously > - active. */ > -extern scoped_restore_tmpl make_scoped_defer_target_commit_resume (); > - > /* For target_read_memory see target/target.h. */ > > /* The default target_ops::to_wait implementation. */ > -- > 2.29.2 >