Enhancement - show old and new thread info when switching during debugging

Enhancement - show old and new thread info when switching during debugging

[pfee]

Hello all,

The following patch is also attached to bug 13042.
http://sourceware.org/bugzilla/show_bug.cgi?id=13042

It gives the user info about the thread they're switching from in addition to 
that which they're switching to.  This is useful if the switch was not intended 
and 

they wish to continue debugging the old thread.

Thanks,
Paul

2011-07-29  Paul Fee  <pfee@talk21.com>

    * infrun.c (normal_stop): Output information about previous thread when 
switching

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.497
diff -u -r1.497 infrun.c
--- infrun.c    21 Jul 2011 23:46:09 -0000      1.497
+++ infrun.c    29 Jul 2011 13:54:44 -0000
@@ -5770,9 +5770,12 @@
       && last.kind != TARGET_WAITKIND_SIGNALLED
       && last.kind != TARGET_WAITKIND_EXITED)
     {
+      const char* previous_pid_str = strdup (target_pid_to_str 
(previous_inferior_ptid));
       target_terminal_ours_for_output ();
-      printf_filtered (_("[Switching to %s]\n"),
-                      target_pid_to_str (inferior_ptid));
+      printf_filtered (_("[Switching from %s to %s]\n"),
+                       previous_pid_str,
+                       target_pid_to_str (inferior_ptid));
+      free ((void*)previous_pid_str);
       annotate_thread_changed ();
       previous_inferior_ptid = inferior_ptid;
     }

Re: Enhancement - show old and new thread info when switching during debugging

[Joel Brobecker]

> The following patch is also attached to bug 13042.
> http://sourceware.org/bugzilla/show_bug.cgi?id=13042
> 
> It gives the user info about the thread they're switching from in
> addition to that which they're switching to.  This is useful if the
> switch was not intended and they wish to continue debugging the old
> thread.

Isn't that information already present in the previous "[Switching to
thread ...]" message?

-- 
Joel

Re: Enhancement - show old and new thread info when switching during debugging

[pfee]

> > The following patch is also attached to bug 13042.
> > http://sourceware.org/bugzilla/show_bug.cgi?id=13042
> > 
> > It  gives the user info about the thread they're switching from in
> > addition  to that which they're switching to.  This is useful if the
> > switch  was not intended and they wish to continue debugging the old
> >  thread.
> 
> Isn't that information already present in the previous  "[Switching to
> thread ...]" message?

I've copied output from GDB 7.3 before and after my patch into the bug report 
along with some notes on the implementation.

Without the patch the user is told what thread they're switching to, but not 
what thread they're switching from, e.g.


[Switching to Thread 0x7ffff7fd6700 (LWP 4267)]
In a program with numerous threads, the old thread may be the one the user is 
interested it.  They may not have wanted to switch, but by chance the new thread 
has hit a breakpoint.  By giving information about what thread we're switching 
away from, it becomes easier for the user to switch back to that thread and 
ignore the accidental thread switch.

Example of new output:

[Switching from Thread 0x7ffff7fd8740 (LWP 4279) to Thread 0x7ffff7fd6700 (LWP 
4282)]Thanks,
Paul

Re: Enhancement - show old and new thread info when switching during debugging

[Joel Brobecker]

> I've copied output from GDB 7.3 before and after my patch into the bug
> report along with some notes on the implementation.

I think I understand what you are trying to say.  What I am saying is
that you can find which thread you switched from, by going up the
debugger output until you find a "[Switching to Thread ...]" message.
So, the information is already there. I agree it's not ultra convenient,
but I personally think it's better than your approach (the new message
becomes way to long, IMO).

In your case, if going up the output is not good enough, I would
suggest two alternative solutions, in order of preference:
  - Have GDB maintain variable $old_thread or $prev_thread that
    you can then use to switch to the previous thread;

      (gdb) thread $old_thread

    would return you to the previous thread before the switch.
  - At least print the new thread ID first;

-- 
Joel

Re: Enhancement - show old and new thread info when switching during debugging

[pfee]

> > I've copied output from GDB 7.3 before and after my patch into the  bug

> > report along with some notes on the implementation.
> 
> I think I  understand what you are trying to say.  What I am saying is
> that you can  find which thread you switched from, by going up the
> debugger output until  you find a "[Switching to Thread ...]" message.
> So, the information is  already there. I agree it's not ultra convenient,
> but I personally think it's  better than your approach (the new message
> becomes way to long,  IMO).
> 
> In your case, if going up the output is not good enough, I  would
> suggest two alternative solutions, in order of preference:
>   -  Have GDB maintain variable $old_thread or $prev_thread that
>     you  can then use to switch to the previous thread;
> 
>       (gdb)  thread $old_thread
> 
>     would return you to the previous thread  before the switch.
>   - At least print the new thread ID first;

Ah, I see your point now also.

The previous "Switching to" message may be lost if the terminal output buffer 
isn't long enough.

I like your idea of being able to use a "thread $prev_thread" command.  That's 
better than my patch as I have printed out the hex target thread ID and decimal 
LWP numbers but not the GDB thread ID. It's the GDB thread ID that used with the 
"thread" command.  Hence even with my patch, the user would still have to issue 
"info threads", then match the LWP (or target ID) against GDB's ID.

However I don't currently know how to interact with GDB convenience variables.  
You second suggestion is to reorder the output e.g.

[Switching to Thread 0x7ffff7fd67000 (LWP 4282) from Thread 0x7ffff7fd8740 (LWP 
4279)]

That would be simple to do.  Would you recommend adjusting the printf statement 
or learning how to create a new convenience variable?

Thanks,
Paul

Re: Enhancement - show old and new thread info when switching during debugging

[Philippe Waroquiers]

On Fri, 2011-07-29 at 17:33 +0100, pfee@talk21.com wrote:
> [Switching to Thread 0x7ffff7fd67000 (LWP 4282) from Thread 0x7ffff7fd8740 (LWP 
> 4279)]
An alternative is to add a hook:

