[7/9] Enqueue signal even when resuming threads

  Nowadays, we only enqueue signal when we leave thread pending in
linux_resume_one_thread.  If lwp->resume->sig isn't zero (GDB wants
to resume with signal), we pass lwp->resume->sig to
linux_resume_one_lwp.

In order to reduce the difference between resuming thread with signal
and proceeding thread with signal, when we resume thread, we can
enqueue signal too, and proceed thread.  The signal will be consumed in
linux_resume_one_lwp_throw from lwp->pending_signals.

gdb/gdbserver:

2016-06-29  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (proceed_one_lwp): Declare.
	(linux_resume_one_thread): Remove local variable 'step'.
	Lift code enqueue signal.  Call proceed_one_lwp instead of
	linux_resume_one_lwp.
---
 gdb/gdbserver/linux-low.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)
  

On 06/30/2016 03:09 PM, Yao Qi wrote:
> Nowadays, we only enqueue signal when we leave thread pending in
> linux_resume_one_thread.  If lwp->resume->sig isn't zero (GDB wants
> to resume with signal), we pass lwp->resume->sig to
> linux_resume_one_lwp.
> 
> In order to reduce the difference between resuming thread with signal
> and proceeding thread with signal, when we resume thread, we can
> enqueue signal too, and proceed thread.  The signal will be consumed in
> linux_resume_one_lwp_throw from lwp->pending_signals.

This makes one subtle change.  If the thread already
had a pending signal, and we're getting a resume request with
a signal that is a different signal from the one already queued,
then before the patch, we'd tell the kernel to deliver the new
signal first, and then only deliver the pending signal the next time
the thread is resumed.  While after the patch, we'll enqueue the
new signal, and deliver the one that was already pending first.

Can't really say whether the old behavior was necessary.  At least
the new behavior seems to make order or signal delivery consistent
with how the order the signals were made pending in the
first place, so seems better.

This made me notice that this scenario of having more than one
pending signal doesn't seem to be handled perfectly.  We deliver
the first signal, but nothing makes sure to get back control
of the thread immediately in order to deliver the other pending
signal, so it seems the thread may execute a bunch of arbitrary code
until it next stops and is re-resumed, at which point we'll deliver
the other pending signal.  Maybe the simplest would be to
force the thread to immediately stop again, by calling
send_sigstop before resuming it, whenever we have more pending
signals.  Subject for a separate patch, in any case.

Thanks,
Pedro Alves
  

On Fri, Jul 1, 2016 at 4:06 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2016 03:09 PM, Yao Qi wrote:
>> Nowadays, we only enqueue signal when we leave thread pending in
>> linux_resume_one_thread.  If lwp->resume->sig isn't zero (GDB wants
>> to resume with signal), we pass lwp->resume->sig to
>> linux_resume_one_lwp.
>>
>> In order to reduce the difference between resuming thread with signal
>> and proceeding thread with signal, when we resume thread, we can
>> enqueue signal too, and proceed thread.  The signal will be consumed in
>> linux_resume_one_lwp_throw from lwp->pending_signals.
>
> This makes one subtle change.  If the thread already
> had a pending signal, and we're getting a resume request with
> a signal that is a different signal from the one already queued,
> then before the patch, we'd tell the kernel to deliver the new
> signal first, and then only deliver the pending signal the next time
> the thread is resumed.  While after the patch, we'll enqueue the
> new signal, and deliver the one that was already pending first.
>
> Can't really say whether the old behavior was necessary.  At least
> the new behavior seems to make order or signal delivery consistent
> with how the order the signals were made pending in the
> first place, so seems better.
>
> This made me notice that this scenario of having more than one
> pending signal doesn't seem to be handled perfectly.  We deliver
> the first signal, but nothing makes sure to get back control
> of the thread immediately in order to deliver the other pending
> signal, so it seems the thread may execute a bunch of arbitrary code
> until it next stops and is re-resumed, at which point we'll deliver
> the other pending signal.  Maybe the simplest would be to
> force the thread to immediately stop again, by calling
> send_sigstop before resuming it, whenever we have more pending
> signals.  Subject for a separate patch, in any case.

You meant "after resuming it" rather than "before resuming it", right?  We have
two pending signals, so we resume the lwp and deliver the first signal.  After
resuming, we need to immediately deliver the second signal, so we call
send_sigstop.

IIUC, this patch is OK as-is, right?
  

On 07/01/2016 05:45 PM, Yao Qi wrote:

> You meant "after resuming it" rather than "before resuming it", right?  We have
> two pending signals, so we resume the lwp and deliver the first signal.  After
> resuming, we need to immediately deliver the second signal, so we call
> send_sigstop.

No, I really meant "before resuming it".  We'd queue a SIGSTOP
in the kernel, and then resume the thread with
PTRACE_CONTINUE/STEP + signal.  The idea being that the thread would
continue out of the signal delivery path in the kernel side with
the signal we resume it with, so if there's a signal handler,
it's what the kernel makes the thread execute as soon as it reaches
userspace.  But given we had _also_ queued a SIGSTOP, the thread would
immediately report the SIGSTOP before it had a chance of executing the
first instruction of the handler.  IOW, it'd report the SIGSTOP in
the first instruction of the handler, or where it was already
stopped before, if the signal signal passed to PTRACE_CONTINUE
is SIG_IGN.

Seeing the thread stop for a SIGSTOP that gdbserver had itself
sent, gdbserver would immediately re-resume the thread, this time,
with the other pending signal.  This latter part probably already
works without any change.  See tail end of linux_low_filter_event,
where we have:

  if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGSTOP
      && child->stop_expected)
    {
      if (debug_threads)
	debug_printf ("Expected stop.\n");
      child->stop_expected = 0;

> IIUC, this patch is OK as-is, right?
> 

Yes.

Thanks,
Pedro Alves
  

On 07/01/2016 05:55 PM, Pedro Alves wrote:
> On 07/01/2016 05:45 PM, Yao Qi wrote:
> 
>> You meant "after resuming it" rather than "before resuming it", right?  We have
>> two pending signals, so we resume the lwp and deliver the first signal.  After
>> resuming, we need to immediately deliver the second signal, so we call
>> send_sigstop.
> 
> No, I really meant "before resuming it".  We'd queue a SIGSTOP
> in the kernel, and then resume the thread with
> PTRACE_CONTINUE/STEP + signal.  The idea being that the thread would
> continue out of the signal delivery path in the kernel side with
> the signal we resume it with, so if there's a signal handler,
> it's what the kernel makes the thread execute as soon as it reaches
> userspace.  But given we had _also_ queued a SIGSTOP, the thread would
> immediately report the SIGSTOP before it had a chance of executing the
> first instruction of the handler.  IOW, it'd report the SIGSTOP in
> the first instruction of the handler, or where it was already
> stopped before, if the signal signal passed to PTRACE_CONTINUE
> is SIG_IGN.

Eh, actually, if this works, looks like I just come up with a way
to step into a signal handler on software single-step targets.

Thanks,
Pedro Alves

> 
> Seeing the thread stop for a SIGSTOP that gdbserver had itself
> sent, gdbserver would immediately re-resume the thread, this time,
> with the other pending signal.  This latter part probably already
> works without any change.  See tail end of linux_low_filter_event,
> where we have:
> 
>   if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGSTOP
>       && child->stop_expected)
>     {
>       if (debug_threads)
> 	debug_printf ("Expected stop.\n");
>       child->stop_expected = 0;
> 
>> IIUC, this patch is OK as-is, right?
>>
> 
> Yes.
> 
> Thanks,
> Pedro Alves
>