Re: [RFC] thread apply commands change selected frame

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

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


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



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

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

[-- Attachment #3: threadapply.exp.patch --]
[-- Type: text/plain, Size: 947 bytes --]

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"
+