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

[Pedro Alves <pedro@codesourcery.com>]

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