From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31834 invoked by alias); 7 Jul 2008 18:59:58 -0000 Received: (qmail 31666 invoked by uid 22791); 7 Jul 2008 18:59:53 -0000 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 07 Jul 2008 18:59:33 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id 33B4C98415; Mon, 7 Jul 2008 18:59:31 +0000 (GMT) Received: from caradoc.them.org (22.svnf5.xdsl.nauticom.net [209.195.183.55]) by nan.false.org (Postfix) with ESMTP id 052369805C; Mon, 7 Jul 2008 18:59:31 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.69) (envelope-from ) id 1KFvvu-000309-5v; Mon, 07 Jul 2008 14:59:30 -0400 Date: Mon, 07 Jul 2008 18:59:00 -0000 From: Daniel Jacobowitz To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [non-stop] 10/10 handle exited threads [was: Re: [non-stop] 10/10 split user/internal threads] Message-ID: <20080707185930.GF1778@caradoc.them.org> Mail-Followup-To: Pedro Alves , gdb-patches@sourceware.org References: <200806152209.45049.pedro@codesourcery.com> <200807011437.45222.pedro@codesourcery.com> <20080701140311.GA30550@caradoc.them.org> <200807020439.07288.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200807020439.07288.pedro@codesourcery.com> User-Agent: Mutt/1.5.17 (2008-05-11) 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: 2008-07/txt/msg00087.txt.bz2 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