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

Luis Machado
2016-02-17T13:42:59Z

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."?