Re: [non-stop] 10/10 handle exited threads [was: Re: [non-stop] 10/10 split user/internal threads]

[Daniel Jacobowitz <drow at false dot org>]

On Wed, Jul 02, 2008 at 04:39:07AM +0100, Pedro Alves wrote:
> 	(print_thread_info): Update.  Account for exited threads.  Don't
> 	warn about missed frame restoring here, it's done in the cleanup.

its

> +	  /* Since the context is already set to this new thread,
> +	     reset it's ptid, and reswitch inferior_ptid to it.  */

and here, too.

> @@ -163,6 +223,10 @@ add_thread (ptid_t ptid)
>    return add_thread_with_info (ptid, NULL);
>  }
>  
> +/* Delete thread PTID and notify of thread exit.  If this is
> +   inferior_ptid, don't actually delete it, but tag it as exited and
> +   do the notification.  If PTID is the user selected thread, clear
> +   it.  */
>  void
>  delete_thread (ptid_t ptid)
>  {
> @@ -177,12 +241,35 @@ delete_thread (ptid_t ptid)
>    if (!tp)
>      return;
>  
> +  /* If this is the current thread, or there's code out there that
> +     relies on it existing (refcout > 0) we can't delete yet.  Mark it
> +     as exited, and notify it.  */

refcount

> @@ -747,21 +869,64 @@ restore_current_thread (ptid_t ptid)
>  {
>    if (!ptid_equal (ptid, inferior_ptid))
>      {
> -      switch_to_thread (ptid);
> +      if (non_stop)
> +	context_switch_to (ptid);
> +      else
> +	switch_to_thread (ptid);
>      }
>  }
>  
>  static void
> -restore_selected_frame (struct frame_id a_frame_id)
> +restore_selected_frame (struct frame_id a_frame_id, int frame_level)
>  {
> -  struct frame_info *selected_frame_info = NULL;
> +  struct frame_info *frame = NULL;
> +  int count;
>  
> -  if (frame_id_eq (a_frame_id, null_frame_id))
> -    return;        
> +  gdb_assert (frame_level >= 0);
>  
> -  if ((selected_frame_info = frame_find_by_id (a_frame_id)) != NULL)
> +  /* Restore by level first, check if the frame id is the same as
> +     expected.  If that fails, try restoring by frame id.  If that
> +     fails, nothing to do, just warn the user.  */
> +
> +  count = frame_level;
> +  frame = find_relative_frame (get_current_frame (), &count);
> +  if (count == 0
> +      && frame != NULL
> +      /* Either the frame ids match, of they're both invalid.
> +	 The later case is not failsafe, but since it's highly
> +	 unlikelly the search by level finds the wrong frame,
> +		 it's 99.9(9)% of the times (for all practical
> +		 purposes) safe.  */
> +      && (frame_id_eq (get_frame_id (frame), a_frame_id)
> +	  /* Note: could be better to check every frame_id
> +	     member for equality here.  */
> +	  || (!frame_id_p (get_frame_id (frame))
> +	      && !frame_id_p (a_frame_id))))

Indentation, spelling; "latter" and "unlikely" and "of the time".

> @@ -877,18 +1057,10 @@ thread_apply_command (char *tidlist, int
>    if (*cmd == '\000')
>      error (_("Please specify a command following the thread ID list"));
>  
> -  current_ptid = inferior_ptid;
> -
> -  if (!is_running (inferior_ptid))
> -    saved_frame_id = get_frame_id (get_selected_frame (NULL));
> -  else
> -    saved_frame_id = null_frame_id;
> -  old_chain = make_cleanup_restore_current_thread (inferior_ptid, saved_frame_id);
> -
>    /* Save a copy of the command in case it is clobbered by
>       execute_command */
>    saved_cmd = xstrdup (cmd);
> -  saved_cmd_cleanup_chain = make_cleanup (xfree, (void *) saved_cmd);
> +  old_chain = make_cleanup (xfree, saved_cmd);
>    while (tidlist < cmd)
>      {
>        struct thread_info *tp;
> @@ -926,26 +1098,24 @@ thread_apply_command (char *tidlist, int
>  	    warning (_("Thread %d has terminated."), start);
>  	  else
>  	    {
> +	      make_cleanup_restore_current_thread ();
> +
>  	      if (non_stop)
>  		context_switch_to (tp->ptid);
>  	      else
>  		switch_to_thread (tp->ptid);
> +
>  	      printf_filtered (_("\nThread %d (%s):\n"), tp->num,
>  			       target_tid_to_str (inferior_ptid));
>  	      execute_command (cmd, from_tty);
> -	      strcpy (cmd, saved_cmd);	/* Restore exact command used previously */
> +
> +	      /* Restore exact command used previously.  */
> +	      strcpy (cmd, saved_cmd);
>  	    }
>  	}
>      }
>  
> -  if (!ptid_equal (current_ptid, inferior_ptid))
> -    thread_has_changed = 1;
> -
> -  do_cleanups (saved_cmd_cleanup_chain);
>    do_cleanups (old_chain);
> -  /* Print stack frame only if we changed thread.  */
> -  if (thread_has_changed)
> -    print_stack_frame (get_current_frame (), 1, SRC_LINE);
>  }
>  
>  /* Switch to the specified thread.  Will dispatch off to thread_apply_command

You've moved creation of the cleanup into the loop, but not moved the
call to do_cleanups.  Do we ever need more than one cleanup for a
thread apply command?

> Index: src/gdb/infrun.c
> ===================================================================
> --- src.orig/gdb/infrun.c	2008-07-01 23:03:37.000000000 +0100
> +++ src/gdb/infrun.c	2008-07-01 23:50:48.000000000 +0100
> @@ -48,6 +48,7 @@
>  
>  #include "gdb_assert.h"
>  #include "mi/mi-common.h"
> +#include "event-top.h"
>  
>  /* Prototypes for local functions */
>  
> @@ -1526,11 +1527,20 @@ fetch_inferior_event (void *client_data)
>  {
>    struct execution_control_state ecss;
>    struct execution_control_state *ecs = &ecss;
> +  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
> +  int was_sync = sync_execution;
>  
>    memset (ecs, 0, sizeof (*ecs));
>  
>    overlay_cache_invalid = 1;
>  
> +  if (non_stop)
> +    /* In non-stop mode, the user/frontend should not notice a thread
> +       switch due to internal events.  Make sure we reverse to the
> +       user selected thread and frame after handling the event and
> +       running any breakpoint commands.  */
> +    make_cleanup_restore_current_thread ();
> +
>    /* We have to invalidate the registers BEFORE calling target_wait
>       because they can be loaded from the target while in target_wait.
>       This makes remote debugging a bit more efficient for those
> @@ -1567,6 +1577,14 @@ fetch_inferior_event (void *client_data)
>        else
>  	inferior_event_handler (INF_EXEC_COMPLETE, NULL);
>      }
> +
> +  /* Revert thread and frame.  */
> +  do_cleanups (old_chain);
> +
> +  /* If the inferior was in sync execution mode, and now isn't,
> +     restore the prompt.  */
> +  if (was_sync && !sync_execution)
> +    display_gdb_prompt (0);
>  }
>  
>  /* Prepare an execution control state for looping through a
> @@ -3705,6 +3723,11 @@ normal_stop (void)
>  
>    get_last_target_status (&last_ptid, &last);
>  
> +  /* In non-stop mode, we don't want GDB to switch threads on the
> +     user's back, to avoid races where the user is typing a command to
> +     apply to thread x, but GDB switches to thread y before the user
> +     finishes entering the command.  */

behind the user's back

Otherwise, OK.

-- 
Daniel Jacobowitz
CodeSourcery