Re: [PATCH v2] PR threads/20743: Don't attempt to suspend or resume exited threads.

[John Baldwin <jhb at freebsd dot org>]

On Wednesday, April 12, 2017 01:11:45 PM Luis Machado wrote:
> On 04/04/2017 12:32 PM, John Baldwin wrote:
> > When resuming a native FreeBSD process, ignore exited threads when
> > suspending/resuming individual threads prior to continuing the process.
> >
> > gdb/ChangeLog:
> >
> > 	PR threads/20743
> > 	* fbsd-nat.c (resume_one_thread_cb): Remove.
> > 	(resume_all_threads_cb): Remove.
> > 	(fbsd_resume): Use ALL_NON_EXITED_THREADS instead of
> > 	iterate_over_threads.
> ...
> > @@ -711,13 +679,37 @@ fbsd_resume (struct target_ops *ops,
> >    if (ptid_lwp_p (ptid))
> >      {
> >        /* If ptid is a specific LWP, suspend all other LWPs in the process.  */
> > -      iterate_over_threads (resume_one_thread_cb, &ptid);
> > +      struct thread_info *tp;
> > +      int request;
> > +
> > +      ALL_NON_EXITED_THREADS (tp)
> > +        {
> > +	  if (ptid_get_pid (tp->ptid) != ptid_get_pid (ptid))
> > +	    continue;
> > +
> > +	  if (ptid_get_lwp (tp->ptid) == ptid_get_lwp (ptid))
> > +	    request = PT_RESUME;
> > +	  else
> > +	    request = PT_SUSPEND;
> > +
> > +	  if (ptrace (request, ptid_get_lwp (tp->ptid), NULL, 0) == -1)
> > +	    perror_with_name (("ptrace"));
> > +	}
> 
> Identation of the ALL_NON_EXITED_THREADS block is off. I'd check the 
> identation of the entire block to make sure it is sane.

Hmm, the raw code looks fine.  I know that my MUA (kmail) messes up formatting
of code as it displays tabs as 4 characters instead of 8?  Here's the raw
code with tabs expanded to spaces:

      ALL_NON_EXITED_THREADS (tp)
        {
          if (ptid_get_pid (tp->ptid) != ptid_get_pid (ptid))
            continue;

          if (ptid_get_lwp (tp->ptid) == ptid_get_lwp (ptid))
            request = PT_RESUME;
          else
            request = PT_SUSPEND;

          if (ptrace (request, ptid_get_lwp (tp->ptid), NULL, 0) == -1)
            perror_with_name (("ptrace"));
        }

> A question i have is why did we have to remove the original functions. 
> Couldn't we have checked the non-exited-ness of the threads inside the 
> callback?

That was what the V1 patch did, but you and Pedro requested it use
ALL_NON_EXITED_THREADS instead, hence version 2.

> Another bit... Since we're changing this code, might as well improve the 
> perror message so it is more meaningful?

I could perhaps do a followup to include the ptrace op in the various
perror's in this file (all of them use this, as do the various BSD
nat.c files used for register fetch/store).

> Otherwise i have no further comments. I assume you ran gdb's testsuite 
> against this change and verified the results are sane?

There were no regressions at least.  With the stock tree there are
several unexpected failures already which I will get to at some point.

-- 
John Baldwin