[PATCH] Another annotation for threads

[PATCH] Another annotation for threads

[Nick Roberts]

This is a patch for another annotation, this time for thread changes.  It
follows an earlier patch (not committed) for notifying thread changes in MI
using observers.  In that patch thread changes were notified in two places:
normal_stop in infrun.c and do_captured_thread_select in thread.c.  Here
normal_stop is used but the second call has been moved to thread_command so
that temporary thread changes in commands like "info thread", "-var-update" etc
aren't reported.

In the doc patch, I've used @r{, annotation} with findex but I don't know what
all this does as annotate.texinfo doesn't appear to have a function index.

If this patch is OK, I will submit a similar one for MI using an observer.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2008-05-21  Nick Roberts  <nickrob@snap.net.nz>

	* annotate.c (annotate_thread_changed): New function.
	* thread.c (thread_command) : Use it.
	* infrun.c (normal_stop): Use it.


2008-05-21  Nick Roberts  <nickrob@snap.net.nz>

	* annotate.texinfo (Multi-threaded Apps): Add entry for thread-changed
	annotation.


2008-05-21  Nick Roberts  <nickrob@snap.net.nz>

	* gdb.base/annota1.exp (thread_switch): Test for thread-changed
	annotation.


--- annotate.c.~1.16.~	2008-05-17 10:27:23.000000000 +1200
+++ annotate.c	2008-05-21 10:25:51.000000000 +1200
@@ -243,6 +243,15 @@ annotate_new_thread (void)
 }
 
 void
+annotate_thread_changed (void)
+{
+  if (annotation_level > 1)
+    {
+      printf_unfiltered (("\n\032\032thread-changed\n"));
+    }
+}
+
+void
 annotate_field_begin (struct type *type)
 {
   if (annotation_level == 2)


--- thread.c.~1.70.~	2008-05-17 20:02:16.000000000 +1200
+++ thread.c	2008-05-21 10:55:29.000000000 +1200
@@ -738,6 +738,7 @@ thread_command (char *tidstr, int from_t
       return;
     }
 
+  annotate_thread_changed ();
   gdb_thread_select (uiout, tidstr, NULL);
 }
 

--- infrun.c.~1.277.~	2008-05-08 16:32:23.000000000 +1200
+++ infrun.c	2008-05-21 10:26:59.000000000 +1200
@@ -3605,6 +3605,7 @@ normal_stop (void)
       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;
     }
 

--- annotate.texinfo.~1.9.~	2008-05-21 13:53:53.000000000 +1200
+++ annotate.texinfo	2008-05-21 14:14:18.000000000 +1200
@@ -825,17 +825,24 @@ depend on the language).
 @chapter Multi-threaded Applications
 @cindex annotations for multi-threaded apps
 
-@findex new-thread@r{, annotation}
-The following annotation reports that the application being debugged is
-multi-threaded.
+The following annotations report thread related changes of state.
 
-@smallexample
-^Z^Znew-thread
-@end smallexample
+@table @code
+@findex new-thread@r{, annotation}
+@item ^Z^Znew-thread
 
 This annotation is issued once for each thread that is created apart from
 the main thread, which is not reported.
 
+@findex thread-changed@r{, annotation}
+@item ^Z^Zthread-changed
+
+The selected thread has changed.  This may occur at the request of the
+user with the @code{thread} command, or as a result of execution,
+e.g., another thread hits a breakpoint.
+
+@end table
+
 @raisesections
 @include fdl.texi
 @lowersections


--- annota1.exp.~1.30.~	2008-05-20 18:48:14.000000000 +1200
+++ annota1.exp	2008-05-21 13:51:11.000000000 +1200
@@ -527,7 +527,18 @@ proc thread_test {} {
     }
 }
 
+proc thread_switch {} {
+	send_gdb "thread 1\n"
+	gdb_expect {
+	    -re ".*\032\032thread-changed" {
+	    pass "thread switch"
+	    }
+	    timeout { fail "thread switch (timeout)" }
+	}
+}
+
 thread_test
+thread_switch
 
 # restore the original prompt for the rest of the testsuite
 

Re: [PATCH] Another annotation for threads

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Wed, 21 May 2008 14:53:54 +1200
> 
> In the doc patch, I've used @r{, annotation} with findex but I don't know what
> all this does as annotate.texinfo doesn't appear to have a function index.

That's a bug created when we stopped @include-ing annotate.texinfo in
gdb.texinfo: there's no index in annotate.info at all.  Thanks for
catching this; I will fix it soon.  annotate.texinfo uses @findex
extensively, so it's not like you are introducing a new problem here.

