Re: [PATCH 05/11 v5] Add target/target.h

[Pedro Alves <palves at redhat dot com>]

(Reviewing in pieces)

On 08/01/2014 11:19 AM, Gary Benson wrote:

> +/* See target/target.h.  */
> +
> +int
> +target_read_uint32 (CORE_ADDR memaddr, unsigned int *result)
> +{

The type of 'result' should be uint32_t then.

> +
> +/* Resume execution of the target process PTID (or a group of
> +   threads).  STEP says whether to 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.  */

Most of this comment doesn't make any sense for gdbserver, and I don't
ever will.  (GDBserver's resume interface is more flexible than gdb's.)
There's no inferior_ptid in gdbserver.  There's no thread->suspend.stop_signal.
In the gdbserver implemention this adds:

> +  resume_info.thread = ptid;
> +  resume_info.kind = step ? resume_step : resume_continue;
> +  resume_info.sig = signal;
> +  (*the_target->resume) (&resume_info, 1);

... when STEP is true and PTID is a wildcard, what this actually does
is tell the target to step each thread in the wildcard, all in parallel.

I think we should instead add a new 'target_continue_ptid (ptid_t ptid)'
method, that then consumes/calls gdb's target_resume in gdb's implementation,
and gdbserver's '(*the_target->resume) (&resume_info, 1);' in the
gdbserver implementation?

> +
> +extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);

Thanks,
Pedro Alves