From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2541 invoked by alias); 31 Jan 2007 08:58:39 -0000 Received: (qmail 2527 invoked by uid 22791); 31 Jan 2007 08:58:37 -0000 X-Spam-Check-By: sourceware.org Received: from vir-del-02.spheriq.net (HELO vir-del-02.spheriq.net) (194.50.41.41) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 31 Jan 2007 08:58:28 +0000 Received: from vir-out-01.spheriq.net ([194.50.41.30]) by vir-del-02.spheriq.net with ESMTP id l0V8wHmL032641 for ; Wed, 31 Jan 2007 08:58:17 GMT Received: from vir-cus-02.spheriq.net (vir-cus-02.spheriq.net [194.50.41.86]) by vir-out-01.spheriq.net with ESMTP id l0V8wGFR006584 for ; Wed, 31 Jan 2007 08:58:17 GMT Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by vir-cus-02.spheriq.net with ESMTP id l0V8wCkH029906 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 31 Jan 2007 08:58:15 GMT Received: from zeta.dmz-eu.st.com (ns2.st.com [164.129.230.9]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 5AEF6DA48; Wed, 31 Jan 2007 08:58:04 +0000 (GMT) Received: from mail1.cro.st.com (mail1.cro.st.com [164.129.40.131]) by zeta.dmz-eu.st.com (STMicroelectronics) with ESMTP id DA67E472C4; Wed, 31 Jan 2007 08:58:03 +0000 (GMT) Received: from [164.129.44.95] (crx595.cro.st.com [164.129.44.95]) by mail1.cro.st.com (MOS 3.7.5a-GA) with ESMTP id CJP59729 (AUTH "denis pilat"); Wed, 31 Jan 2007 09:58:02 +0100 (CET) Message-ID: <45C05A1A.6010109@st.com> Date: Wed, 31 Jan 2007 08:58:00 -0000 From: Denis PILAT User-Agent: Thunderbird 1.5.0.9 (X11/20061206) MIME-Version: 1.0 To: Daniel Jacobowitz , gdb-patches Subject: Re: [RFC] thread apply commands change selected frame References: <45A61E73.8030400@st.com> <20070121173341.GG12463@nevyn.them.org> <45B73813.9060005@st.com> In-Reply-To: <45B73813.9060005@st.com> Content-Type: multipart/mixed; boundary="------------010702040103050907000800" 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: 2007-01/txt/msg00601.txt.bz2 This is a multi-part message in MIME format. --------------010702040103050907000800 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 727 >> >> It looks right to me. With a ChangeLog entry and testsuite results >> from some target with threads, it would be OK to commit. >> > I propose a new implementation that does not change the old behavior of gdb with respect to the printing of the last frame in case we changed of thread during "thread apply" command execution. This implementation does not make any regression in the testsuite for a i386-linux target. To me the part that prints the stack frame at the end of execution of "thread apply" could be entirely removed but this will imply some changes in the testsuite. I can do that it's up to you. About the bug initially fixed by this patch, I include some new tests in threadapply.exp. -- Denis --------------010702040103050907000800 Content-Type: text/plain; name="thread.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="thread.patch" Content-length: 6907 2007-01-30 Denis Pilat * thread.c (make_cleanup_restore_current_thread): New function. (info_threads_command): Use of make_cleanup_restore_current_thread to restore the current thread and the selected frame. (restore_selected_frame): New function. (struct current_thread_cleanup): Add frame_id field. (do_restore_current_thread_cleanup): Add restoring of the selected frame. (make_cleanup_restore_current_thread): Likewise. (thread_apply_all_command): backup the selected frame while entering the function and restore it at exit. (thread_apply_command): Likewise. Index: thread.c =================================================================== --- thread.c (revision 553) +++ thread.c (working copy) @@ -65,6 +65,8 @@ static void thread_apply_command (char * static void restore_current_thread (ptid_t); static void switch_to_thread (ptid_t ptid); static void prune_threads (void); +static struct cleanup *make_cleanup_restore_current_thread (ptid_t, + struct frame_id); void delete_step_resume_breakpoint (void *arg) @@ -408,9 +410,14 @@ info_threads_command (char *arg, int fro struct thread_info *tp; ptid_t current_ptid; struct frame_info *cur_frame; - struct frame_id saved_frame_id = get_frame_id (get_selected_frame (NULL)); + struct cleanup *old_chain; + struct frame_id saved_frame_id; char *extra_info; + /* Backup current thread and selected frame. */ + saved_frame_id = get_frame_id (get_selected_frame (NULL)); + old_chain = make_cleanup_restore_current_thread (inferior_ptid, saved_frame_id); + prune_threads (); target_find_new_threads (); current_ptid = inferior_ptid; @@ -427,30 +434,22 @@ info_threads_command (char *arg, int fro if (extra_info) printf_filtered (" (%s)", extra_info); puts_filtered (" "); - + /* That switch put us at the top of the stack (leaf frame). */ switch_to_thread (tp->ptid); print_stack_frame (get_selected_frame (NULL), 0, LOCATION); } - switch_to_thread (current_ptid); + /* Restores the current thread and the frame selected before + the "info threads" command. */ + do_cleanups (old_chain); - /* Restores the frame set by the user before the "info threads" - command. We have finished the info-threads display by switching - back to the current thread. That switch has put us at the top of - the stack (leaf frame). */ - cur_frame = frame_find_by_id (saved_frame_id); - if (cur_frame == NULL) + /* If case we were not able to find the original frame, print the + new selected frame. */ + if (frame_find_by_id (saved_frame_id) == NULL) { - /* Ooops, can't restore, tell user where we are. */ warning (_("Couldn't restore frame in current thread, at frame 0")); print_stack_frame (get_selected_frame (NULL), 0, LOCATION); } - else - { - select_frame (cur_frame); - /* re-show current frame. */ - show_stack_frame (cur_frame); - } } /* Switch from one thread to another. */ @@ -474,13 +473,27 @@ restore_current_thread (ptid_t ptid) if (!ptid_equal (ptid, inferior_ptid)) { switch_to_thread (ptid); - print_stack_frame (get_current_frame (), 1, SRC_LINE); + } +} + +static void +restore_selected_frame (struct frame_id a_frame_id) +{ + struct frame_info *selected_frame_info = NULL; + + if (frame_id_eq (a_frame_id, null_frame_id)) + return; + + if ((selected_frame_info = frame_find_by_id (a_frame_id)) != NULL) + { + select_frame (selected_frame_info); } } struct current_thread_cleanup { ptid_t inferior_ptid; + struct frame_id selected_frame_id; }; static void @@ -488,15 +501,18 @@ do_restore_current_thread_cleanup (void { struct current_thread_cleanup *old = arg; restore_current_thread (old->inferior_ptid); + restore_selected_frame (old->selected_frame_id); xfree (old); } static struct cleanup * -make_cleanup_restore_current_thread (ptid_t inferior_ptid) +make_cleanup_restore_current_thread (ptid_t inferior_ptid, + struct frame_id a_frame_id) { struct current_thread_cleanup *old = xmalloc (sizeof (struct current_thread_cleanup)); old->inferior_ptid = inferior_ptid; + old->selected_frame_id = a_frame_id; return make_cleanup (do_restore_current_thread_cleanup, old); } @@ -516,11 +532,16 @@ thread_apply_all_command (char *cmd, int struct cleanup *old_chain; struct cleanup *saved_cmd_cleanup_chain; char *saved_cmd; + struct frame_id saved_frame_id; + ptid_t current_ptid; + int thread_has_changed = 0; if (cmd == NULL || *cmd == '\000') error (_("Please specify a command following the thread ID list")); - - old_chain = make_cleanup_restore_current_thread (inferior_ptid); + + current_ptid = inferior_ptid; + saved_frame_id = get_frame_id (get_selected_frame (NULL)); + old_chain = make_cleanup_restore_current_thread (inferior_ptid, saved_frame_id); /* It is safe to update the thread list now, before traversing it for "thread apply all". MVS */ @@ -540,8 +561,15 @@ thread_apply_all_command (char *cmd, int strcpy (cmd, saved_cmd); /* Restore exact command used previously */ } + 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); + } static void @@ -552,6 +580,9 @@ thread_apply_command (char *tidlist, int struct cleanup *old_chain; struct cleanup *saved_cmd_cleanup_chain; char *saved_cmd; + struct frame_id saved_frame_id; + ptid_t current_ptid; + int thread_has_changed = 0; if (tidlist == NULL || *tidlist == '\000') error (_("Please specify a thread ID list")); @@ -561,7 +592,9 @@ thread_apply_command (char *tidlist, int if (*cmd == '\000') error (_("Please specify a command following the thread ID list")); - old_chain = make_cleanup_restore_current_thread (inferior_ptid); + current_ptid = inferior_ptid; + saved_frame_id = get_frame_id (get_selected_frame (NULL)); + 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 */ @@ -613,8 +646,14 @@ thread_apply_command (char *tidlist, int } } + 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 --------------010702040103050907000800 Content-Type: text/plain; name="threadapply.exp.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="threadapply.exp.patch" Content-length: 947 2007-01-30 Denis Pilat * gdb.threads/threadapply.exp: check that frame is not changed by the thread apply all command. Index: testsuite/gdb.threads/threadapply.exp =================================================================== --- testsuite/gdb.threads/threadapply.exp (revision 553) +++ testsuite/gdb.threads/threadapply.exp (working copy) @@ -69,3 +69,10 @@ gdb_test_multiple "define backthread" "d gdb_test "set backtrace limit 3" "" gdb_test "thread apply all backthread" "Thread ..*\\\$1 = 0x14.*Thread ..*\\\$2 = 0x14.*Thread ..*\\\$3 = 0x14.*Thread ..*\\\$4 = 0x14.*Thread ..*\\\$5 = 0x14.*Thread ..*\\\$. = 0x14" +# Go into the thread_function to check that a simple "thread apply" +# does not change the selected frame. +gdb_test "step" "thread_function.*" +gdb_test "up" +gdb_test "thread apply all print 1" +gdb_test "down" "#0.*thread_function.*" "Thread apply must not change the selected frame" + --------------010702040103050907000800--