(gdb) define hook-thread
Type commands for definition of "hook-thread".
End with a line saying just "end".
>thread
>end
(gdb) info thre
  2 Thread 0xb7fe3b70 (LWP 2321)  0x0070c416 in __kernel_vsyscall ()
* 1 Thread 0xb7fe46c0 (LWP 2318)  main (argc=1, argv=0xbffff244)
    at sleepers.c:169
(gdb) thre 2
[Current thread is 1 (Thread 0xb7fe46c0 (LWP 2318))]
[Switching to thread 2 (Thread 0xb7fe3b70 (LWP 2321))]#0  0x0070c416 in __kernel_vsyscall ()
(gdb) 

It would have been nice to do
  if $argc >= 1
    thread
  end

so that the thread command without arg would not output twice the
current thread.

It is not clear to me why the command hook does not get the command
arguments.

execute_cmd_pre_hook calls   
   execute_user_command (c->hook_pre, (char *) 0);

If execute_command 
   calls first
      execute_cmd_pre_hook (c); 
then
      execute_user_command (c, arg);

So, I suppose that by adding one argument to execute_cmd_pre_hook, 
the hook could pass arg to execute_user_command.
(same for the post hook).

If that looks a good idea, it might become my 3rd gdb patch :).

Philippe

Re: Enhancement - show old and new thread info when switching during debugging

[Tom Tromey]

Joel> Isn't that information already present in the previous "[Switching to
Joel> thread ...]" message?

We can always add an option, like "set print thread previous on" to
control the output.

Tom

Re: Enhancement - show old and new thread info when switching during debugging

[Joel Brobecker]

> Joel> Isn't that information already present in the previous "[Switching to
> Joel> thread ...]" message?
> 
> We can always add an option, like "set print thread previous on" to
> control the output.

Generally speaking, I prefer to avoid introducing options unless
really necessary. In this particular case, it sounds like the most
convenient way of solving the problem is to have a convenience
variable. And it sounds like the only reason why Paul hasn't looked
at the implementation yet is due to lack of familiarity with this
code?

    % grep '"bpnum"' *.c
    breakpoint.c:  set_internalvar_integer (lookup_internalvar ("bpnum"), num);

So, I would think that adding something similar in
thread.c:switch_to_thread would do the job.

-- 
Joel

Re: Enhancement - show old and new thread info when switching during debugging

[Pedro Alves]

On Saturday 30 July 2011 03:44:05, Joel Brobecker wrote:
> So, I would think that adding something similar in
> thread.c:switch_to_thread would do the job.

It wouldn't.  What he wants is the last user selected thread,
not the thread that happens to be current when switch_to_thread
is called.

- user resumes with thread 1 selected
- thread 2 hits breakpoint, gdb switches to thread 2
  - breakpoint doesn't cause stop, target is re-resumed
- thread 3 hits breakpoint, gdb switches to thread 3
  - breakpoint causes stop, previous_inferior_ptid is
    thread 1, but the last switch_to_thread would have
    recorded $previous_thread as thread 2

If you want a convenience variable, please model it on
$_thread --- see end of thread.c:_initialize_thread.

Note that the previously selected thread may not exist anymore,
it may have exited meanwhile, for example, or the inferior
exec'ed.

-- 
Pedro Alves

Re: Enhancement - show old and new thread info when switching during debugging

[pfee]

> > So, I would  think that adding something similar in

> > thread.c:switch_to_thread would  do the job.
> 
> It wouldn't.  What he wants is the last user selected  thread,
> not the thread that happens to be current when switch_to_thread
> is  called.
> 
> - user resumes with thread 1 selected
> - thread 2 hits  breakpoint, gdb switches to thread 2
>   - breakpoint doesn't cause stop,  target is re-resumed
> - thread 3 hits breakpoint, gdb switches to thread  3
>   - breakpoint causes stop, previous_inferior_ptid is
>      thread 1, but the last switch_to_thread would have
>     recorded  $previous_thread as thread 2
> 
> If you want a convenience variable, please  model it on
> $_thread --- see end of thread.c:_initialize_thread.
> 
> Note  that the previously selected thread may not exist anymore,
> it may have exited  meanwhile, for example, or the inferior
> exec'ed.
> 
> -- 
> Pedro  Alves
>

Hi Pedro,

I agree with your example, the previous_inferior_ptid in infrun.c, normal_stop() 
has the information I need.  I'll try and use that to populate the new 
convenience variable.

I'll a try to construct a test program that creates a scenario where the 
previous thread as died by the time normal_stop() executes.

Thanks,
Paul

Re: Enhancement - show old and new thread info when switching during debugging

[pfee]

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

> > What he wants is the last user selected  thread,

> >  not the thread that happens to be current when switch_to_thread
> > is   called.
> > 
> > - user resumes with thread 1 selected
> > - thread  2 hits  breakpoint, gdb switches to thread 2
> >   - breakpoint  doesn't cause stop,  target is re-resumed
> > - thread 3 hits  breakpoint, gdb switches to thread  3
> >   - breakpoint causes  stop, previous_inferior_ptid is
> >      thread 1, but the  last switch_to_thread would have
> >     recorded   $previous_thread as thread 2
> > 
> > If you want a convenience  variable, please  model it on
> > $_thread --- see end of  thread.c:_initialize_thread.
> > 
> > Note  that the previously  selected thread may not exist anymore,
> > it may have exited   meanwhile, for example, or the inferior
> > exec'ed.
> > 
> > -- 
> > Pedro  Alves
> >
> 
> Hi Pedro,
> 
> I agree with your  example, the previous_inferior_ptid in infrun.c, 
>normal_stop() 
>
> has the  information I need.  I'll try and use that to populate the new 
> convenience variable.
> 
> I'll a try to construct a test program that  creates a scenario where the 
> previous thread as died by the time  normal_stop() executes.
> 

Hello again,

I've looked into convenience variables and have developed a new patch.  The gdb 
output when switching threads is no longer altered, instead the $_prev_thread 
convenience variable can be used to switch back to the previous thread.

