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

pfee
2011-08-16T16:01:38Z

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