Re: [patch] Fix internal-error on dead LWPs with no associated thread

[Pedro Alves <pedro@codesourcery.com>]

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  <jan.kratochvil@redhat.com>
> 
> 	Attach new threads in the GNU/Linux all-stop mode immediately.
> 	* inferior.h (default_new_thread_found): New prototype.
> 	* infrun.c (handle_inferior_event <new_thread_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