When making this patch I renamed an existing variable from 
previous_inferior_ptid to current_inferior_ptid.  That way we have values that 
move from inferior_ptid (the new ptid) to current_inferior_ptid and finally to  
previous_inferior_ptid (which is exposed via the convenience variable).  This 
seemed better than leaving the variables names as was and introducing something 

confusing like previous_previous_inferior_ptid.

By the way, I've made the patches against GDB trunk, I presume that's preferred 
over GDB 7.3.


Comments welcome.
Thanks,
Paul

2011-08-08  Paul Fee <pfee@talk21.com>

    * inferior.h: Add function for handling $_prev_thread convenience variable
    * infrun.c: Rename previous_inferior_ptid to current_inferior_ptid.
    Add function for handling $_prev_thread convenience variable
    * thread.c: Add $_prev_thread convenience variable

[-- Attachment #2: prev_thread.patch --]
[-- Type: application/octet-stream, Size: 5596 bytes --]

Index: inferior.h
===================================================================
RCS file: /cvs/src/src/gdb/inferior.h,v
retrieving revision 1.161
diff -c -p -r1.161 inferior.h
*** inferior.h	21 Jul 2011 23:46:08 -0000	1.161
--- inferior.h	8 Aug 2011 11:23:59 -0000
*************** extern int stop_on_solib_events;
*** 204,209 ****
--- 204,212 ----
  
  extern void start_remote (int from_tty);
  
+ extern struct value *prev_thread_id_make_value (struct gdbarch *,
+                                                 struct internalvar *);
+ 
  extern void normal_stop (void);
  
  extern int signal_stop_state (int);
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.498
diff -c -p -r1.498 infrun.c
*** infrun.c	4 Aug 2011 19:10:12 -0000	1.498
--- infrun.c	8 Aug 2011 11:24:02 -0000
*************** int sync_execution = 0;
*** 125,130 ****
--- 125,136 ----
     when the inferior stopped in a different thread than it had been
     running in.  */
  
+ static ptid_t current_inferior_ptid;
+ 
+ /* Values move from interior_ptid to current_inferior_ptid to
+  * previous_inferior_ptid.  The previous value is exposed to the
+  * user through the $_prev_thread convenience variable.
+  */
  static ptid_t previous_inferior_ptid;
  
  /* Default behavior is to detach newly forked processes (legacy).  */
*************** proceed (CORE_ADDR addr, enum target_sig
*** 2069,2075 ****
      }
  
    /* We'll update this if & when we switch to a new thread.  */
!   previous_inferior_ptid = inferior_ptid;
  
    regcache = get_current_regcache ();
    gdbarch = get_regcache_arch (regcache);
--- 2075,2081 ----
      }
  
    /* We'll update this if & when we switch to a new thread.  */
!   current_inferior_ptid = inferior_ptid;
  
    regcache = get_current_regcache ();
    gdbarch = get_regcache_arch (regcache);
*************** init_wait_for_inferior (void)
*** 2289,2295 ****
  
    target_last_wait_ptid = minus_one_ptid;
  
!   previous_inferior_ptid = inferior_ptid;
    init_infwait_state ();
  
    /* Discard any skipped inlined frames.  */
--- 2295,2301 ----
  
    target_last_wait_ptid = minus_one_ptid;
  
!   current_inferior_ptid = inferior_ptid;
    init_infwait_state ();
  
    /* Discard any skipped inlined frames.  */
*************** handle_inferior_event (struct execution_
*** 3719,3725 ****
  	  switch_to_thread (deferred_step_ptid);
  	  deferred_step_ptid = null_ptid;
  	  /* Suppress spurious "Switching to ..." message.  */
! 	  previous_inferior_ptid = inferior_ptid;
  
  	  resume (1, TARGET_SIGNAL_0);
  	  prepare_to_wait (ecs);
--- 3725,3731 ----
  	  switch_to_thread (deferred_step_ptid);
  	  deferred_step_ptid = null_ptid;
  	  /* Suppress spurious "Switching to ..." message.  */
! 	  current_inferior_ptid = inferior_ptid;
  
  	  resume (1, TARGET_SIGNAL_0);
  	  prepare_to_wait (ecs);
*************** print_no_history_reason (void)
*** 5730,5735 ****
--- 5736,5753 ----
    ui_out_text (current_uiout, "\nNo more reverse-execution history.\n");
  }
  
+ /* Return the previous thread's id.  Return a value of 0 if
+    no previous thread was selected, or it doesn't exist.  */
+ 
+ struct value *
+ prev_thread_id_make_value (struct gdbarch *gdbarch, struct internalvar *var)
+ {
+   struct thread_info *tp = find_thread_ptid (previous_inferior_ptid);
+ 
+   return value_from_longest (builtin_type (gdbarch)->builtin_int,
+                  (tp ? tp->num : 0));
+ }
+ 
  /* Here to return control to GDB when the inferior stops for real.
     Print appropriate messages, remove breakpoints, give terminal our modes.
  
*************** normal_stop (void)
*** 5770,5785 ****
       Note that SIGNALLED here means "exited with a signal", not
       "received a signal".  */
    if (!non_stop
!       && !ptid_equal (previous_inferior_ptid, inferior_ptid)
        && target_has_execution
        && last.kind != TARGET_WAITKIND_SIGNALLED
        && last.kind != TARGET_WAITKIND_EXITED)
      {
        target_terminal_ours_for_output ();
        printf_filtered (_("[Switching to %s]\n"),
! 		       target_pid_to_str (inferior_ptid));
        annotate_thread_changed ();
!       previous_inferior_ptid = inferior_ptid;
      }
  
    if (!breakpoints_always_inserted_mode () && target_has_execution)
