[RFC] thread apply commands change selected frame

[Denis PILAT <denis.pilat@st.com>]

[-- 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