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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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
>