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

Daniel Jacobowitz
2008-07-07T18:59:58Z

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