--- 5788,5804 ----
       Note that SIGNALLED here means "exited with a signal", not
       "received a signal".  */
    if (!non_stop
!       && !ptid_equal (current_inferior_ptid, inferior_ptid)
        && target_has_execution
        && last.kind != TARGET_WAITKIND_SIGNALLED
        && last.kind != TARGET_WAITKIND_EXITED)
      {
        target_terminal_ours_for_output ();
        printf_filtered (_("[Switching to %s]\n"),
!                target_pid_to_str (inferior_ptid));
        annotate_thread_changed ();
!       previous_inferior_ptid = current_inferior_ptid;
!       current_inferior_ptid = inferior_ptid;
      }
  
    if (!breakpoints_always_inserted_mode () && target_has_execution)
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.142
diff -c -p -r1.142 thread.c
*** thread.c	4 Aug 2011 19:10:13 -0000	1.142
--- thread.c	8 Aug 2011 11:24:02 -0000
*************** Show printing of thread events (such as 
*** 1498,1501 ****
--- 1498,1502 ----
           &setprintlist, &showprintlist);
  
    create_internalvar_type_lazy ("_thread", thread_id_make_value);
+   create_internalvar_type_lazy ("_prev_thread", prev_thread_id_make_value);
  }

Re: Enhancement - show old and new thread info when switching during debugging

[Pedro Alves]

Hellow,

On Monday 08 August 2011 12:34:42, pfee@talk21.com wrote:

> I've looked into convenience variables and have developed a new patch.  The gdb 
> output when switching threads is no longer altered, instead the $_prev_thread 
> convenience variable can be used to switch back to the previous thread.
> 
> When making this patch I renamed an existing variable from 
> previous_inferior_ptid to current_inferior_ptid.  That way we have values that 
> move from inferior_ptid (the new ptid) to current_inferior_ptid and finally to  
> previous_inferior_ptid (which is exposed via the convenience variable).  This 
> seemed better than leaving the variables names as was and introducing something 
> 
> confusing like previous_previous_inferior_ptid.

Sorry, but current_inferior_ptid for this will be even more confusing.
There's several current_FOO globals that represent the currently selected
state.  And one of them is "current_inferior".  Having current_inferior() return
one thing, and current_inferior_ptid mean another thing will be a recipe
for long term confusion.

> 
> By the way, I've made the patches against GDB trunk, I presume that's preferred 
> over GDB 7.3.

Yes, thanks.  7.3 is in maintenance mode now.  New features go to trunk.

>Content-Type: application/octet-stream; name="prev_thread.patch"
>Content-Transfer-Encoding: base64
>Content-Disposition: attachment; filename="prev_thread.patch"

Any chance you can convince your mailer to attach patches
with "Content-Type: text/x-patch" or some other text mime
type?  If you're using a web email account, I think that
means telling your browser the mime type of the ".patch"
extension.  If you can get it to not use base64, and use
"content-disposition: inline", you get extra bonus points.

> + /* Values move from interior_ptid to current_inferior_ptid to
> +  * previous_inferior_ptid.  The previous value is exposed to the
> +  * user through the $_prev_thread convenience variable.
> +  */

Plese no leading * on every comment line.  See other
comments, and follow the same style.

In order for this to be accepted, it will need some
documentation in the manual.

I'm not sure the variable is sufficiently well defined yet.
What does gdb print in this case?

 (gdb) thread 2
 (gdb) thread 3
 (gdb) p $_prev_thread

It feels like it should print thread 2, that is, we'd
define it to the thread the user last had selected,
and so:

 (gdb) thread $_prev_thread
 (gdb) thread $_prev_thread

would cycle between thread 2 and 3.

This definition works for non-stop mode as well.
Otherwise, as is, we get to define it as "the thread that
the user had selected the last time an execution command
was ran" (and the $_prev_thread is undefined/meaningless in
non-stop mode).

-- 
Pedro Alves

Re: Enhancement - show old and new thread info when switching during debugging

[pfee]

> > I've looked  into convenience variables and have developed a new patch.  The 
>gdb 
>

> > output when switching threads is no longer altered, instead the  
>$_prev_thread 
>
> > convenience variable can be used to switch back to the  previous thread.
> > 
> > When making this patch I renamed an existing  variable from 
> > previous_inferior_ptid to current_inferior_ptid.   That way we have values 
>that 
>
> > move from inferior_ptid (the new ptid) to  current_inferior_ptid and finally 
>to  
>
> > previous_inferior_ptid  (which is exposed via the convenience variable).  
>This 
>
> > seemed  better than leaving the variables names as was and introducing 
>something 
>
> > 
> > confusing like previous_previous_inferior_ptid.
> 
> Sorry,  but current_inferior_ptid for this will be even more confusing.
> There's  several current_FOO globals that represent the currently  selected
> state.  And one of them is "current_inferior".  Having  current_inferior() 
>return
> one thing, and current_inferior_ptid mean another  thing will be a recipe
> for long term confusion.

No problem, I'll pick another name.

> 
> > 
> > By the  way, I've made the patches against GDB trunk, I presume that's 
>preferred 
>
> > over GDB 7.3.
> 
> Yes, thanks.  7.3 is in maintenance mode  now.  New features go to trunk.

That's what I expected, thanks.

> 
> >Content-Type:  application/octet-stream;  name="prev_thread.patch"
> >Content-Transfer-Encoding:  base64
> >Content-Disposition: attachment;  filename="prev_thread.patch"
> 
> Any chance you can convince your mailer to  attach patches
> with "Content-Type: text/x-patch" or some other text  mime
> type?  If you're using a web email account, I think that
> means  telling your browser the mime type of the ".patch"
> extension.  If you  can get it to not use base64, and use
> "content-disposition: inline", you get  extra bonus points.

I'm using Yahoo webmail, it's handling of attachments could be better.  I'll try 
using a traditional email client.

> 
> > + /* Values move from interior_ptid to  current_inferior_ptid to
> > +  * previous_inferior_ptid.  The  previous value is exposed to the
> > +  * user through the  $_prev_thread convenience variable.
> > +  */
> 
> Please no leading *  on every comment line.  See other
> comments, and follow the same  style.

Ok