> If this patch is OK, I will submit a similar one for MI using an observer.

This part is OK:

> 2008-05-21  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* annotate.texinfo (Multi-threaded Apps): Add entry for thread-changed
> 	annotation.

Thanks.

Re: [PATCH] Another annotation for threads

[Daniel Jacobowitz]

On Wed, May 21, 2008 at 02:53:54PM +1200, Nick Roberts wrote:
> 
> This is a patch for another annotation, this time for thread changes.  It
> follows an earlier patch (not committed) for notifying thread changes in MI
> using observers.  In that patch thread changes were notified in two places:
> normal_stop in infrun.c and do_captured_thread_select in thread.c.  Here
> normal_stop is used but the second call has been moved to thread_command so
> that temporary thread changes in commands like "info thread", "-var-update" etc
> aren't reported.
> 
> In the doc patch, I've used @r{, annotation} with findex but I don't know what
> all this does as annotate.texinfo doesn't appear to have a function index.
> 
> If this patch is OK, I will submit a similar one for MI using an observer.

It's quite likely that I've gotten turned around in all the discussion
while I was away, and you've already answered this.  But here's my
question anyway: if there's going to be an observer to do this in MI,
why shouldn't annotate.c use the same observer to call
annotate_thread_changed?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Another annotation for threads

[Nick Roberts]

 > > If this patch is OK, I will submit a similar one for MI using an observer.
 > 
 > It's quite likely that I've gotten turned around in all the discussion
 > while I was away, and you've already answered this.  But here's my
 > question anyway: if there's going to be an observer to do this in MI,
 > why shouldn't annotate.c use the same observer to call
 > annotate_thread_changed?

For the same reason that the "new-thread" annotation was eventually done
without annotations: GDB/MI developers might decide to call it at other
locations.

Also it means it has the same idiom as all the other annotations and, on it's
own, it's a simple change that's not very intrusive.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] Another annotation for threads

[Daniel Jacobowitz]

On Fri, Jun 06, 2008 at 09:20:06AM +1200, Nick Roberts wrote:
>  > > If this patch is OK, I will submit a similar one for MI using an observer.
>  > 
>  > It's quite likely that I've gotten turned around in all the discussion
>  > while I was away, and you've already answered this.  But here's my
>  > question anyway: if there's going to be an observer to do this in MI,
>  > why shouldn't annotate.c use the same observer to call
>  > annotate_thread_changed?
> 
> For the same reason that the "new-thread" annotation was eventually done
> without annotations: GDB/MI developers might decide to call it at other
> locations.
> 
> Also it means it has the same idiom as all the other annotations and, on it's
> own, it's a simple change that's not very intrusive.

I don't find that very convincing, but the patch is OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Another annotation for threads

[Joel Brobecker]

> > For the same reason that the "new-thread" annotation was eventually
> > done without annotations: GDB/MI developers might decide to call it
> > at other locations.
> > 
> > Also it means it has the same idiom as all the other annotations
> > and, on it's own, it's a simple change that's not very intrusive.
> 
> I don't find that very convincing, but the patch is OK.

FWIW, neither was I when we had the discussion of the new-thread annotation.
I eventually let it go because I thought that this was an isolated addition
and because it helped simplifying the emacs front-end. Sounds like I was
wrong about being an isolated addition :-(.

-- 
Joel

Re: [PATCH] Another annotation for threads

[Nick Roberts]

Daniel Jacobowitz writes:
 > On Fri, Jun 06, 2008 at 09:20:06AM +1200, Nick Roberts wrote:
 > >  > > If this patch is OK, I will submit a similar one for MI using an observer.
 > >  > 
 > >  > It's quite likely that I've gotten turned around in all the discussion
 > >  > while I was away, and you've already answered this.  But here's my
 > >  > question anyway: if there's going to be an observer to do this in MI,
 > >  > why shouldn't annotate.c use the same observer to call
 > >  > annotate_thread_changed?
 > > 
 > > For the same reason that the "new-thread" annotation was eventually done
 > > without annotations: GDB/MI developers might decide to call it at other
 > > locations.
 > > 
 > > Also it means it has the same idiom as all the other annotations and, on
 > > it's own, it's a simple change that's not very intrusive.
 > 
 > I don't find that very convincing, ...

But that's exactly what did happen.  Shortly after I submitted a patch for the
"new-thread" annotation which used the new_thread observer, the observer was
moved to report the main thread.  It's pragmatic argument rather than technical
one.  I have no control over MI development and Vladimir has stated on several
occasions that MI considerations are paramount.

                                   ... but the patch is OK.

Committed.  Thanks.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] Another annotation for threads

