[2/3] Initialize last_resume_kind for remote fork child

This patch fixes some intermittent test failures in
gdb.base/foll-vfork.exp where a vfork child could sometimes be
(incorrectly) resumed when handling the vfork event.  In this
case the result was a subsequent event reported to the client
side as a SIGTRAP delivered to the as-yet-unknown child thread.

The fix is to initialize the threadinfo.last_resume_kind field
for new fork children in gdbserver/linux-low.c:handle_extended_wait.
This prevents the event handler from incorrectly resuming the child
if the stop event that is generated when it starts is reported
before the vfork event that initiated it.  The same thing is done
for the native case in linux-nat.c:linux_child_follow_fork.

In addition, it seemed prudent to initialize lwp_info.status_pending_p
for the new fork child.  I also rearranged the initialization code
so that all of the lwp_info initialization was together, rather than 
intermixed with thread_info and process_info initialization.

Tested native, native-gdbserver, native-extended-gdbserver on
x86_64 GNU/Linux.

OK?

Thanks
--Don

gdb/gdbserver/
2015-05-22  Don Breazeal  <donb@codesourcery.com>

	* linux-low.c (handle_extended_wait):

---
 gdb/gdbserver/linux-low.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

On 05/22/2015 07:55 PM, Don Breazeal wrote:
> This patch fixes some intermittent test failures in
> gdb.base/foll-vfork.exp where a vfork child could sometimes be
> (incorrectly) resumed when handling the vfork event.  In this
> case the result was a subsequent event reported to the client
> side as a SIGTRAP delivered to the as-yet-unknown child thread.
> 
> The fix is to initialize the threadinfo.last_resume_kind field
> for new fork children in gdbserver/linux-low.c:handle_extended_wait.
> This prevents the event handler from incorrectly resuming the child
> if the stop event that is generated when it starts is reported
> before the vfork event that initiated it.  

I'm not seeing how what comes first makes a difference, actually.

> The same thing is done
> for the native case in linux-nat.c:linux_child_follow_fork.

The code that resumes the child by mistake in question must be
resume_stopped_resumed_lwps, and that is called similarly by
both backends after pulling events out of the kernel, and right
after linux_*_filter_event (which is what calls into handle_extended_wait):

      /* Now that we've pulled all events out of the kernel, resume
	 LWPs that don't have an interesting event to report.  */
      iterate_over_lwps (minus_one_ptid,
			 resume_stopped_resumed_lwps, &minus_one_ptid);

...

     /* Now that we've pulled all events out of the kernel, resume
	 LWPs that don't have an interesting event to report.  */
      if (stopping_threads == NOT_STOPPING_THREADS)
	for_each_inferior (&all_threads, resume_stopped_resumed_lwps);

That happens before linux_child_follow_fork is reached.

The first difference is that gdb's version does not add the fork
child to the lwp list in handle_extended_wait.  But even if it did,
the way linux-nat.c tags lwps as "not stopped-resumed" is different
in gdb and gdbserver.  gdb's has:

static int
resume_stopped_resumed_lwps (struct lwp_info *lp, void *data)
{
  ptid_t *wait_ptid_p = data;

  if (lp->stopped
      && lp->resumed
      && !lwp_status_pending_p (lp))
    {

while gdbserver's:

static void
resume_stopped_resumed_lwps (struct inferior_list_entry *entry)
{
  struct thread_info *thread = (struct thread_info *) entry;
  struct lwp_info *lp = get_thread_lwp (thread);

  if (lp->stopped
      && !lp->status_pending_p
      && thread->last_resume_kind != resume_stop
      && thread->last_status.kind == TARGET_WAITKIND_IGNORE)
    {

So the main difference here is that linux-nat.c checks that "resumed" flag
which doesn't exist in the gdbserver version.  Checking lwp->resumed in
gdb is equivalent to checking:

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

in gdbserver/linux-low.c.  gdbserver/linux-low.c's add_thread function
creates threads with last_resume_kind == resume_continue by default.
So the fix is to make sure to override that to resume_stop.  gdb's
linux_child_follow_fork version does the equivalent, but that's not
what exactly prevents a spurious resume (it's the fact that add_lwp
creates lwps with the 'resume' flag cleared).

> 
> In addition, it seemed prudent to initialize lwp_info.status_pending_p
> for the new fork child.  

That's fine.  I think the child can well report a signal other than
SIGSTOP first (or even be killed/disappear), in which case we should
leave it pending (thus set status_pending_p), but that's a separate,
preexisting issue.

> I also rearranged the initialization code
> so that all of the lwp_info initialization was together, rather than 
> intermixed with thread_info and process_info initialization.
> 
> Tested native, native-gdbserver, native-extended-gdbserver on
> x86_64 GNU/Linux.
> 
> OK?

So the fix is OK, though I think the commit log could be
clarified.

> 
> Thanks
> --Don
> 
> gdb/gdbserver/
> 2015-05-22  Don Breazeal  <donb@codesourcery.com>
> 
> 	* linux-low.c (handle_extended_wait):

Seems incomplete.  :-)

> 
> ---
>  gdb/gdbserver/linux-low.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 9f3ea48..d763c66 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -457,6 +457,7 @@ handle_extended_wait (struct lwp_info *event_lwp, int wstat)
>  	  struct process_info *parent_proc;
>  	  struct process_info *child_proc;
>  	  struct lwp_info *child_lwp;
> +	  struct thread_info *child_thr;
>  	  struct target_desc *tdesc;
>  
>  	  ptid = ptid_build (new_pid, new_pid, 0);
> @@ -479,6 +480,10 @@ handle_extended_wait (struct lwp_info *event_lwp, int wstat)
>  	  child_lwp = add_lwp (ptid);
>  	  gdb_assert (child_lwp != NULL);
>  	  child_lwp->stopped = 1;
> +	  child_lwp->must_set_ptrace_flags = 1;
> +	  child_lwp->status_pending_p = 0;
> +	  child_thr = get_lwp_thread (child_lwp);
> +	  child_thr->last_resume_kind = resume_stop;
>  	  parent_proc = get_thread_process (event_thr);
>  	  child_proc->attached = parent_proc->attached;
>  	  clone_all_breakpoints (&child_proc->breakpoints,
> @@ -488,7 +493,6 @@ handle_extended_wait (struct lwp_info *event_lwp, int wstat)
>  	  tdesc = xmalloc (sizeof (struct target_desc));
>  	  copy_target_description (tdesc, parent_proc->tdesc);
>  	  child_proc->tdesc = tdesc;
> -	  child_lwp->must_set_ptrace_flags = 1;
>  
>  	  /* Clone arch-specific process data.  */
>  	  if (the_low_target.new_fork != NULL)
> 

Thanks,
Pedro Alves