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

[Pedro Alves <palves@redhat.com>]

On 09/11/2014 11:26 AM, Gary Benson wrote:
> Doug Evans wrote:
>> 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.
>> [...]
>>> +/* See target/target.h.  */
>>> +
>>> +void
>>> +target_continue_ptid (ptid_t ptid)
>>> +{
>>> +  target_resume (ptid, 0, GDB_SIGNAL_0);
>>> +}
>>
>> 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.
> 
> I don't know the answer to this, I just moved the code from one
> place to the next :)  Possibly it's because (I think) under the
> hood target_stop_ptid sends a SIGSTOP to the inferior, so you
> don't want to restart it with that signal queued.

No, that's not it.  The SIGSTOP is an internal detail of the
target implementation.  It's reported to the target_wait caller
as GDB_SIGNAL_0 (meaning, the thread stopped for no reason
other than that GDB wanted it to stop).

> The comment for target_continue_ptid says:
> 
>>> +/* Restart a target that was previously stopped by target_stop_ptid.
>>> +   This function must be provided by the client.  */
> 
> That implies to me "don't use this for targets not previously
> stopped by target_stop_ptid".
> 
> Maybe someone more familiar with this code could elaborate?

I guess that'd be me...

In the case that target_stop_ptid returns anything else other
than GDB_SIGNAL_0, resuming with GDB_SIGNAL_0, GDB_SIGNAL_DEFAULT or
the signal that the thread had stopped for would all be wrong.
If we got a synchronous SIGSEGV, for instance, we'd want to abort
the agent call and return error.  We'd likely disable access to the
agent from there on, as it's messed up.  If instead we got an
asynchronous signal, we'd want to hold on to it, and requeue it
later, once the agent command is done with.

[As an example showing this same complication being handled
similarly, see See enqueue_one_deferred_signal and
collecting_fast_tracepoint in gdbserver/linux-low.c.  This
handles the case of getting signals when they arrive while
a thread is in a fast tracepoint jump pad.    That's a lower
level though.]

But, we have to look at where/how this is presently being used
to understand why the current code got away from handling
that complication.

The thread that we're interacting with blocks all signals.
See gdbserver/tracepoint.c:

static void
gdb_agent_init (void)
{
  int res;
  pthread_t thread;
  sigset_t new_mask;
  sigset_t orig_mask;

  /* We want the helper thread to be as transparent as possible, so
     have it inherit an all-signals-blocked mask.  */

  sigfillset (&new_mask);
  res = pthread_sigmask (SIG_SETMASK, &new_mask, &orig_mask);
  if (res)

and, we run commands with all breakpoints lifted:

/* Ask the in-process agent to run a command.  Since we don't want to
   have to handle the IPA hitting breakpoints while running the
   command, we pause all threads, remove all breakpoints, and then set
   the helper thread re-running.  We communicate with the helper
   thread by means of direct memory xfering, and a socket for
   synchronization.  */

static int
run_inferior_command (char *cmd, int len)
{
  int err = -1;
  int pid = ptid_get_pid (current_ptid);

  trace_debug ("run_inferior_command: running: %s", cmd);

  pause_all (0);
  uninsert_all_breakpoints ();

  err = agent_run_command (pid, (const char *) cmd, len);



If we want to reuse target_stop_ptid for other cases in the
future, we'll likely make it return the stop waitstatus then.

For now, I think just documenting target_continue_ptid as
resuming with no signal is good enough.

Thanks,
Pedro Alves