[Nick Roberts]

Joel Brobecker writes:
 > > > For the same reason that the "new-thread" annotation was eventually
 > > > done without annotations: GDB/MI developers might decide to call it
 > > > at other locations.
 > > > 
 > > > Also it means it has the same idiom as all the other annotations
 > > > and, on it's own, it's a simple change that's not very intrusive.
 > > 
 > > I don't find that very convincing, but the patch is OK.
 > 
 > FWIW, neither was I when we had the discussion of the new-thread annotation.
 > I eventually let it go because I thought that this was an isolated addition
 > and because it helped simplifying the emacs front-end. Sounds like I was
 > wrong about being an isolated addition :-(.

It might be construed as bad attitude but I don't really understand why
there is so much concern about such a small change that will only impact
thosee who use GDB from Emacs.

A while ago (three years actually) I submitted a patch to remove far more
markup than I am adding:

http://sourceware.org/ml/gdb-patches/2005-06/msg00189.html

but there was no interest.  

It started from a dialogue with Andrew Cagney to relieve some of the pressure
to remove the remaining annotations but only drew a response from Bob Rossi.  I
think we concluded that the breakpoints-invalid and frames-invalid annotations
could go.  So I'll offer now to submit another patch just to remove those
twoannotations (which fired far too often, anyway).  This should more than make
up for the two I've just added.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] Another annotation for threads

[Tom Tromey]

>>>>> "Nick" == Nick Roberts <nickrob@snap.net.nz> writes:

Nick> But that's exactly what did happen.  Shortly after I submitted a
Nick> patch for the "new-thread" annotation which used the new_thread
Nick> observer, the observer was moved to report the main thread.
Nick> It's pragmatic argument rather than technical one.  I have no
Nick> control over MI development and Vladimir has stated on several
Nick> occasions that MI considerations are paramount.

I'm curious about the long-term view in this area.  I'm perhaps not as
familiar with gdb history as I ought to be to enter this discussion...

It seems to me that the Python work will want notification of all the
same kinds of things that Emacs would, that MI would, and that Insight
would -- regardless of how something happens (an independent event in
the inferior, code run from Python, or user activity on the CLI),
folks will want to be able to write Python that reacts to these
somethings.

Abstractly I would have expected that the gdb core would use observer
notification, and the various state-change consumers (MI, annotations,
Python, etc) would simply register observers as needed.  (At least,
this is what I expected once I found out that events are deprecated.
I already had to add a couple of observers for Python, so this isn't
totally theoretical for me.)

Is this how things "ought" to work?  I mean ideally?
If not, would someone mind summarizing how it really should work?

Tom

Re: [PATCH] Another annotation for threads

[Daniel Jacobowitz]

On Thu, Jun 05, 2008 at 07:06:16PM -0600, Tom Tromey wrote:
> Is this how things "ought" to work?  I mean ideally?

Yes.  But there was disagreement over when a particular observer
ought to be called.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Another annotation for threads

[Pedro Alves]

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

A Friday 06 June 2008 01:49:12, Nick Roberts wrote:
> Daniel Jacobowitz writes:

>                                    ... but the patch is OK.
>
> Committed.  Thanks.

2008-05-21  Nick Roberts  <nickrob@snap.net.nz>

        * annotate.c (annotate_thread_changed): New function.
        * thread.c (thread_command) : Use it.
        * infrun.c (normal_stop): Use it.

This broke the build, as the annotate_thread_changed function isn't
declared anywhere.

I checked the attached in, as obvious.

Out of curiosity, was there a reason you committed every file as
individual commits?  It's customary to do a patch per commit
(not that is matter that much with CVS, but still nice to be
able to correlate by date, and to look at gdb-cvs).

-- 
Pedro Alves

[-- Attachment #2: annotate.diff --]
[-- Type: text/x-diff, Size: 591 bytes --]

2008-06-06  Pedro Alves  <pedro@codesourcery.com>

	* annotate.h (annotate_thread_changed): Declare.

---
 gdb/annotate.h |    1 +
 1 file changed, 1 insertion(+)

Index: src/gdb/annotate.h
===================================================================
--- src.orig/gdb/annotate.h	2008-06-06 03:27:03.000000000 +0100
+++ src/gdb/annotate.h	2008-06-06 03:27:44.000000000 +0100
@@ -44,6 +44,7 @@ extern void annotate_breakpoints_table_e
 
 extern void annotate_frames_invalid (void);
 extern void annotate_new_thread (void);
+extern void annotate_thread_changed (void);
 
 struct type;
 

Re: [PATCH] Another annotation for threads

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> On Thu, Jun 05, 2008 at 07:06:16PM -0600, Tom Tromey wrote:
>> Is this how things "ought" to work?  I mean ideally?

Daniel> Yes.  But there was disagreement over when a particular observer
Daniel> ought to be called.

I think I missed an earlier thread about this.

This disagreement could be solved, somewhat hackily, by adding more
information to the observer notification, so observers can more easily
discard the ones they are not interested in.

Anyway, thanks for responding.  I don't want to talk about it too much
in the abstract -- but I did want to get a general idea for changes we
write on the Python branch, and I think I understand now.

Tom

Re: [PATCH] Another annotation for threads

[Nick Roberts]

 > 2008-05-21  Nick Roberts  <nickrob@snap.net.nz>
 > 
 >         * annotate.c (annotate_thread_changed): New function.
 >         * thread.c (thread_command) : Use it.
 >         * infrun.c (normal_stop): Use it.
 > 
 > This broke the build, as the annotate_thread_changed function isn't
 > declared anywhere.
 > 
 > I checked the attached in, as obvious.

Yes, sorry.  I have several changes in that directory and I missed that file in
the patch that I posted.

 > Out of curiosity, was there a reason you committed every file as
 > individual commits?  It's customary to do a patch per commit
 > (not that is matter that much with CVS, but still nice to be
 > able to correlate by date, and to look at gdb-cvs).

It's the way VC works in Emacs.  I could use pcl-cvs but I've not really got
used to it.  Each file is committed individually but the changeset is given
in the ChangeLog, although multiple ChangeLogs are needed across directories
as is the case here.

It would be nice to move GDB to a modern version control system where patches
are really committed as changesets.  Systems such as GIT allow local branches
also which would help to keep local changes separate too.

It's also easier to try multiple file patches out from the mailing list and
revert them, etc, etc.

For now, anyway, I'll try to use pcl-cvs to keep the traffic down on gdb-cvs.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] Another annotation for threads

[Joel Brobecker]

> It might be construed as bad attitude but I don't really understand why
> there is so much concern about such a small change that will only impact
> thosee who use GDB from Emacs.

I don't personally consider this bad attitude, as you had some technical
arguments in favor of your approach. I balanced the various advantages
and drawbacks of both approaches, and judged that, although I would have
still prefered us to use observers, your approach was an acceptable
temporary compromise (temporary meaning until annotation are removed).
One of the elements on which I based my judgement was the fact that
I understood that this was going to be the only new annotation.

> A while ago (three years actually) I submitted a patch to remove far more
> markup than I am adding:
> 
> http://sourceware.org/ml/gdb-patches/2005-06/msg00189.html
> 
> but there was no interest.  
> 
> It started from a dialogue with Andrew Cagney to relieve some of the
> pressure to remove the remaining annotations but only drew a response
> from Bob Rossi.  I think we concluded that the breakpoints-invalid and
> frames-invalid annotations could go.  So I'll offer now to submit
> another patch just to remove those twoannotations (which fired far too
> often, anyway).  This should more than make up for the two I've just
> added.

My perspective on this is that I would like to see as little effort
spent on annotations as possible. If you think the change is going
to make GDB better, then perhaps your suggestion is worth it. If it's
just to reduce the pressure to remove annotations from GDB, then
I think we need to look at the maintenance cost of keeping them in.
If possible, I'd rather remove them all in one go rather than remove
some of them in exchange for the new ones being added.

I don't know the annotations well enough to know what the actual impact
might be.  They might make GDB better.  But I'm quite concerned about
the front ends out there that might be relying on some the annotations
that are proposed to be removed.

-- 
Joel

Re: [PATCH] Another annotation for threads

[Daniel Jacobowitz]

On Fri, Jun 06, 2008 at 02:59:41PM +1200, Nick Roberts wrote:
> It would be nice to move GDB to a modern version control system where patches
> are really committed as changesets.  Systems such as GIT allow local branches
> also which would help to keep local changes separate too.

I have strong hopes of moving us to Subversion, which would simplify
mirroring into git.  I'll be talking more about this in a week or two,
once Subversion 1.5 is released - it's at rc9 just now.

-- 
Daniel Jacobowitz
CodeSourcery