> 
> In order for this to be accepted, it will need  some
> documentation in the manual.
> 
> I'm not sure the variable is  sufficiently well defined yet.
> What does gdb print in this case?
> 
>   (gdb) thread 2
>  (gdb) thread 3
>  (gdb) p $_prev_thread
> 
> It feels like  it should print thread 2, that is, we'd
> define it to the thread the user last  had selected,
> and so:
> 
>  (gdb) thread $_prev_thread
>  (gdb) thread  $_prev_thread
> 
> would cycle between thread 2 and 3.
> 
> This definition  works for non-stop mode as well.
> Otherwise, as is, we get to define it as  "the thread that
> the user had selected the last time an execution  command
> was ran" (and the $_prev_thread is undefined/meaningless  in
> non-stop mode).

I appreciate the feedback, your summary of the current operation is correct.  
I'll take a look at making $_prev_thread be influenced by the user entering 
"thread" commands.  Since that should then give us consistent operation in both 
all-stop and non-stop modes, the additional change will be worthwhile.

Thanks,
Paul

Re: Enhancement - show old and new thread info when switching during debugging

[pfee]

>> Sorry,  but current_inferior_ptid for this will be even more confusing.
>> There's several current_FOO globals that represent the currently selected
>> state.  And one of them is "current_inferior".  Having current_inferior()
>> return one thing, and current_inferior_ptid mean another thing will be a 
>> recipe for long term confusion.
> 
> No problem, I'll pick another name.

I've reverted current_inferior_ptid back to previous_inferior_ptid, which 
makes the patch smaller.

The new variable is now previous_selected_ptid.

>> Any chance you can convince your mailer to  attach patches
>> with "Content-Type: text/x-patch" or some other text  mime
>> type?
> 
> I'm using Yahoo webmail, it's handling of attachments could be better. 
> I'll try using a traditional email client.

I tried kmail which allows setting the content-type and content-disposition, but 
the email didn't reach the gdb-patches list.  I'm reverting to using webmail and 
I'll put the patch as plain text below.

>> Please no leading *  on every comment line.  See other
>> comments, and follow the same  style.
> 
> Ok

Done

> 
>> 
>> In order for this to be accepted, it will need  some
>> documentation in the manual.

Let's consider documentation once the patch is acceptable.

>> 
>> I'm not sure the variable is  sufficiently well defined yet.
>> What does gdb print in this case?
>> 
>>   (gdb) thread 2
>>  (gdb) thread 3
>>  (gdb) p $_prev_thread
>> 
>> It feels like  it should print thread 2, that is, we'd
>> define it to the thread the user last  had selected,
>> and so:
>> 
>>  (gdb) thread $_prev_thread
>>  (gdb) thread  $_prev_thread
>> 
>> would cycle between thread 2 and 3.

I now also set previous_selected_ptid in do_captured_thread_select(), which 
results in the behaviour you've asked for.

>> 
>> This definition  works for non-stop mode as well.
>> Otherwise, as is, we get to define it as  "the thread that
>> the user had selected the last time an execution  command
>> was ran" (and the $_prev_thread is undefined/meaningless  in
>> non-stop mode).
> 
> I appreciate the feedback, your summary of the current operation is
> correct. I'll take a look at making $_prev_thread be influenced by the
> user entering
> "thread" commands.  Since that should then give us consistent operation in
> both all-stop and non-stop modes, the additional change will be
> worthwhile.

I tested the patch in non-stop mode.  $_prev_thread only gets modified when 
the user switches using the "thread" command.  I think that's what you're 
asking for also.

Thanks,
Paul

2011-08-09  Paul Fee <pfee@talk21.com>

    * inferior.h: Add $_prev_thread convenience variable.
    * infrun.c: Add $_prev_thread convenience variable, set it when current 
thread changes.
    * thread.c: Add $_prev_thread convenience variable, set it when user 
switches threads.

Index: inferior.h
===================================================================
RCS file: /cvs/src/src/gdb/inferior.h,v
retrieving revision 1.161
diff -c -p -r1.161 inferior.h
*** inferior.h    21 Jul 2011 23:46:08 -0000    1.161
--- inferior.h    9 Aug 2011 14:44:23 -0000
*************** extern const char *get_inferior_io_termi
*** 94,99 ****
--- 94,105 ----
  
  extern ptid_t inferior_ptid;
  
+ /* Values move from interior_ptid to previous_inferior_ptid to
+  * previous_selected_ptid.  The previous value is exposed to the
+  * user through the $_prev_thread convenience variable.
+  */
+ extern ptid_t previous_selected_ptid;
+ 
  /* Are we simulating synchronous execution? This is used in async gdb
     to implement the 'run', 'continue' etc commands, which will not
     redisplay the prompt until the execution is actually over.  */
*************** extern int stop_on_solib_events;
*** 204,209 ****
--- 210,218 ----
  
  extern void start_remote (int from_tty);
  
+ extern struct value *prev_thread_id_make_value (struct gdbarch *,
+                                                 struct internalvar *);
+ 
  extern void normal_stop (void);
  
  extern int signal_stop_state (int);
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.498
diff -c -p -r1.498 infrun.c
*** infrun.c    4 Aug 2011 19:10:12 -0000    1.498
--- infrun.c    9 Aug 2011 14:44:29 -0000
*************** int sync_execution = 0;
*** 127,132 ****
--- 127,138 ----
  
  static ptid_t previous_inferior_ptid;
  
+ /* Values move from interior_ptid to previous_inferior_ptid to
+    previous_selected_ptid.  The previous selected value is exposed
+    to the user through the $_prev_thread convenience variable.  */
+ 
+ ptid_t previous_selected_ptid;
+ 
  /* Default behavior is to detach newly forked processes (legacy).  */
  int detach_fork = 1;
  
*************** print_no_history_reason (void)
*** 5730,5735 ****
--- 5736,5753 ----
    ui_out_text (current_uiout, "\nNo more reverse-execution history.\n");
  }
  
