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

Luis Machado
2017-04-12T18:12:01Z

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.

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?

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

> }
> else
> {
> /* If ptid is a wildcard, resume all matching threads (they won't run
> until the process is continued however). */
> - iterate_over_threads (resume_all_threads_cb, &ptid);
> + struct thread_info *tp;
> +
> + ALL_NON_EXITED_THREADS (tp)
> + {
> + if (!ptid_match (tp->ptid, ptid))
> + continue;
> +
> + if (ptrace (PT_RESUME, ptid_get_lwp (tp->ptid), NULL, 0) == -1)
> + perror_with_name (("ptrace"));
> + }

Identation is off too.

Same as above for the error message.

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

Other folks can chime in.