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

[Luis Machado <lgustavo@codesourcery.com>]

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.