[RFC] thread apply commands change selected frame

Denis PILAT
2007-01-11T11:24:52Z

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

Hi everyone,

I noticed a potential problem in thread.c: the "thread apply" commands
change the selected frame, I think it should not.
If you are doing
(gdb) up
(gdb) thread apply all print 1
(gdb) down
then the frame selected before the up is not being restored by the down,
we have an error message instead.

In the following patch:
1- I've added the restoring of the the selected frame in the restoring
of the current thread. All is done in the
make_cleanup_restore_current_thread() and
do_restore_current_thread_cleanup() functions.
I don't know if there could be some cases where we don't want to restore
the selected frame when having changed the current thread ? If so may be
we can separate both restoring.
I'm also wondering about getting the selected frame in
make_cleanup_restore_current_thread() directly without passing it as an
argument. You're comments are welcome.

2- I removed the print_stack_frame from the restore_current_thread()
function, I think it's up to the caller to decide if he need to print or
not the stack frame?
About that I think I must print the selected frame, not the current.
What's your opinion ?

3- I saw in info_threads_command() that the selected frame was restored
because the usage of switch_to_thread() changed it, so I used in
info_threads_command() the same principle as above, using cleanup
function. I also removed show_stack_frame() since it does nothing.

I need your opinion before going ahead in this patch.
--
Denis

[-- Attachment #2: thread.c.patch --]
[-- Type: text/plain, Size: 5708 bytes --]

Index: thread.c
===================================================================
--- thread.c (revision 549)
+++ 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 set by the user 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 abale to find the backup frame, print the
+ 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,13 @@ 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;

if (cmd == NULL || *cmd == '\000')
error (_("Please specify a command following the thread ID list"));
-
- old_chain = make_cleanup_restore_current_thread (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 */
@@ -542,6 +560,7 @@ thread_apply_all_command (char *cmd, int

do_cleanups (saved_cmd_cleanup_chain);
do_cleanups (old_chain);
+ print_stack_frame (get_current_frame (), 1, SRC_LINE);
}

static void
@@ -552,6 +571,7 @@ 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;

if (tidlist == NULL || *tidlist == '\000')
error (_("Please specify a thread ID list"));
@@ -561,7 +581,8 @@ 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);
+ 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 */
@@ -615,6 +636,7 @@ thread_apply_command (char *tidlist, int

do_cleanups (saved_cmd_cleanup_chain);
do_cleanups (old_chain);
+ print_stack_frame (get_current_frame (), 1, SRC_LINE);
}

/* Switch to the specified thread. Will dispatch off to thread_apply_command