| Message ID | 20170404173258.6512-1-jhb@FreeBSD.org |
|---|---|
| State | New, archived |
| Headers |
Received: (qmail 46706 invoked by alias); 4 Apr 2017 17:33:46 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 46691 invoked by uid 89); 4 Apr 2017 17:33:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=resuming X-HELO: mail.baldwin.cx Received: from bigwig.baldwin.cx (HELO mail.baldwin.cx) (96.47.65.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 04 Apr 2017 17:33:43 +0000 Received: from ralph.baldwin.cx.com (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id 4250410A8C0 for <gdb-patches@sourceware.org>; Tue, 4 Apr 2017 13:33:43 -0400 (EDT) From: John Baldwin <jhb@FreeBSD.org> To: gdb-patches@sourceware.org Subject: [PATCH v2] PR threads/20743: Don't attempt to suspend or resume exited threads. Date: Tue, 4 Apr 2017 10:32:58 -0700 Message-Id: <20170404173258.6512-1-jhb@FreeBSD.org> X-IsSubscribed: yes |
Commit Message
John Baldwin
April 4, 2017, 5:32 p.m. UTC
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(-)
Comments
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); >
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.
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.
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.
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?
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.
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
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
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.
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
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.
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);