[11/18] gdbserver: fix killed-outside.exp

  killed-outside.exp regresses with "maint set target-non-stop on".  The
logs show:

 (gdb) continue
 Continuing.
 infrun: clear_proceed_status_thread (Thread 9028.9028)
 infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
 infrun: proceed: resuming Thread 9028.9028
 Sending packet: $Z0,3615a03966,1#4b...  Notification received: Stop:X9;process:2344
 Packet received: E01
 Sending packet: $Z0,3615a13970,1#47...Packet received: E01
 Sending packet: $Z0,3615a14891,1#4a...Packet received: E01
 infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [Thread 9028.9028] at 0x4005e4
 Sending packet: $vCont;c:p2344.2344#1a...Packet received: E.target not running.
 Sending packet: $qXfer:threads:read::0,fff#03...Packet received: l<threads>\n</threads>\n
 Sending packet: $vStopped#55...Packet received: OK
 Unexpected vCont reply in non-stop mode: E.target not running.
 (gdb) remote_async_inferior_event_handler
 infrun: target_wait (-1.0.0, status) =
 infrun:   9028.0.0 [process 9028],
 infrun:   status->kind = signalled, signal = GDB_SIGNAL_KILL
 infrun: TARGET_WAITKIND_SIGNALLED

 Program terminated with signal SIGKILL, Killed.
 The program no longer exists.
 infrun: stop_waiting
 infrun: clear_step_over_info
 infrun: stop_all_threads
 remote_thread_exit_events(1)

Note the "Unexpected vCont reply" error.

I traced it to a problem in status_pending_p_callback.  It resumes an
LWP when it shouldn't.

gdb/gdbserver/ChangeLog:
2015-10-14  Pedro Alves  <palves@redhat.com>

	* linux-low.c (thread_still_has_status_pending_p): Don't check
	vCont;t here.
	(lwp_resumed): New function.
	(status_pending_p_callback): Return early if the LWP is not
	supposed to be resumed.
---
 gdb/gdbserver/linux-low.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)
  

Pedro Alves <palves@redhat.com> writes:

> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 1c447c2..af9ca22 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -1535,12 +1535,6 @@ thread_still_has_status_pending_p (struct thread_info *thread)
>    if (!lp->status_pending_p)
>      return 0;
>  
> -  /* If we got a `vCont;t', but we haven't reported a stop yet, do
> -     report any status pending the LWP may have.  */
> -  if (thread->last_resume_kind == resume_stop
> -      && thread->last_status.kind != TARGET_WAITKIND_IGNORE)
> -    return 0;
> -

Shouldn't we return 1 instead of 0 here?  This is consistent with the
comments above.

