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

[pfee@talk21.com]

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