Re: [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions

[Luis Machado <lgustavo at codesourcery dot com>]

I missed 5/5... Mostly nits, overall it looks sane to me.

On 02/17/2016 12:44 AM, Pedro Alves wrote:
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 15210c9..f72937c 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2357,6 +2357,8 @@ do_target_resume (ptid_t resume_ptid, int step, enum gdb_signal sig)
>       target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
>
>     target_resume (resume_ptid, step, sig);
> +
> +  target_do_resume ();
>   }

I'm looking at the sequence of function names and they don't look too clear.

do_target_resume/target_resume/target_do_resume.

Should we have better names for these functions? Ones that make it more 
explicit what each function is doing and the fact that we are 
potentially defering resumptions? Like "queue_resume_actions" for 
target_resume or "commit_resumption_actions" for target_do_resume?

> @@ -6200,6 +6555,56 @@ remove_new_fork_children (struct threads_listing_context *context)
>   		 remove_child_of_pending_fork, &param);
>   }
>
> +/* Check whether EVENT would prevent a global or process wildcard
> +   vCont action.  */
> +
> +static int
> +check_pending_event_prevents_wildcard_vcont_callback
> +  (QUEUE (stop_reply_p) *q,
> +   QUEUE_ITER (stop_reply_p) *iter,
> +   stop_reply_p event,
> +   void *data)
> +{
> +  struct inferior *inf;
> +  int *may_global_wildcard_vcont = (int *) data;
> +
> +  if (event->ws.kind == TARGET_WAITKIND_NO_RESUMED
> +      || event->ws.kind == TARGET_WAITKIND_NO_HISTORY)
> +    return 1;
> +
> +  if (event->ws.kind == TARGET_WAITKIND_FORKED
> +      || event->ws.kind == TARGET_WAITKIND_VFORKED)
> +    *may_global_wildcard_vcont = 0;
> +
> +  inf = find_inferior_ptid (event->ptid);
> +
> +  /* This may be the first time we heard about this process.
> +     Regardless, we must not do a global wildcard resume, otherwise
> +     we'd resume this process too.  */
> +  *may_global_wildcard_vcont = 0;
> +  if (inf != NULL)
> +    inf->priv->may_wildcard_vcont = 0;
> +
> +  return 1;
> +}
> +
> +/* Check whether any event pending in the vStopped queue would prevent
> +   a global or process wildcard vCont action.  Clear
> +   *may_global_wildcard if we can't do a global wildcard (vCont;c),
> +   and clear the event inferior's may_wildcard_vcont flag if we can do
> +   a process-wide wildcard resume (vCont;c:pPID.-1).  */
> +

... inferior's may_wildcard_vcont flag if we can do ...

Can do or can't do?

> diff --git a/gdb/target.h b/gdb/target.h
> index 7c8513a..c40d1c7 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -461,6 +461,8 @@ struct target_ops
>   		       int TARGET_DEBUG_PRINTER (target_debug_print_step),
>   		       enum gdb_signal)
>         TARGET_DEFAULT_NORETURN (noprocess ());
> +    void (*to_do_resume) (struct target_ops *)
> +      TARGET_DEFAULT_IGNORE ();
>       ptid_t (*to_wait) (struct target_ops *,
>   		       ptid_t, struct target_waitstatus *,
>   		       int TARGET_DEBUG_PRINTER (target_debug_print_options))
> @@ -1317,19 +1319,41 @@ extern void target_detach (const char *, int);
>
>   extern void target_disconnect (const char *, int);
>
> -/* Resume execution of the target process PTID (or a group of
> -   threads).  STEP says whether to hardware single-step or to run free;
> -   SIGGNAL is the signal to be given to the target, or GDB_SIGNAL_0 for no
> -   signal.  The caller may not pass GDB_SIGNAL_DEFAULT.  A specific
> -   PTID means `step/resume only this process id'.  A wildcard PTID
> -   (all threads, or all threads of process) means `step/resume
> -   INFERIOR_PTID, and let other threads (for which the wildcard PTID
> -   matches) resume with their 'thread->suspend.stop_signal' signal
> -   (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal
> -   if in "no pass" state.  */
> -
> +/* Resume execution (or prepare for execution) of a target thread,
> +   process or all processes.  STEP says whether to hardware
> +   single-step or to run free; SIGGNAL is the signal to be given to
> +   the target, or GDB_SIGNAL_0 for no signal.  The caller may not pass
> +   GDB_SIGNAL_DEFAULT.  A specific PTID means `step/resume only this
> +   process id'.  A wildcard PTID (all threads, or all threads of
> +   process) means `step/resume INFERIOR_PTID, and let other threads
> +   (for which the wildcard PTID matches) resume with their
> +   'thread->suspend.stop_signal' signal (usually GDB_SIGNAL_0) if it
> +   is in "pass" state, or with no signal if in "no pass" state.
> +
> +   In order to efficiently handle batches of resumption requests,
> +   targets may implement this method such that it records the
> +   resumption request, but defers the actual resumption to the
> +   target_do_resume method implementation.  See target_do_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 call target_do_resume after calling target_resume or
> +   more times.  A target may thus use this method in coordination with


"GDB always calls ... target_resume one or more times."?