>    if (thread->last_resume_kind != resume_stop
>        && (lp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT
>  	  || lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT))
> @@ -1597,6 +1591,24 @@ thread_still_has_status_pending_p (struct thread_info *thread)
>    return 1;
>  }
>  
> +/* Returns true if LWP is resumed from the client's perspective.  */
> +
> +static int
> +lwp_resumed (struct lwp_info *lwp)
> +{
> +  struct thread_info *thread = get_lwp_thread (lwp);
> +
> +  if (thread->last_resume_kind != resume_stop)
> +    return 1;
> +
> +  /* Did we get a `vCont;t', but we haven't reported a stop yet?  */
> +  if (thread->last_resume_kind == resume_stop
> +      && thread->last_status.kind == TARGET_WAITKIND_IGNORE)
> +    return 1;
> +

I feel "reported" isn't precise.  Is "got" better?  The code means
GDBserver has already got vCont;t and sent SIGSTOP, but hasn't *got*
stop event out of event loop.  After GDBserver got this event, it then
reports the event back to GDB if needed.
  

Hi Yao,

It's taking me a bit to go through your comments.
Thanks for the review, and sorry about that.

On 10/27/2015 09:48 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
>> index 1c447c2..af9ca22 100644
>> --- a/gdb/gdbserver/linux-low.c
>> +++ b/gdb/gdbserver/linux-low.c
>> @@ -1535,12 +1535,6 @@ thread_still_has_status_pending_p (struct thread_info *thread)
>>    if (!lp->status_pending_p)
>>      return 0;
>>  
>> -  /* If we got a `vCont;t', but we haven't reported a stop yet, do
>> -     report any status pending the LWP may have.  */
>> -  if (thread->last_resume_kind == resume_stop
>> -      && thread->last_status.kind != TARGET_WAITKIND_IGNORE)
>> -    return 0;
>> -
> 
> Shouldn't we return 1 instead of 0 here?  This is consistent with the
> comments above.

I may have misunderstood what you mean, but if we remove this code,
that's what we get -- the "return 1;" at the end is reached.

> 
>>    if (thread->last_resume_kind != resume_stop
>>        && (lp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT
>>  	  || lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT))
>> @@ -1597,6 +1591,24 @@ thread_still_has_status_pending_p (struct thread_info *thread)
>>    return 1;
>>  }
>>  
>> +/* Returns true if LWP is resumed from the client's perspective.  */
>> +
>> +static int
>> +lwp_resumed (struct lwp_info *lwp)
>> +{
>> +  struct thread_info *thread = get_lwp_thread (lwp);
>> +
>> +  if (thread->last_resume_kind != resume_stop)
>> +    return 1;
>> +
>> +  /* Did we get a `vCont;t', but we haven't reported a stop yet?  */
>> +  if (thread->last_resume_kind == resume_stop
>> +      && thread->last_status.kind == TARGET_WAITKIND_IGNORE)
>> +    return 1;
>> +
> 
> I feel "reported" isn't precise.  Is "got" better?  The code means
> GDBserver has already got vCont;t and sent SIGSTOP, but hasn't *got*
> stop event out of event loop.  After GDBserver got this event, it then
> reports the event back to GDB if needed.

Hmm, reported to gdb is really what I mean.  How about this:

  /* Did gdb send us a `vCont;t', but we haven't reported the
     corresponding stop to gdb yet?  If so, the thread is still
     resumed/running from gdb's perspective.  */
  if (thread->last_resume_kind == resume_stop
      && thread->last_status.kind == TARGET_WAITKIND_IGNORE)
    return 1;

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

>>> -  /* If we got a `vCont;t', but we haven't reported a stop yet, do
>>> -     report any status pending the LWP may have.  */
>>> -  if (thread->last_resume_kind == resume_stop
>>> -      && thread->last_status.kind != TARGET_WAITKIND_IGNORE)
>>> -    return 0;
>>> -
>> 
>> Shouldn't we return 1 instead of 0 here?  This is consistent with the
>> comments above.
>
> I may have misunderstood what you mean, but if we remove this code,
> that's what we get -- the "return 1;" at the end is reached.
>

What I meant is to change "return 0" to "return 1", like,

  /* If we got a `vCont;t', but we haven't reported a stop yet, do
     report any status pending the LWP may have.  */
  if (thread->last_resume_kind == resume_stop
      && thread->last_status.kind != TARGET_WAITKIND_IGNORE)
-    return 0;
+    return 1;

that is the same as removing them.

>> 
>>>    if (thread->last_resume_kind != resume_stop
>>>        && (lp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT
>>>  	  || lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT))
>>> @@ -1597,6 +1591,24 @@ thread_still_has_status_pending_p (struct thread_info *thread)
>>>    return 1;
>>>  }
>>>  
>>> +/* Returns true if LWP is resumed from the client's perspective.  */
>>> +
>>> +static int
>>> +lwp_resumed (struct lwp_info *lwp)
>>> +{
>>> +  struct thread_info *thread = get_lwp_thread (lwp);
>>> +
>>> +  if (thread->last_resume_kind != resume_stop)
>>> +    return 1;
>>> +
>>> +  /* Did we get a `vCont;t', but we haven't reported a stop yet?  */
>>> +  if (thread->last_resume_kind == resume_stop
>>> +      && thread->last_status.kind == TARGET_WAITKIND_IGNORE)
>>> +    return 1;
>>> +
>> 
>> I feel "reported" isn't precise.  Is "got" better?  The code means
>> GDBserver has already got vCont;t and sent SIGSTOP, but hasn't *got*
>> stop event out of event loop.  After GDBserver got this event, it then
>> reports the event back to GDB if needed.
>
> Hmm, reported to gdb is really what I mean.  How about this:
>
>   /* Did gdb send us a `vCont;t', but we haven't reported the
>      corresponding stop to gdb yet?  If so, the thread is still
>      resumed/running from gdb's perspective.  */
>   if (thread->last_resume_kind == resume_stop
>       && thread->last_status.kind == TARGET_WAITKIND_IGNORE)
>     return 1;

Oh, sounds it is about thread's state from GDB's perspective.  Patch is
good to me.

By the way, after GDBserver reported the stop GDB, does the thread
changed to

   (thread->last_resume_kind == resume_stop
    && thread->last_status.kind != TARGET_WAITKIND_IGNORE)

it is used in proceed_one_lwp.
  

On 11/26/2015 04:51 PM, Yao Qi wrote:

> By the way, after GDBserver reported the stop GDB, does the thread
> changed to
> 
>    (thread->last_resume_kind == resume_stop
>     && thread->last_status.kind != TARGET_WAITKIND_IGNORE)
> 
> it is used in proceed_one_lwp.
> 

Yes, after the stop is reported out of linux_wait, server.c does:

      else
	{
	  /* We're reporting this thread as stopped.  Update its
	     "want-stopped" state to what the client wants, until it
	     gets a new resume action.  */
	  current_thread->last_resume_kind = resume_stop;
	  current_thread->last_status = last_status;
	}

Thanks,
Pedro Alves