+ /* Return the previous thread's id.  Return a value of 0 if
+    no previous thread was selected, or it doesn't exist.  */
+ 
+ struct value *
+ prev_thread_id_make_value (struct gdbarch *gdbarch, struct internalvar *var)
+ {
+   struct thread_info *tp = find_thread_ptid (previous_selected_ptid);
+ 
+   return value_from_longest (builtin_type (gdbarch)->builtin_int,
+                  (tp ? tp->num : 0));
+ }
+ 
  /* Here to return control to GDB when the inferior stops for real.
     Print appropriate messages, remove breakpoints, give terminal our modes.
  
*************** normal_stop (void)
*** 5777,5784 ****
      {
        target_terminal_ours_for_output ();
        printf_filtered (_("[Switching to %s]\n"),
!                target_pid_to_str (inferior_ptid));
        annotate_thread_changed ();
        previous_inferior_ptid = inferior_ptid;
      }
  
--- 5795,5803 ----
      {
        target_terminal_ours_for_output ();
        printf_filtered (_("[Switching to %s]\n"),
!                target_pid_to_str (inferior_ptid));
        annotate_thread_changed ();
+       previous_selected_ptid = previous_inferior_ptid;
        previous_inferior_ptid = inferior_ptid;
      }
  
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.142
diff -c -p -r1.142 thread.c
*** thread.c    4 Aug 2011 19:10:13 -0000    1.142
--- thread.c    9 Aug 2011 14:44:30 -0000
*************** do_captured_thread_select (struct ui_out
*** 1394,1399 ****
--- 1394,1400 ----
    if (!thread_alive (tp))
      error (_("Thread ID %d has terminated."), num);
  
+   previous_selected_ptid = inferior_ptid;
    switch_to_thread (tp->ptid);
  
    annotate_thread_changed ();
*************** Show printing of thread events (such as 
*** 1498,1501 ****
--- 1499,1503 ----
           &setprintlist, &showprintlist);
  
    create_internalvar_type_lazy ("_thread", thread_id_make_value);
+   create_internalvar_type_lazy ("_prev_thread", prev_thread_id_make_value);
  }

Re: Enhancement - show old and new thread info when switching during debugging

[Pedro Alves]

On Tuesday 09 August 2011 16:25:33, pfee@talk21.com wrote:

> I tried kmail which allows setting the content-type and content-disposition, but 
> the email didn't reach the gdb-patches list.  I'm reverting to using webmail and 
> I'll put the patch as plain text below.

Looks like the patch ends up line mangled that way.  :-/  The email would
have been rejected if you sent as html instead of text.  Otherwise, it's
likely something with your smtp configuration in kmail.  I use kmail
every day with no such issue.

> 2011-08-09  Paul Fee <pfee@talk21.com>

Double space between your name and the email address too.

> 
>     * inferior.h: Add $_prev_thread convenience variable.
>     * infrun.c: Add $_prev_thread convenience variable, set it when current 
> thread changes.
>     * thread.c: Add $_prev_thread convenience variable, set it when user 
> switches threads.

Line mangling/wrapping here.  Please keep lines under 80 columns.  Align
with a tab.  Please see other changelog entries as guideline (or better
the GNU coding standards chapter on Change Logs).  Something like:

	* inferior.h (prev_selected_thread): Declare.
	* infrun.c (prev_selected_thread): New global.
	(prev_thread_id_make_value): New function.
	... etc.

> 
> Index: inferior.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/inferior.h,v
> retrieving revision 1.161
> diff -c -p -r1.161 inferior.h
> *** inferior.h    21 Jul 2011 23:46:08 -0000    1.161
> --- inferior.h    9 Aug 2011 14:44:23 -0000
> *************** extern const char *get_inferior_io_termi
> *** 94,99 ****
> --- 94,105 ----
>   
>   extern ptid_t inferior_ptid;
>   
> + /* Values move from interior_ptid to previous_inferior_ptid to
> +  * previous_selected_ptid.  The previous value is exposed to the
> +  * user through the $_prev_thread convenience variable.
> +  */
> + extern ptid_t previous_selected_ptid;

Same comment formatting again.  Typo s/interior/inferior/

The comment is no longer correct.  I'd go for not explaining the
implementation, but what the variable is supposed to track:

 /* The previously user/frontend selected thread.  Exposed through the
    $_prev_thread convenience variable.  */
  extern ptid_t previous_selected_ptid;

and moving this declaration to gdbthread.h, and the definition
to thread.c.

> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.498
> diff -c -p -r1.498 infrun.c
> *** infrun.c    4 Aug 2011 19:10:12 -0000    1.498
> --- infrun.c    9 Aug 2011 14:44:29 -0000
> *************** int sync_execution = 0;
> *** 127,132 ****
> --- 127,138 ----
>   
>   static ptid_t previous_inferior_ptid;
>   
> + /* Values move from interior_ptid to previous_inferior_ptid to
> +    previous_selected_ptid.  The previous selected value is exposed
> +    to the user through the $_prev_thread convenience variable.  */

Comment duplication leads to bit rot.  Please drop this copy.

> + 
> + ptid_t previous_selected_ptid;
> + 
>   /* Default behavior is to detach newly forked processes (legacy).  */
>   int detach_fork = 1;
>   
> *************** print_no_history_reason (void)
> *** 5730,5735 ****
> --- 5736,5753 ----
>     ui_out_text (current_uiout, "\nNo more reverse-execution history.\n");
>   }
>   
> + /* Return the previous thread's id.  Return a value of 0 if
> +    no previous thread was selected, or it doesn't exist.  */
> + 
> + struct value *
> + prev_thread_id_make_value (struct gdbarch *gdbarch, struct internalvar *var)
> + {
> +   struct thread_info *tp = find_thread_ptid (previous_selected_ptid);
> + 
> +   return value_from_longest (builtin_type (gdbarch)->builtin_int,
> +                  (tp ? tp->num : 0));
> + }

Please move this to thread.c, right next to thread_id_make_value,
and make it static.

