From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5768 invoked by alias); 29 Jun 2009 18:41:00 -0000 Received: (qmail 5753 invoked by uid 22791); 29 Jun 2009 18:40:57 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 29 Jun 2009 18:40:49 +0000 Received: (qmail 29177 invoked from network); 29 Jun 2009 18:40:47 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 29 Jun 2009 18:40:47 -0000 From: Pedro Alves To: Jan Kratochvil Subject: Re: [patch] Fix internal-error on dead LWPs with no associated thread Date: Mon, 29 Jun 2009 18:41:00 -0000 User-Agent: KMail/1.9.10 Cc: gdb-patches@sourceware.org References: <20090629100922.GA26882@host0.dyn.jankratochvil.net> <20090629174929.GA9652@host0.dyn.jankratochvil.net> <20090629182852.GA14913@host0.dyn.jankratochvil.net> In-Reply-To: <20090629182852.GA14913@host0.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200906291941.52129.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-06/txt/msg00840.txt.bz2 On Monday 29 June 2009 19:28:52, Jan Kratochvil wrote: > On Mon, 29 Jun 2009 19:49:29 +0200, Jan Kratochvil wrote: > > > Does anyone know why does the new_thread_event bit need to resume the target > > > at all? Removing that resume should fix the issue too. > > Attached here. (this one is for CVS HEAD so it still includes your patch above) Haya! This is much more than I said... 1 - I'm not convinced currently that adding threads immediately to the list in all-stop mode in linux_handle_extended_wait is a good idea. See here for thoughts around that: http://sourceware.org/ml/gdb/2009-05/msg00067.html 2 - If the target has let the thread escape this far without having added it to the list, *and* the target needs to book-keep extra thread info associated with the thread structure, than your patch looks like paparing over a bug. It's just a simple to handle it in the target's target_wait implementation, just before returning an event. 3 - I really just meant to just remove this whole block: - if (ecs->new_thread_event) - { - if (non_stop) - /* Non-stop assumes that the target handles adding new threads - to the thread list. */ - internal_error (__FILE__, __LINE__, "\ -targets should add new threads to the thread list themselves in non-stop mode."); - - /* We may want to consider not doing a resume here in order to - give the user a chance to play with the new thread. It might - be good to make that a user-settable option. */ - - /* At this point, all threads are stopped (happens automatically - in either the OS or the native code). Therefore we need to - continue all threads in order to make progress. */ - - if (!ptid_equal (ecs->ptid, inferior_ptid)) - context_switch (ecs->ptid); - target_resume (RESUME_ALL, 0, TARGET_SIGNAL_0); - prepare_to_wait (ecs); - return; - } > > Expecting you are right but only for GNU/Linux. > > This patch has no regressions on {x86_64,i686}-fedora-linux-gnu. > > > Thanks, > Jan > > > gdb/ > 2009-06-29 Jan Kratochvil > > Attach new threads in the GNU/Linux all-stop mode immediately. > * inferior.h (default_new_thread_found): New prototype. > * infrun.c (handle_inferior_event ): Call > target_new_thread_found, move the original code to ... > (default_new_thread_found): ... a new function. > * linux-nat.c (linux_handle_extended_wait): Create the GDB structures > for new thread even in the all-stop mode. New comment point 3. > (linux_nat_new_thread_found): New function. > (linux_nat_add_target): Register linux_nat_new_thread_found. > * target.c (update_current_target): Initialize to_new_thread_found. > * target.h (struct target_ops): New field `to_new_thread_found'. > (target_new_thread_found): New macro. > > --- a/gdb/inferior.h > +++ b/gdb/inferior.h > @@ -244,6 +244,8 @@ extern void ensure_not_running (void); > > void set_step_info (struct frame_info *frame, struct symtab_and_line sal); > > +extern void default_new_thread_found (ptid_t ptid); > + > /* From infcmd.c */ > > extern void tty_command (char *, int); > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -2732,21 +2732,7 @@ handle_inferior_event (struct execution_control_state *ecs) > > if (ecs->new_thread_event) > { > - if (non_stop) > - /* Non-stop assumes that the target handles adding new threads > - to the thread list. */ > - internal_error (__FILE__, __LINE__, "\ > -targets should add new threads to the thread list themselves in non-stop mode."); > - > - /* We may want to consider not doing a resume here in order to > - give the user a chance to play with the new thread. It might > - be good to make that a user-settable option. */ > - > - /* At this point, all threads are stopped (happens automatically > - in either the OS or the native code). Therefore we need to > - continue all threads in order to make progress. */ > - > - target_resume (RESUME_ALL, 0, TARGET_SIGNAL_0); > + target_new_thread_found (ecs->ptid); > prepare_to_wait (ecs); > return; > } > @@ -4116,6 +4102,34 @@ infrun: not switching back to stepped thread, it has vanished\n"); > keep_going (ecs); > } > > +/* Handle new unexpected LWP PTID. At this point, both the parent and the new > + child LWP are stopped (happens automatically in either the OS or the native > + code). Therefore we need to continue all threads in order to make progress. > + > + GNU/Linux support uses linux_nat_new_thread_found instead. */ > + > +void > +default_new_thread_found (ptid_t ptid) > +{ > + if (non_stop) > + /* Non-stop assumes that the target handles adding new threads > + to the thread list. */ > + internal_error (__FILE__, __LINE__, "\ > +targets should add new threads to the thread list themselves in non-stop mode."); > + > + /* We may want to consider not doing a resume here in order to > + give the user a chance to play with the new thread. It might > + be good to make that a user-settable option. */ > + > + /* target_resume requires valid INFERIOR_PTID while it may have been deleted > + in the meantime. At least the new PTID was now created by add_thread. */ > + > + if (!ptid_equal (ptid, inferior_ptid)) > + context_switch (ptid); > + > + target_resume (RESUME_ALL, 0, TARGET_SIGNAL_0); > +} > + > /* Is thread TP in the middle of single-stepping? */ > > static int > --- a/gdb/linux-nat.c > +++ b/gdb/linux-nat.c > @@ -1843,35 +1843,32 @@ linux_handle_extended_wait (struct lwp_info *lp, int status, > else > status = 0; > > - if (non_stop) > - { > - /* Add the new thread to GDB's lists as soon as possible > - so that: > + /* Add the new thread to GDB's lists as soon as possible > + so that: > > - 1) the frontend doesn't have to wait for a stop to > - display them, and, > + 1) the frontend doesn't have to wait for a stop to > + display them, and, > > - 2) we tag it with the correct running state. */ > + 2) we tag it with the correct running state. > > - /* If the thread_db layer is active, let it know about > - this new thread, and add it to GDB's list. */ > - if (!thread_db_attach_lwp (new_lp->ptid)) > - { > - /* We're not using thread_db. Add it to GDB's > - list. */ > - target_post_attach (GET_LWP (new_lp->ptid)); > - add_thread (new_lp->ptid); > - } > + 3) we do not consider the first breakpoint just as > + a NEW_THREAD_EVENT. */ > > - if (!stopping) > - { > - set_running (new_lp->ptid, 1); > - set_executing (new_lp->ptid, 1); > - } > + /* If the thread_db layer is active, let it know about > + this new thread, and add it to GDB's list. */ > + if (!thread_db_attach_lwp (new_lp->ptid)) > + { > + /* We're not using thread_db. Add it to GDB's > + list. */ > + target_post_attach (GET_LWP (new_lp->ptid)); > + add_thread (new_lp->ptid); > } > > if (!stopping) > { > + set_running (new_lp->ptid, 1); > + set_executing (new_lp->ptid, 1); > + > new_lp->stopped = 0; > new_lp->resumed = 1; > ptrace (PTRACE_CONT, new_pid, 0, > @@ -1926,6 +1923,20 @@ linux_handle_extended_wait (struct lwp_info *lp, int status, > _("unknown ptrace event %d"), event); > } > > +/* linux_handle_extended_wait already creates the GDB structures for the new > + LWP PTID. LWPs therefore should not appear unexpectedly. */ > + > +static void > +linux_nat_new_thread_found (ptid_t ptid) > +{ > + if (linux_supports_tracefork (GET_LWP (ptid))) > + internal_error (__FILE__, __LINE__, ("\ > +Unexpected %s despite PTRACE_O_TRACEFORK is supported on this GNU/Linux OS."), > + target_pid_to_str (ptid)); > + > + linux_ops->to_new_thread_found (ptid); > +} > + > /* Wait for LP to stop. Returns the wait status, or 0 if the LWP has > exited. */ > > @@ -4599,6 +4610,8 @@ linux_nat_add_target (struct target_ops *t) > > t->to_supports_multi_process = linux_nat_supports_multi_process; > > + t->to_new_thread_found = linux_nat_new_thread_found; > + > /* We don't change the stratum; this target will sit at > process_stratum and thread_db will set at thread_stratum. This > is a little strange, since this is a multi-threaded-capable > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -634,6 +634,7 @@ update_current_target (void) > INHERIT (to_get_ada_task_ptid, t); > /* Do not inherit to_search_memory. */ > INHERIT (to_supports_multi_process, t); > + INHERIT (to_new_thread_found, t); > INHERIT (to_magic, t); > /* Do not inherit to_memory_map. */ > /* Do not inherit to_flash_erase. */ > @@ -777,6 +778,8 @@ update_current_target (void) > de_fault (to_supports_multi_process, > (int (*) (void)) > return_zero); > + de_fault (to_new_thread_found, > + default_new_thread_found); > #undef de_fault > > /* Finally, position the target-stack beneath the squashed > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -542,6 +542,9 @@ struct target_ops > simultaneously? */ > int (*to_supports_multi_process) (void); > > + /* An event was received for a so far unknown PTID. */ > + void (*to_new_thread_found) (ptid_t ptid); > + > int to_magic; > /* Need sub-structure for target machine related rather than comm related? > */ > @@ -1118,6 +1121,9 @@ extern char *normal_pid_to_str (ptid_t ptid); > (current_target.to_can_execute_reverse ? \ > current_target.to_can_execute_reverse () : 0) > > +#define target_new_thread_found \ > + (*current_target.to_new_thread_found) > + > extern const struct target_desc *target_read_description (struct target_ops *); > > #define target_get_ada_task_ptid(lwp, tid) \ > -- Pedro Alves