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

John Baldwin
2017-04-14T22:40:40Z

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