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

Pedro Alves
2011-08-10T15:10:50Z

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