This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] thread apply commands change selected frame
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
2007-01-30 Denis Pilat <denis.pilat@st.com>
* 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
2007-01-30 Denis Pilat <denis.pilat@st.com>
* 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"
+