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

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

[John Baldwin]

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.
---
 gdb/ChangeLog  |  8 ++++++++
 gdb/fbsd-nat.c | 60 +++++++++++++++++++++++++---------------------------------
 2 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fc8dbe18da..a1927f5e2e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2017-04-04  John Baldwin  <jhb@FreeBSD.org>
+
+	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.
+
 2017-04-04  Simon Marchi  <simon.marchi@polymtl.ca>
 
 	* remote.c (set_general_thread, set_continue_thread): Use ptid_t
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index d99f436070..f80a47ba42 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -653,38 +653,6 @@ fbsd_next_vfork_done (void)
 #endif
 #endif
 
-static int
-resume_one_thread_cb (struct thread_info *tp, void *data)
-{
-  ptid_t *ptid = (ptid_t *) data;
-  int request;
-
-  if (ptid_get_pid (tp->ptid) != ptid_get_pid (*ptid))
-    return 0;
-
-  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"));
-  return 0;
-}
-
-static int
-resume_all_threads_cb (struct thread_info *tp, void *data)
-{
-  ptid_t *filter = (ptid_t *) data;
-
-  if (!ptid_match (tp->ptid, *filter))
-    return 0;
-
-  if (ptrace (PT_RESUME, ptid_get_lwp (tp->ptid), NULL, 0) == -1)
-    perror_with_name (("ptrace"));
-  return 0;
-}
-
 /* Implement the "to_resume" target_ops method.  */
 
 static void
@@ -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"));
+	}
     }
   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"));
+	}
       ptid = inferior_ptid;
     }
   super_resume (ops, ptid, step, signo);
-- 
2.11.0

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

[John Baldwin]

Ping?

On Tuesday, April 04, 2017 10:32:58 AM 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.
> ---
>  gdb/ChangeLog  |  8 ++++++++
>  gdb/fbsd-nat.c | 60 +++++++++++++++++++++++++---------------------------------
>  2 files changed, 34 insertions(+), 34 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index fc8dbe18da..a1927f5e2e 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,11 @@
> +2017-04-04  John Baldwin  <jhb@FreeBSD.org>
> +
> +	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.
> +
>  2017-04-04  Simon Marchi  <simon.marchi@polymtl.ca>
>  
>  	* remote.c (set_general_thread, set_continue_thread): Use ptid_t
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index d99f436070..f80a47ba42 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -653,38 +653,6 @@ fbsd_next_vfork_done (void)
>  #endif
>  #endif
>  
> -static int
> -resume_one_thread_cb (struct thread_info *tp, void *data)
> -{
> -  ptid_t *ptid = (ptid_t *) data;
> -  int request;
> -
> -  if (ptid_get_pid (tp->ptid) != ptid_get_pid (*ptid))
> -    return 0;
> -
> -  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"));
> -  return 0;
> -}
> -
> -static int
> -resume_all_threads_cb (struct thread_info *tp, void *data)
> -{
> -  ptid_t *filter = (ptid_t *) data;
> -
> -  if (!ptid_match (tp->ptid, *filter))
> -    return 0;
> -
> -  if (ptrace (PT_RESUME, ptid_get_lwp (tp->ptid), NULL, 0) == -1)
> -    perror_with_name (("ptrace"));
> -  return 0;
> -}
> -
>  /* Implement the "to_resume" target_ops method.  */
>  
>  static void
> @@ -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"));
> +	}
>      }
>    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"));
> +	}
>        ptid = inferior_ptid;
>      }
>    super_resume (ops, ptid, step, signo);
> 


-- 
John Baldwin

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

[Luis Machado]

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.

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

[John Baldwin]

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

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

[Luis Machado]

On 04/14/2017 05:40 PM, John Baldwin wrote:
> 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:

Ah, that could very well be it.

>
>       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"));
>         }
>

The indentation here looks fine indeed.

>> 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.
>

Sorry, i swapped out the context of v1.

>> 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).
>

That sounds like a good idea and could be postponed to a more convenient 
time.

>> 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.
>

Great. I have no further comments.

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

[John Baldwin]

On Friday, April 14, 2017 08:01:25 PM Luis Machado wrote:
> On 04/14/2017 05:40 PM, John Baldwin wrote:
> >> 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.
> >
> 
> Great. I have no further comments.

To be clear (since I believe I messed this up before), I still need approval
from an approver?

-- 
John Baldwin

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

[Luis Machado]

On 04/17/2017 01:27 PM, John Baldwin wrote:
> On Friday, April 14, 2017 08:01:25 PM Luis Machado wrote:
>> On 04/14/2017 05:40 PM, John Baldwin wrote:
>>>> 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.
>>>
>>
>> Great. I have no further comments.
>
> To be clear (since I believe I messed this up before), I still need approval
> from an approver?
>

Right. I'm mainly reviewing. I can't formally approve.

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

[Pedro Alves]

On 04/04/2017 06: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.

LGTM.  OK for master and 8.0.

Thanks,
Pedro Alves

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

[Simon Marchi]

On 2017-04-17 14:27, John Baldwin wrote:
> On Friday, April 14, 2017 08:01:25 PM Luis Machado wrote:
>> On 04/14/2017 05:40 PM, John Baldwin wrote:
>> >> 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.
>> >
>> 
>> Great. I have no further comments.
> 
> To be clear (since I believe I messed this up before), I still need 
> approval
> from an approver?

I read both the v1 and v2 threads to get the context, the patch looks 
good to me.  Pedro was fine with the ALL_NON_EXITED_THREADS approach as 
well, so go ahead and push.  I think it should go in the 8.0 branch as 
well.

Btw, do you know why the double parenthesis in

   perror_with_name (("ptrace"));

?  Feel free to remove the extra parenthesis before pushing.

Thanks,

Simon

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

[Simon Marchi]

On 2017-04-18 10:27, Simon Marchi wrote:
> I read both the v1 and v2 threads to get the context, the patch looks
> good to me.  Pedro was fine with the ALL_NON_EXITED_THREADS approach
> as well, so go ahead and push.  I think it should go in the 8.0 branch
> as well.
> 
> Btw, do you know why the double parenthesis in
> 
>   perror_with_name (("ptrace"));
> 
> ?  Feel free to remove the extra parenthesis before pushing.
> 
> Thanks,
> 
> Simon

Oops, I missed Pedro's reply before replying.

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

[Pedro Alves]

On 04/18/2017 03:27 PM, Simon Marchi wrote:

> Btw, do you know why the double parenthesis in
> 
>   perror_with_name (("ptrace"));
> 
> ?  Feel free to remove the extra parenthesis before pushing.

That's to quiet the ARI.  Otherwise it complains about the
missing _() for i18n.

Thanks,
Pedro Alves

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

[John Baldwin]

On Tuesday, April 18, 2017 10:27:51 AM Simon Marchi wrote:
> On 2017-04-17 14:27, John Baldwin wrote:
> > On Friday, April 14, 2017 08:01:25 PM Luis Machado wrote:
> >> On 04/14/2017 05:40 PM, John Baldwin wrote:
> >> >> 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.
> >> >
> >> 
> >> Great. I have no further comments.
> > 
> > To be clear (since I believe I messed this up before), I still need 
> > approval
> > from an approver?
> 
> I read both the v1 and v2 threads to get the context, the patch looks 
> good to me.  Pedro was fine with the ALL_NON_EXITED_THREADS approach as 
> well, so go ahead and push.  I think it should go in the 8.0 branch as 
> well.

Thanks, pushed to both.

-- 
John Baldwin