> + 
>   /* Here to return control to GDB when the inferior stops for real.
>      Print appropriate messages, remove breakpoints, give terminal our modes.
>   
> *************** normal_stop (void)
> *** 5777,5784 ****
>       {
>         target_terminal_ours_for_output ();
>         printf_filtered (_("[Switching to %s]\n"),
> !                target_pid_to_str (inferior_ptid));
>         annotate_thread_changed ();
>         previous_inferior_ptid = inferior_ptid;
>       }
>   
> --- 5795,5803 ----
>       {
>         target_terminal_ours_for_output ();
>         printf_filtered (_("[Switching to %s]\n"),
> !                target_pid_to_str (inferior_ptid));

I can't quite tell what changed in this line?

>         annotate_thread_changed ();
> +       previous_selected_ptid = previous_inferior_ptid;
>         previous_inferior_ptid = inferior_ptid;

I believe you should set or clear previous_selected_ptid
on program exits too.  This branch is only reached if
the program is still alive.

>       }

You'll need to handle a few more places.  There's the
"inferior" command -- see inferior.c and look for
switch_to_thread calls; and I think "detach" and "kill"
might need handling too(and the 
"... inferior" variants in inferior.c).

Otherwise it's looking good.  Thanks.

-- 
Pedro Alves

Re: Enhancement - show old and new thread info when switching during debugging

[pfee]

> > I tried  kmail which allows setting the content-type and content-disposition, 
>
> > but  the email didn't reach the gdb-patches list.  I'm reverting to  using 
>webmail 
>
> > and I'll put the patch as plain text below.
> 
> Looks  like the patch ends up line mangled that way.  :-/  The email  would
> have been rejected if you sent as html instead of text.   Otherwise, it's
> likely something with your smtp configuration in kmail.   I use kmail
> every day with no such issue.

In order to use kmail, I have to spoof my webmail address.  I think something is 

detecting that and blocking the messages.  Is this format ok, if not I'll switch 

to an address that works with kmail.

> 
> > 2011-08-09  Paul  Fee <pfee@talk21.com>
> 
> Double space  between your name and the email address too.
> 
> > 
> >      * inferior.h: Add $_prev_thread convenience variable.
> >     *  infrun.c: Add $_prev_thread convenience variable, set it when current 


> >  thread changes.
> >     * thread.c: Add $_prev_thread convenience  variable, set it when user 
> > switches threads.
> 
> Line  mangling/wrapping here.  Please keep lines under 80 columns.   Align
> with a tab.  Please see other changelog entries as guideline (or  better
> the GNU coding standards chapter on Change Logs).  Something  like:
> 
>     * inferior.h (prev_selected_thread):  Declare.
>     * infrun.c (prev_selected_thread): New  global.
>     (prev_thread_id_make_value): New  function.
>     ... etc.

Still learning, thanks for the pointers.  Unfortunately my webmail client eats 
leading tabs, so I can't get the formatting perfect.

> 
> > 
> > Index:  inferior.h
> >  ===================================================================
> > RCS  file: /cvs/src/src/gdb/inferior.h,v
> > retrieving revision 1.161
> >  diff -c -p -r1.161 inferior.h
> > *** inferior.h    21 Jul 2011  23:46:08 -0000    1.161
> > --- inferior.h    9 Aug 2011  14:44:23 -0000
> > *************** extern const char  *get_inferior_io_termi
> > *** 94,99 ****
> > --- 94,105  ----
> >  
> >   extern ptid_t inferior_ptid;
> >  
> > + /* Values move from interior_ptid to previous_inferior_ptid  to
> > +  * previous_selected_ptid.  The previous value is exposed  to the
> > +  * user through the $_prev_thread convenience  variable.
> > +  */
> > + extern ptid_t  previous_selected_ptid;
> 
> Same comment formatting again.  Typo  s/interior/inferior/
> 
> The comment is no longer correct.  I'd go for  not explaining the
> implementation, but what the variable is supposed to  track:
Ok

> 
>  /* The previously user/frontend selected thread.  Exposed  through the
>     $_prev_thread convenience variable.   */
>   extern ptid_t previous_selected_ptid;
> 
> and moving this  declaration to gdbthread.h, and the definition
> to thread.c.
Ok

> 
> >  Index: infrun.c
> >  ===================================================================
> > RCS  file: /cvs/src/src/gdb/infrun.c,v
> > retrieving revision 1.498
> > diff  -c -p -r1.498 infrun.c
> > *** infrun.c    4 Aug 2011 19:10:12  -0000    1.498
> > --- infrun.c    9 Aug 2011 14:44:29  -0000
> > *************** int sync_execution = 0;
> > *** 127,132  ****
> > --- 127,138 ----
> >  
> >   static ptid_t  previous_inferior_ptid;
> >  
> > + /* Values move from  interior_ptid to previous_inferior_ptid to
> > +     previous_selected_ptid.  The previous selected value is exposed
> >  +    to the user through the $_prev_thread convenience variable.   */
> 
> Comment duplication leads to bit rot.  Please drop this  copy.
Ok

> 
> > + 
> > + ptid_t previous_selected_ptid;
> > + 
> >   /* Default behavior is to detach newly forked processes  (legacy).  */
> >   int detach_fork = 1;
> >  
> >  *************** print_no_history_reason (void)
> > *** 5730,5735  ****
> > --- 5736,5753 ----
> >     ui_out_text  (current_uiout, "\nNo more reverse-execution history.\n");
> >    }
> >  
> > + /* Return the previous thread's id.  Return a  value of 0 if
> > +    no previous thread was selected, or it  doesn't exist.  */
> > + 
> > + struct value *
> > +  prev_thread_id_make_value (struct gdbarch *gdbarch, struct internalvar  
>*var)
> > + {
> > +   struct thread_info *tp = find_thread_ptid  (previous_selected_ptid);
> > + 
> > +   return value_from_longest  (builtin_type (gdbarch)->builtin_int,
> > +                   (tp ? tp->num : 0));
> > +  }
> 
> Please move this to thread.c, right next to  thread_id_make_value,
> and make it static.
Ok

