Re: [PATCH v3 3/5] gdb: move commit_resume to process_stratum_target

Andrew Burgess
2021-01-08T18:12:44Z

* Simon Marchi <simon.marchi@polymtl.ca> [2021-01-07 23:17:32 -0500]:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> 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) <commit_resume>: 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)
> <commit_resume>: 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<process_stratum_target *, inferior *> 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<bool>
> +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<process_stratum_target *> 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<bool>
> + 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<int>
> -make_scoped_defer_target_commit_resume ()
> -{
> - return make_scoped_restore (&defer_target_commit_resume, 1);
> -}
> -
> void
> target_pass_signals (gdb::array_view<const unsigned char> 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<int> make_scoped_defer_target_commit_resume ();
> -
> /* For target_read_memory see target/target.h. */
>
> /* The default target_ops::to_wait implementation. */
> --
> 2.29.2
>