Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

Doug Evans
2014-09-10T17:45:21Z

Gary Benson writes:
> This commit introduces two new functions to stop and restart target
> processes that shared code can use and that clients must implement.
> It also changes some shared code to use these functions.
>
> The changes in this patch replace the target_stop, target_wait
> and target_resume parts of this patch I posted on August 1:
> https://sourceware.org/ml/gdb-patches/2014-08/msg00014.html
> The remainder of that patch is in patch 2 of this series.
>
> gdb/ChangeLog:
>
> * target/target.h (target_stop_ptid, target_continue_ptid):
> Declare.
> * target.c (target_stop_ptid, target_continue_ptid): New
> functions.
> * common/agent.c [!GDBSERVER]: Don't include infrun.h.
> (agent_run_command): Always use target_stop_ptid and
> target_continue_ptid.
>
> gdb/gdbserver/ChangeLog:
>
> * target.c (target_stop_ptid, target_continue_ptid): New
> functions.
> [...]
> diff --git a/gdb/target.c b/gdb/target.c
> index 711e7cb..339b1d1 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3027,6 +3027,31 @@ target_stop (ptid_t ptid)
> (*current_target.to_stop) (&current_target, ptid);
> }
>
> [...]
> +/* See target/target.h. */
> +
> +void
> +target_continue_ptid (ptid_t ptid)
> +{
> + target_resume (ptid, 0, GDB_SIGNAL_0);
> +}
> +

Hi.
How come GDB_SIGNAL_0 is used here?
Maybe it's correct, but it's not immediately clear.

The reason I ask is because there are two ways to "continue"
the inferior:
1) resume it where it left off, and if it stopped because
of a signal then forward on that signal (assuming the
signal is not "nopass") (GDB_SIGNAL_DEFAULT).
2) Either inject a new signal (GDB_SIGNAL_FOO) or cancel out a previously
queued signal (GDB_SIGNAL_0).

GDB_SIGNAL_0 is used to resume the target and discarding
any signal that it may have stopped for.
GDB_SIGNAL_DEFAULT is used for (1).

I realize the comments for target_resume say to not pass
GDB_SIGNAL_DEFAULT to it. But the name "target_continue_ptid"
with no option to choose between (1) and (2)
says to me "do what GDB_SIGNAL_DEFAULT" does.

> +/* Restart a target that was previously stopped by target_stop_ptid.
> + This function must be provided by the client. */
> +
> +extern void target_continue_ptid (ptid_t ptid);
> +