> 
> > + 
> >   /*  Here to return control to GDB when the inferior stops for real.
> >       Print appropriate messages, remove breakpoints, give terminal our  
>modes.
> >  
> > *************** normal_stop (void)
> > ***  5777,5784 ****
> >       {
> >          target_terminal_ours_for_output ();
> >          printf_filtered (_("[Switching to %s]\n"),
> > !                 target_pid_to_str (inferior_ptid));
> >          annotate_thread_changed ();
> >          previous_inferior_ptid = inferior_ptid;
> >        }
> >  
> > --- 5795,5803 ----
> >        {
> >         target_terminal_ours_for_output  ();
> >         printf_filtered (_("[Switching to  %s]\n"),
> > !                 target_pid_to_str (inferior_ptid));
> 
> I can't quite tell what changed in  this line?
I made some tab/space changes, I'll revert the change to make my patch smaller.

> 
> >         annotate_thread_changed  ();
> > +       previous_selected_ptid =  previous_inferior_ptid;
> >          previous_inferior_ptid = inferior_ptid;
> 
> I believe you should set or clear  previous_selected_ptid
> on program exits too.  This branch is only  reached if
> the program is still alive.

The previous_selected_ptid remains populated when the program dies.  When 
the user accesses the convenience variable zero is returned as the 
find_thread_ptid() call within prev_thread_id_make_value() returns NULL.  Hence
we don't need to clear previous_selected_ptid.  Do you agree?

> 
> >        }
> 
> You'll need to handle a few more places.  There's  the
> "inferior" command -- see inferior.c and look for
> switch_to_thread  calls; and I think "detach" and "kill"
> might need handling too(and the 
> "... inferior" variants in inferior.c).

I looked at this.  I'm thinking that the previous_selected_ptid, rather than 
being a single global variable, should become a member of "struct inferior".  
Therefore you could switch back and forth between inferiors and have 
$_prev_thread updated to be specific to each inferior.

Before make that chance I'd like your advice.  Is that's what you had in
mind and would this be an appropriate way to implement it?

> 
> Otherwise it's looking  good.  Thanks.
Thanks for your help.  Revised patch below.

2011-08-09  Paul Fee  <pfee@talk21.com>
     * gdbthread.h: Declare storage for new convenience variable.
  * infrun.c (normal_stop): Set $_prev_thread when stopping in different thread.
  * thread.c: Define storage for new convenience variable.
  (do_captured_thread_select): Set $_prev_thread when user switches threads.
  (prev_thread_id_make_value): New function to lookup $_prev_thread.
  (_initialize_thread): Add $_prev_thread convenience variable.

Index: gdbthread.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbthread.h,v
retrieving revision 1.64
diff -r1.64 gdbthread.h
228a229,232
> /* The previously user/frontend selected thread.  Exposed through the
>    $_prev_thread convenience variable.  */
> extern ptid_t previous_selected_ptid;
> 
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.498
diff -r1.498 infrun.c
5781a5782
>       previous_selected_ptid = previous_inferior_ptid;
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.142
diff -r1.142 thread.c
49a50,51
> ptid_t previous_selected_ptid;
> 
1396a1399
>   previous_selected_ptid = inferior_ptid;
1451a1455,1467
> /* Return the previous thread's id.  Return a value of 0 if
>    no previous thread was selected, or it doesn't exist.  */
> 
> static struct value *
> prev_thread_id_make_value (struct gdbarch *gdbarch, struct internalvar *var)
> {
>  struct thread_info *tp = find_thread_ptid (previous_selected_ptid);
> 
>  return value_from_longest (builtin_type (gdbarch)->builtin_int,
>                             (tp ? tp->num : 0));
> }
> 
> 
1500a1517
>   create_internalvar_type_lazy ("_prev_thread", prev_thread_id_make_value);

Re: Enhancement - show old and new thread info when switching during debugging

[pfee]

> > Joel> Isn't that information already present in the previous "[Switching  to

> > Joel> thread ...]" message?
> > 
> > We can always add an  option, like "set print thread previous on" to
> > control the  output.
> 
> Generally speaking, I prefer to avoid introducing options  unless
> really necessary. In this particular case, it sounds like the  most
> convenient way of solving the problem is to have a  convenience
> variable. And it sounds like the only reason why Paul hasn't  looked
> at the implementation yet is due to lack of familiarity with  this
> code?
> 
>     % grep '"bpnum"' *.c
>      breakpoint.c:  set_internalvar_integer (lookup_internalvar ("bpnum"),  
>num);
> 
> So, I would think that adding something similar  in
> thread.c:switch_to_thread would do the job.
> 

I'll look into convenience variables.  Since it saves translating LWPs into GDB 
thread IDs and avoids output "noise", I think it's the better than my patch.

I'll warn you in advance it might be a week until I develop something as I may 
be without Internet access for a while.

Thanks for the tips on where to look next.

-- 
Paul

Re: Enhancement - show old and new thread info when switching during debugging

[Tom Tromey]

>>>>> "Paul" == pfee  <pfee@talk21.com> writes:

A few style nits in the patch...

Paul> +      const char* previous_pid_str = strdup (target_pid_to_str 

Space before the "*", not after.

Paul> +      free ((void*)previous_pid_str);

Use xfree, not free.
You don't need the cast.

Tom

Re: Enhancement - show old and new thread info when switching during debugging

[Jan Kratochvil]

On Fri, 29 Jul 2011 15:59:15 +0200, pfee@talk21.com wrote:
> This is useful if the switch was not intended 

If not obvious maybe in such case maybe you do not want the switching.

(gdb) set target-async on 
(gdb) set non-stop on
(gdb) run
[...]
Breakpoint 1, start (arg=0x0) at /home/jkratoch/t/threadit2.c:15
15	  sleep (100);
(gdb) info threads 
  2 Thread 0x7ffff7fd2700 (LWP 32656)  start (arg=0x0) at /home/jkratoch/t/threadit2.c:15
* 1 Thread 0x7ffff7fd3720 (LWP 32653)  (running)

GDB stayed at thread 1, thread 2 was stopped by that Breakpoint 1.

But sure non-stop mode is a bit different behavior from the all-stop mode.


Regards,
Jan