[PATCH] New annotation for threads

[PATCH] New annotation for threads

[Nick Roberts]

This is just an annotation equivalent of Vladimir's patch for an observer
for new threads.

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


2008-04-18  Nick Roberts  <nickrob@snap.net.nz>

	* annotate.h: (annotate_new_thread): New extern.

	* annotate.c (annotate_new_thread): New function.

	* thread.c (add_thread_with_info): Use it.


** annotate.h	10 Jan 2008 11:34:22 +1300	1.9
--- annotate.h	18 Apr 2008 17:30:37 +1200	
*************** extern void annotate_record (void);
*** 43,48 ****
--- 43,49 ----
  extern void annotate_breakpoints_table_end (void);
  
  extern void annotate_frames_invalid (void);
+ extern void annotate_new_thread (void);
  
  struct type;
  
*** annotate.c	10 Jan 2008 11:34:21 +1300	1.15
--- annotate.c	18 Apr 2008 17:29:19 +1200	
*************** annotate_frames_invalid (void)
*** 234,239 ****
--- 234,248 ----
  }
  
  void
+ annotate_new_thread (void)
+ {
+   if (annotation_level > 1)
+     {
+       printf_unfiltered (("\n\032\032new-thread\n"));
+     }
+ }
+ 
+ void
  annotate_field_begin (struct type *type)
  {
    if (annotation_level == 2)


*** thread.c	25 Mar 2008 08:17:29 +1200	1.65
--- thread.c	18 Apr 2008 17:31:41 +1200	
***************
*** 40,45 ****
--- 40,46 ----
  #include <signal.h>
  #include "ui-out.h"
  #include "observer.h"
+ #include "annotate.h"
  
  /* Definition of struct thread_info exported to gdbthread.h */
  
*************** add_thread_with_info (ptid_t ptid, struc
*** 140,146 ****
      printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid));
  
    observer_notify_new_thread (result);
!   
    return result;
  }
  
--- 141,148 ----
      printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid));
  
    observer_notify_new_thread (result);
!   annotate_new_thread ();
!  
    return result;
  }
  

Re: [PATCH] New annotation for threads

[Joel Brobecker]

> This is just an annotation equivalent of Vladimir's patch for an observer
> for new threads.

I'd really like to have others' opinions on this one.  I thought that
we weren't going to change our annotations anymore. And even though
we were not going to remove annotations for a while longer, we were
going to focuse on MI instead.

The change is sufficiently small that I'm thinking, why not, but can't
approve the suggestion just by myself.

Regarding the code itself:

> 2008-04-18  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* annotate.h: (annotate_new_thread): New extern.
> 
> 	* annotate.c (annotate_new_thread): New function.
> 
> 	* thread.c (add_thread_with_info): Use it.

How about creating a new observer for new_threads events instead of
specifically calling an annotation function from "add_thread"? Seems
much cleaner that way.

-- 
Joel

Re: [PATCH] New annotation for threads

[Nick Roberts]

 > > This is just an annotation equivalent of Vladimir's patch for an observer
 > > for new threads.
 > 
 > I'd really like to have others' opinions on this one.  I thought that
 > we weren't going to change our annotations anymore. And even though
 > we were not going to remove annotations for a while longer, we were
 > going to focuse on MI instead.

It doesn't impede MI development, so you could think of it as orthogonal.
Clearly it can eventually be removed with the rest of annotations.

 > The change is sufficiently small that I'm thinking, why not, but can't
 > approve the suggestion just by myself.

I would just use it to notify Emacs that an application was multi-threaded and
display the selected thread number in the mode-line.  It won't work if
Vladimir's patch is approved in it's original form (Apr 26: [RFA] Report the
main thread) but it now looks like any change made there will be just to
MI which won't affect this patch.

 > Regarding the code itself:
 > 
 > > 2008-04-18  Nick Roberts  <nickrob@snap.net.nz>
 > > 
 > > 	* annotate.h: (annotate_new_thread): New extern.
 > > 
 > > 	* annotate.c (annotate_new_thread): New function.
 > > 
 > > 	* thread.c (add_thread_with_info): Use it.
 > 
 > How about creating a new observer for new_threads events instead of
 > specifically calling an annotation function from "add_thread"? Seems
 > much cleaner that way.

I'm not sure what you mean, or how I would register such an observer.
All other annotations are done this way and, unlike MI, annotations don't
have their own interpreter but just mark up CLI.

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

Re: [PATCH] New annotation for threads

[Joel Brobecker]

>  > I'd really like to have others' opinions on this one.  I thought that
>  > we weren't going to change our annotations anymore. And even though
>  > we were not going to remove annotations for a while longer, we were
>  > going to focuse on MI instead.
> 
> It doesn't impede MI development, so you could think of it as orthogonal.
> Clearly it can eventually be removed with the rest of annotations.

OK, here is what I can propose: Send a patch for approval that implements
this new annotation using observers. I will review it, and then we will
let it sit for an extra week to give everyone time to object if they
disagree.

>  > > 2008-04-18  Nick Roberts  <nickrob@snap.net.nz>
>  > > 
>  > > 	* annotate.h: (annotate_new_thread): New extern.
>  > > 
>  > > 	* annotate.c (annotate_new_thread): New function.
>  > > 
>  > > 	* thread.c (add_thread_with_info): Use it.
>  > 
>  > How about creating a new observer for new_threads events instead of
>  > specifically calling an annotation function from "add_thread"? Seems
>  > much cleaner that way.
> 
> I'm not sure what you mean, or how I would register such an observer.
> All other annotations are done this way and, unlike MI, annotations don't
> have their own interpreter but just mark up CLI.

The observers are documented in doc/observer.texi. Basically, you need
to do the following:

    static void
    annotate_new_thread (struct thread_info *thread)
    {
      /* Your annotation here.  */
    }

And then attach this observer to the new_thread event using:

    observer_attach_new_thread (annotate_new_thread);

All this can be done inside annotate.c.

-- 
Joel

Re: [PATCH] New annotation for threads

[Daniel Jacobowitz]

On Thu, May 01, 2008 at 11:17:58AM -0700, Joel Brobecker wrote:
> OK, here is what I can propose: Send a patch for approval that implements
> this new annotation using observers. I will review it, and then we will
> let it sit for an extra week to give everyone time to object if they
> disagree.

I agree there's no harm in the new annotation.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] New annotation for threads

[Nick Roberts]

 > >  > How about creating a new observer for new_threads events instead of
 > >  > specifically calling an annotation function from "add_thread"? Seems
 > >  > much cleaner that way.
 > > 
 > > I'm not sure what you mean, or how I would register such an observer.
 > > All other annotations are done this way and, unlike MI, annotations don't
 > > have their own interpreter but just mark up CLI.
 > 
 > The observers are documented in doc/observer.texi. Basically, you need
 > to do the following:
 > 
 >     static void
 >     annotate_new_thread (struct thread_info *thread)
 >     {
 >       /* Your annotation here.  */
 >     }
 > 
 > And then attach this observer to the new_thread event using:
 > 
 >     observer_attach_new_thread (annotate_new_thread);

Yes, I can see how observers work.  I sent a patch earlier for thread-changed
and frame-changed events.

 > All this can be done inside annotate.c.

Where in annotate.c?  It can't be in _initialize_annotate because that only
gets called at startup and the annotation level can be changed with the
"set annotate" command (which is also why deprecated_delete_breakpoint_hook
and deprecated_modify_breakpoint_hook shouldn't be set there).

None of the other annotations are implemented using observers and this patch is
just meant as a stop gap before migrating fully to MI, not as a permanent
feature.

I therefore ask to commit this patch as is on the grounds that:

1) Emacs is the only application likely to use it.
2) All other users will be unaffected by this change.

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

Re: [PATCH] New annotation for threads

[Joel Brobecker]

>  > All this can be done inside annotate.c.
> 
> Where in annotate.c?  It can't be in _initialize_annotate because that only
> gets called at startup and the annotation level can be changed with the
> "set annotate" command (which is also why deprecated_delete_breakpoint_hook
> and deprecated_modify_breakpoint_hook shouldn't be set there).

The printing of the annotation was already guarded by the annotation
level. So you can in fact attach the observer from your _initialize
procedure, no?

-- 
Joel

Re: [PATCH] New annotation for threads

[Nick Roberts]

 > The printing of the annotation was already guarded by the annotation
 > level. So you can in fact attach the observer from your _initialize
 > procedure, no?

Yes.  You're probably right this looks a better change as
observer_attach_new_thread already exists and changes are confined to
annotate.c.  The patch now looks like below.  I'll write a test and fully check
the testsuite.

Thanks.

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


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

	* annotate.c: Include observer.h.
	(annotate_new_thread): New function
	(_initialize_annotate): Register it in the new_thread observer.


*** annotate.c	10 Jan 2008 11:34:21 +1300	1.15
--- annotate.c	02 May 2008 11:54:05 +1200	
***************
*** 23,28 ****
--- 23,29 ----
  #include "target.h"
  #include "gdbtypes.h"
  #include "breakpoint.h"
+ #include "observer.h"
  \f
  
  /* Prototypes for local functions. */
*************** annotate_frames_invalid (void)
*** 234,239 ****
--- 235,249 ----
  }
  
  void
+ annotate_new_thread (struct thread_info *thread)
+ {
+   if (annotation_level > 1)
+     {
+       printf_unfiltered (("\n\032\032new-thread\n"));
+     }
+ }
+ 
+ void
  annotate_field_begin (struct type *type)
  {
    if (annotation_level == 2)
*************** _initialize_annotate (void)
*** 580,583 ****
--- 590,595 ----
        deprecated_delete_breakpoint_hook = breakpoint_changed;
        deprecated_modify_breakpoint_hook = breakpoint_changed;
      }
+ 
+     observer_attach_new_thread (annotate_new_thread);
  }

Re: [PATCH] New annotation for threads

[Joel Brobecker]

> 2008-05-02  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* annotate.c: Include observer.h.
> 	(annotate_new_thread): New function
> 	(_initialize_annotate): Register it in the new_thread observer.

This is pretty good. But because you have added a #include, you need
to update the dependencies for annotate.c in Makefile.in:

> annotate.o: annotate.c $(defs_h) $(annotate_h) $(value_h) $(target_h) \
>         $(gdbtypes_h) $(breakpoint_h)

Add: $(annotate_h) at then end of the line.

-- 
Joel

Re: [PATCH] New annotation for threads

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Fri, 2 May 2008 12:06:54 +1200
> Cc: gdb-patches@sources.redhat.com
> 
> 2008-05-02  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* annotate.c: Include observer.h.
> 	(annotate_new_thread): New function
> 	(_initialize_annotate): Register it in the new_thread observer.

Thanks, but please add a corresponding bit to the user manual.  The
existing annotations are described there.

Re: [PATCH] New annotation for threads

[Nick Roberts]

 > Thanks, but please add a corresponding bit to the user manual.  The
 > existing annotations are described there.

I plan to represent this patch with a test shortly.  Documenting a feature
helps and encourages it's use.  For this reason, and because annotations is
deprecated, I was intending not to document it.  Why do you think it should be
documented?

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

Re: [PATCH] New annotation for threads

[Stan Shebs]

Nick Roberts wrote:
>  > Thanks, but please add a corresponding bit to the user manual.  The
>  > existing annotations are described there.
>
> I plan to represent this patch with a test shortly.  Documenting a feature
> helps and encourages it's use.  For this reason, and because annotations is
> deprecated, I was intending not to document it.  Why do you think it should be
> documented?
>   
If it's in the code and expected to be maintained, then it should be 
documented. How many times on this list have people been scratching 
their heads trying to figure out what an undocumented bit of code was 
supposed to do? These "deprecated" things seem to have a way of lasting 
longer than anyone's memories. :-)

Stan

Re: [PATCH] New annotation for threads

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sat, 17 May 2008 23:14:50 +1200
> Cc: brobecker@adacore.com, gdb-patches@sources.redhat.com
> 
> Documenting a feature helps and encourages it's use.

Then you should submit a patch to remove all docs of annotations from
the manual, and try to convince us to accept it.  Not documenting just
one piece is inconsistent and doesn't make sense.

> For this reason, and because annotations is deprecated, I was
> intending not to document it.  Why do you think it should be
> documented?

Because annotations are generally documented, and because they are
still part of GDB.  As long as annotations are part of GDB, I will
object to any user-visible change in them that is not documented in
the manual.

Re: [PATCH] New annotation for threads

[Bob Rossi]

On Sat, May 17, 2008 at 04:53:02PM +0300, Eli Zaretskii wrote:
> > From: Nick Roberts <nickrob@snap.net.nz>
> > Date: Sat, 17 May 2008 23:14:50 +1200
> > Cc: brobecker@adacore.com, gdb-patches@sources.redhat.com
> > 
> > Documenting a feature helps and encourages it's use.
> 
> Then you should submit a patch to remove all docs of annotations from
> the manual, and try to convince us to accept it.  Not documenting just
> one piece is inconsistent and doesn't make sense.

I would object to a patch like this. I think that it should be obvious
that the mi is still heavily under development. I feel confident that
Vladmir might be able to get it usable and stable if he keeps up the
good work. Maybe at that point we'd be ready to official drop mi.

Bob Rossi

Re: [PATCH] New annotation for threads

[Bob Rossi]

On Sat, May 17, 2008 at 09:57:57AM -0400, Bob Rossi wrote:
> On Sat, May 17, 2008 at 04:53:02PM +0300, Eli Zaretskii wrote:
> > > From: Nick Roberts <nickrob@snap.net.nz>
> > > Date: Sat, 17 May 2008 23:14:50 +1200
> > > Cc: brobecker@adacore.com, gdb-patches@sources.redhat.com
> > > 
> > > Documenting a feature helps and encourages it's use.
> > 
> > Then you should submit a patch to remove all docs of annotations from
> > the manual, and try to convince us to accept it.  Not documenting just
> > one piece is inconsistent and doesn't make sense.
> 
> I would object to a patch like this. I think that it should be obvious
> that the mi is still heavily under development. I feel confident that
> Vladmir might be able to get it usable and stable if he keeps up the
> good work. Maybe at that point we'd be ready to official drop mi.

Oops, meant "officially drop annotations".

Bob Rossi

Re: [PATCH] New annotation for threads

[Eli Zaretskii]

> Date: Sat, 17 May 2008 09:57:57 -0400
> From: Bob Rossi <bob_rossi@cox.net>
> Cc: Nick Roberts <nickrob@snap.net.nz>, brobecker@adacore.com,
> 	gdb-patches@sources.redhat.com
> 
> On Sat, May 17, 2008 at 04:53:02PM +0300, Eli Zaretskii wrote:
> > > From: Nick Roberts <nickrob@snap.net.nz>
> > > Date: Sat, 17 May 2008 23:14:50 +1200
> > > Cc: brobecker@adacore.com, gdb-patches@sources.redhat.com
> > > 
> > > Documenting a feature helps and encourages it's use.
> > 
> > Then you should submit a patch to remove all docs of annotations from
> > the manual, and try to convince us to accept it.  Not documenting just
> > one piece is inconsistent and doesn't make sense.
> 
> I would object to a patch like this.

Me too, but if Nick could convince most of the other maintainers, I'd
have hard time refusing to do it.

Re: [PATCH] New annotation for threads

[Joel Brobecker]

> > > Then you should submit a patch to remove all docs of annotations from
> > > the manual, and try to convince us to accept it.  Not documenting just
> > > one piece is inconsistent and doesn't make sense.
> > 
> > I would object to a patch like this.
> 
> Me too, but if Nick could convince most of the other maintainers, I'd
> have hard time refusing to do it.

I would also object to such a patch. My opinion on this is that,
as long as we support annotations, they should be properly documented.
The fact that, eventually, they will be phased out in favor of MI
should not influence the quality of our documentation for what is
currently supported.

-- 
Joel

Re: [PATCH] New annotation for threads

[Nick Roberts]

 > > Documenting a feature helps and encourages it's use.
 > 
 > Then you should submit a patch to remove all docs of annotations from
 > the manual, and try to convince us to accept it.  Not documenting just
 > one piece is inconsistent and doesn't make sense.

The difference, of course, is that all the current documentation was presumably
written before annotations were deprecated and before MI existed.  Indeed, a
long time ago Andrew Cagney moved the description for most annotations out of
the GDB manual into a separate file called annotate.texinfo.

 > > For this reason, and because annotations is deprecated, I was
 > > intending not to document it.  Why do you think it should be
 > > documented?
 > 
 > Because annotations are generally documented, and because they are
 > still part of GDB.  As long as annotations are part of GDB, I will
 > object to any user-visible change in them that is not documented in
 > the manual.

I will do this then as it will be easier to document it than try to argue that
it's not desirable.

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

Re: [PATCH] New annotation for threads

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sun, 18 May 2008 09:17:29 +1200
> Cc: brobecker@adacore.com, gdb-patches@sources.redhat.com
> 
>  > Because annotations are generally documented, and because they are
>  > still part of GDB.  As long as annotations are part of GDB, I will
>  > object to any user-visible change in them that is not documented in
>  > the manual.
> 
> I will do this then as it will be easier to document it than try to argue that
> it's not desirable.

Thank you.

Re: [PATCH] New annotation for threads

[Nick Roberts]

 > I plan to represent this patch with a test shortly...

Here it is with documentation.  This is pretty much what I submitted
originally.  Since then the new_thread observer has been moved to
add_thread_silent and, as Daniel points out, might be called at other locations
or change in the future.  With the benefit of hindsight, I think these are good
reasons not to use the observer for annotations.

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


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

	* annotate.c (annotate_new_thread): New function for new-thread
	annotation.

	* annotate.h: (annotate_new_thread): New extern.

	* thread.c (add_thread_with_info): Use it.

	* Makefile.in (thread.o): Add dependency on annotate.h.

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

	* gdb.base/annota1.exp: Test for new annotation.

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

	* annotate.texinfo (Multi-threaded Apps): New node for new annotation.


Index: annotate.c
===================================================================
RCS file: /cvs/src/src/gdb/annotate.c,v
retrieving revision 1.15
diff -p -u -r1.15 annotate.c
--- annotate.c	1 Jan 2008 22:53:09 -0000	1.15
+++ annotate.c	18 May 2008 09:07:38 -0000
@@ -234,6 +234,15 @@ annotate_frames_invalid (void)
 }
 
 void
+annotate_new_thread (void)
+{
+  if (annotation_level > 1)
+    {
+      printf_unfiltered (("\n\032\032new-thread\n"));
+    }
+}
+
+void
 annotate_field_begin (struct type *type)
 {
   if (annotation_level == 2)

Index: annotate.h
===================================================================
RCS file: /cvs/src/src/gdb/annotate.h,v
retrieving revision 1.9
diff -p -u -r1.9 annotate.h
--- annotate.h	1 Jan 2008 22:53:09 -0000	1.9
+++ annotate.h	18 May 2008 09:07:38 -0000
@@ -43,6 +43,7 @@ extern void annotate_record (void);
 extern void annotate_breakpoints_table_end (void);
 
 extern void annotate_frames_invalid (void);
+extern void annotate_new_thread (void);
 
 struct type;
 
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.69
diff -p -u -r1.69 thread.c
--- thread.c	9 May 2008 08:14:47 -0000	1.69
+++ thread.c	18 May 2008 09:07:39 -0000
@@ -40,6 +40,7 @@
 #include <signal.h>
 #include "ui-out.h"
 #include "observer.h"
+#include "annotate.h"
 
 /* Definition of struct thread_info exported to gdbthread.h */
 
@@ -141,7 +142,8 @@ add_thread_with_info (ptid_t ptid, struc
 
   if (print_thread_events)
     printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid));
-  
+
+  annotate_new_thread ();
   return result;
 }
 
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.1021
diff -p -u -r1.1021 Makefile.in
--- Makefile.in	16 May 2008 12:51:21 -0000	1.1021
+++ Makefile.in	18 May 2008 09:07:41 -0000
@@ -2932,7 +2932,7 @@ target-memory.o: target-memory.c $(defs_
 thread.o: thread.c $(defs_h) $(symtab_h) $(frame_h) $(inferior_h) \
 	$(environ_h) $(value_h) $(target_h) $(gdbthread_h) $(exceptions_h) \
 	$(command_h) $(gdbcmd_h) $(regcache_h) $(gdb_h) $(gdb_string_h) \
-	$(ui_out_h) $(observer_h)
+	$(ui_out_h) $(observer_h) $(annotate_h)
 top.o: top.c $(defs_h) $(gdbcmd_h) $(call_cmds_h) $(cli_cmds_h) \
 	$(cli_script_h) $(cli_setshow_h) $(cli_decode_h) $(symtab_h) \
 	$(inferior_h) $(exceptions_h) $(target_h) $(breakpoint_h) \

Index: testsuite/gdb.base/annota1.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/annota1.exp,v
retrieving revision 1.29
diff -p -u -r1.29 annota1.exp
--- testsuite/gdb.base/annota1.exp	16 May 2008 12:48:08 -0000	1.29
+++ testsuite/gdb.base/annota1.exp	18 May 2008 09:07:42 -0000
@@ -490,6 +490,39 @@ if [ regexp "core not found" $exec_outpu
     }
 }
 
+set testfile "watch_thread_num"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+set gdb_prompt $old_gdb_prompt
+
+if { ![get_compiler_info ${binfile}] && [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] == "" } {
+
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+    gdb_load ${binfile}
+    if { ![runto main] } then {
+	fail "run to main"
+	return
+    }
+
+    set gdb_prompt \
+    "\r\n\032\032pre-prompt\r\n$gdb_prompt \r\n\032\032prompt\r\n"
+
+    send_gdb "set annotate 2\n"
+    gdb_expect {
+	-re "set annotate 2\r\n$gdb_prompt$" {}
+    }
+
+    send_gdb "next 2\n"
+    gdb_expect {
+	-re ".*\032\032new-thread" {
+	    pass "new thread"
+	}
+	timeout { fail "new thread (timeout)" }
+    }
+}
+
 # restore the original prompt for the rest of the testsuite
 
 set gdb_prompt $old_gdb_prompt

Index: doc/annotate.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/annotate.texinfo,v
retrieving revision 1.7
diff -p -u -r1.7 annotate.texinfo
--- doc/annotate.texinfo	20 Jan 2007 16:51:56 -0000	1.7
+++ doc/annotate.texinfo	18 May 2008 09:07:42 -0000
@@ -85,6 +85,7 @@ This is Edition @value{EDITION}, @value{
 * Annotations for Running::
                         Whether the program is running, how it stopped, etc.
 * Source Annotations::  Annotations describing source code.
+* Multi-threaded Apps:: An annotation that reports multi-threadedness.
 
 * GNU Free Documentation License::
 @end menu
@@ -820,6 +821,21 @@ source which is being displayed.  @var{a
 followed by one or more lowercase hex digits (note that this does not
 depend on the language).
 
+@node Multi-threaded Apps
+@chapter Multi-threaded Applications
+@cindex annotations for multi-threaded apps
+
+@findex new-thread
+The following annotation reports that the application being debugged is
+multi-threaded.
+
+@smallexample
+^Z^Znew-thread
+@end smallexample
+
+This annotation is issued once for each thread that is created apart from
+the main thread, which is not reported.
+
 @raisesections
 @include fdl.texi
 @lowersections

Re: [PATCH] New annotation for threads

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sun, 18 May 2008 21:11:04 +1200
> 
> 2008-05-18  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* annotate.texinfo (Multi-threaded Apps): New node for new annotation.

Thanks, this is fine, with one comment:

> +@findex new-thread

Please make this

  @findex new-thread@r{, annotation}

Re: [PATCH] New annotation for threads

[Joel Brobecker]

> 2008-05-18  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* annotate.c (annotate_new_thread): New function for new-thread
> 	annotation.
> 
> 	* annotate.h: (annotate_new_thread): New extern.
> 
> 	* thread.c (add_thread_with_info): Use it.
> 
> 	* Makefile.in (thread.o): Add dependency on annotate.h.

(note sure if the empty lines between each entry was intended - just
make sure to remove them when entering this in the ChangeLog).

I'm not terribly happy about this, but that's nonetheless a reasonable
compromise. So this part is OK.

> 2008-05-18  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* gdb.base/annota1.exp: Test for new annotation.

I have a few questions regarding this part:

> +set testfile "watch_thread_num"
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/${testfile}
> +set gdb_prompt $old_gdb_prompt
> +
> +if { ![get_compiler_info ${binfile}] && [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] == "" } {
> +
> +    gdb_exit
> +    gdb_start
> +    gdb_reinitialize_dir $srcdir/$subdir
> +    gdb_load ${binfile}
> +    if { ![runto main] } then {
> +	fail "run to main"
> +	return

I think this will cause the test to exit before you had a chance
to restore the gdb_prompt.  I suggest you put the entire block
of code inside a function, and then call that function at the end
of the script.  That way, you can write the body of the function
as if it was its own script (the practice is to "if gdb_compile != ""
then return..." so that you don't need increasing levels of nesting.
but that's very minor, I don't mind the way you wrote it)

Another alternative would be to have a new specific script. It wouldn't
be any more work at all, and might be cleaner (although, you still do
have to be careful about the gdb_prompt).

> +    send_gdb "set annotate 2\n"
> +    gdb_expect {
> +	-re "set annotate 2\r\n$gdb_prompt$" {}
> +    }
> +
> +    send_gdb "next 2\n"
> +    gdb_expect {
> +	-re ".*\032\032new-thread" {
> +	    pass "new thread"
> +	}
> +	timeout { fail "new thread (timeout)" }
> +    }

I think you should be able to use gdb_test in this case, no?
I realize that you are only repeating the practice that is currently
used throughout the file, but we're trying to avoid the send_gdb/
gdb_expect sequence whenever we can, and use either gdb_test or
gdb_test_multiple.  For one thing, both already handle the various
error conditions, including the timeout.

-- 
Joel

Re: [PATCH] New annotation for threads

[Nick Roberts]

Joel Brobecker writes:
 > > 2008-05-18  Nick Roberts  <nickrob@snap.net.nz>
 > > 
 > > 	* annotate.c (annotate_new_thread): New function for new-thread
 > > 	annotation.
 > > 
 > > 	* annotate.h: (annotate_new_thread): New extern.
 > > 
 > > 	* thread.c (add_thread_with_info): Use it.
 > > 
 > > 	* Makefile.in (thread.o): Add dependency on annotate.h.
 > 
 > (note sure if the empty lines between each entry was intended - just
 > make sure to remove them when entering this in the ChangeLog).

OK.

 > I'm not terribly happy about this, but that's nonetheless a reasonable
 > compromise. So this part is OK.
 > 
 > > 2008-05-18  Nick Roberts  <nickrob@snap.net.nz>
 > > 
 > > 	* gdb.base/annota1.exp: Test for new annotation.
 > 
 > I have a few questions regarding this part:
 > 
 > > +set testfile "watch_thread_num"
 > > +set srcfile ${testfile}.c
 > > +set binfile ${objdir}/${subdir}/${testfile}
 > > +set gdb_prompt $old_gdb_prompt
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 > > +
 > > +if { ![get_compiler_info ${binfile}] && [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] == "" } {
 > > +
 > > +    gdb_exit
 > > +    gdb_start
 > > +    gdb_reinitialize_dir $srcdir/$subdir
 > > +    gdb_load ${binfile}
 > > +    if { ![runto main] } then {
 > > +	fail "run to main"
 > > +	return
 > 
 > I think this will cause the test to exit before you had a chance
 > to restore the gdb_prompt.

I restore it before this test so I don't think so.

 >                             I suggest you put the entire block
 > of code inside a function, and then call that function at the end
 > of the script.  That way, you can write the body of the function
 > as if it was its own script (the practice is to "if gdb_compile != ""
 > then return..." so that you don't need increasing levels of nesting.
 > but that's very minor, I don't mind the way you wrote it)

Presumably it also means more return points.  Usually the test is done at
the start of the script where nesting would be more inconvenient.

 > Another alternative would be to have a new specific script. It wouldn't
 > be any more work at all, and might be cleaner (although, you still do
 > have to be careful about the gdb_prompt).

I'll put it inside a function.  I don't want to create a separate script
because I think this test belongs with all the other tests for level 2
annotations.

 > > +    send_gdb "set annotate 2\n"
 > > +    gdb_expect {
 > > +	-re "set annotate 2\r\n$gdb_prompt$" {}
 > > +    }
 > > +
 > > +    send_gdb "next 2\n"
 > > +    gdb_expect {
 > > +	-re ".*\032\032new-thread" {
 > > +	    pass "new thread"
 > > +	}
 > > +	timeout { fail "new thread (timeout)" }
 > > +    }
 > 
 > I think you should be able to use gdb_test in this case, no?
 > I realize that you are only repeating the practice that is currently
 > used throughout the file, but we're trying to avoid the send_gdb/
 > gdb_expect sequence whenever we can, and use either gdb_test or
 > gdb_test_multiple.  For one thing, both already handle the various
 > error conditions, including the timeout.

Well, I can't get gdb_test to work.  It says in annota1.exp:

# NOTE: When this prompt is in use the gdb_test procedure cannot be used because
# it assumes that the last char after the gdb_prompt is a white space. This is not
# true with this annotated prompt. So we must use send_gdb and gdb_expect.
#

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

Re: [PATCH] New annotation for threads

[Nick Roberts]

 > I'll put it inside a function.  I don't want to create a separate script
 > because I think this test belongs with all the other tests for level 2
 > annotations.

Like below.

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


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

	* gdb.base/annota1.exp (thread_test): Test for new annotation.


--- annota1.exp	17 May 2008 21:09:09 +1200	1.29
+++ annota1.exp	20 May 2008 18:48:14 +1200	
@@ -490,6 +490,45 @@ if [ regexp "core not found" $exec_outpu
     }
 }
 
+proc thread_test {} {
+    global objdir subdir srcdir
+    global gdb_prompt old_gdb_prompt
+    set testfile "watch_thread_num"
+    set srcfile ${testfile}.c
+    set binfile ${objdir}/${subdir}/${testfile}
+    set gdb_prompt $old_gdb_prompt
+
+    if { ![get_compiler_info ${binfile}] && [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] == "" } {
+
+	gdb_exit
+	gdb_start
+	gdb_reinitialize_dir $srcdir/$subdir
+	gdb_load ${binfile}
+	if { ![runto main] } then {
+	    fail "run to main"
+	    return
+	}
+
+	set gdb_prompt \
+	    "\r\n\032\032pre-prompt\r\n$gdb_prompt \r\n\032\032prompt\r\n"
+
+	send_gdb "set annotate 2\n"
+	gdb_expect {
+	-re "set annotate 2\r\n$gdb_prompt$" {}
+	}
+
+	send_gdb "next 2\n"
+	gdb_expect {
+	    -re ".*\032\032new-thread" {
+	    pass "new thread"
+	    }
+	    timeout { fail "new thread (timeout)" }
+	}
+    }
+}
+
+thread_test
+
 # restore the original prompt for the rest of the testsuite
 
 set gdb_prompt $old_gdb_prompt

Re: [PATCH] New annotation for threads

[Joel Brobecker]

>  > I'll put it inside a function.  I don't want to create a separate script
>  > because I think this test belongs with all the other tests for level 2
>  > annotations.
> 
> Like below.

That's reasonable. I, on the other hand, prefer having zillions of
little scripts rather than one master script, because investigating
a failure is often easier. These giant test scripts are a little
hard to follow sometimes...

> 2008-05-20  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* gdb.base/annota1.exp (thread_test): Test for new annotation.

OK.

Thank you,
-- 
Joel

Re: [PATCH] New annotation for threads

[Nick Roberts]

 > That's reasonable. I, on the other hand, prefer having zillions of
 > little scripts rather than one master script, because investigating
 > a failure is often easier. These giant test scripts are a little
 > hard to follow sometimes...

I find debugging all testsuite scripts hard.  Is it possible to run them
interactively?

 > > 2008-05-20  Nick Roberts  <nickrob@snap.net.nz>
 > > 
 > > 	* gdb.base/annota1.exp (thread_test): Test for new annotation.
 > 
 > OK.
 > 
 > Thank you,

Committed, along with the changes to the other files already posted.

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

Re: [PATCH] New annotation for threads

[Joel Brobecker]

> I find debugging all testsuite scripts hard.  Is it possible to run them
> interactively?

Not that I know of. Usually, I just follow the gdb.log file. This file
contains a transcript of the GDB session, so I see the commands being
sent and the output received. I use the .exp file to check what was
actually expected when I see a new FAIL...

-- 
Joel

Re: [PATCH] New annotation for threads

[Eli Zaretskii]

> Date: Mon, 19 May 2008 20:52:26 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com
> 
> > 2008-05-18  Nick Roberts  <nickrob@snap.net.nz>
> > 
> > 	* annotate.c (annotate_new_thread): New function for new-thread
> > 	annotation.
> > 
> > 	* annotate.h: (annotate_new_thread): New extern.
> > 
> > 	* thread.c (add_thread_with_info): Use it.
> > 
> > 	* Makefile.in (thread.o): Add dependency on annotate.h.
> 
> (note sure if the empty lines between each entry was intended - just
> make sure to remove them when entering this in the ChangeLog).

Those empty lines are the canonical ChangeLog format as Emacs
generates it: each file has an empty line before its first entry.

Why do you want Nick to remove them?

Re: [PATCH] New annotation for threads

[Joel Brobecker]

> Those empty lines are the canonical ChangeLog format as Emacs
> generates it: each file has an empty line before its first entry.
> 
> Why do you want Nick to remove them?

Because all the changes are inter-related. Here is what the GNU Coding
Standards say about this:

| Separate unrelated change log entries with blank lines. When two entries
| represent parts of the same change, so that they work together, then
| don't put blank lines between them. Then you can omit the file name and
| the asterisk when successive entries are in the same file.

-- 
Joel

Re: [PATCH] New annotation for threads

[Eli Zaretskii]

> Date: Tue, 20 May 2008 11:24:01 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: nickrob@snap.net.nz, gdb-patches@sources.redhat.com
> 
> | Separate unrelated change log entries with blank lines. When two entries
> | represent parts of the same change, so that they work together, then
> | don't put blank lines between them. Then you can omit the file name and
> | the asterisk when successive entries are in the same file.

That's extra manual work that we shouldn't insist on.  If several
entries represent a single changeset, one can precede them with a
single sentence saying what is the change about.

Re: [PATCH] New annotation for threads

[Joel Brobecker]

> > | Separate unrelated change log entries with blank lines. When two entries
> > | represent parts of the same change, so that they work together, then
> > | don't put blank lines between them. Then you can omit the file name and
> > | the asterisk when successive entries are in the same file.
> 
> That's extra manual work that we shouldn't insist on.  If several
> entries represent a single changeset, one can precede them with a
> single sentence saying what is the change about.

Traditionally, we have followed the advice of the GNU Coding standards
and not used the empty line when the changes were inter-related. I
personally find that this makes the entry more readable, and I suspect
that this was the reason behind the suggestion in the GCS - I can accept
however that this is a matter of taste and also probably habit as well.
But I think that asking someone to follow this rule is reasonable given
the little amount of extra work that it requires. I would also suggest
that emacs be fixed to follow the GCS.

That being said, I'm not going to argue pro or against what you suggest.
I am ok with the deviation if the other maintainers are.

But in the meantime, I have to continue to make sure that contributions
meet the GCS.

-- 
Joel

Re: [PATCH] New annotation for threads

[Nick Roberts]

 > Traditionally, we have followed the advice of the GNU Coding standards
 > and not used the empty line when the changes were inter-related.

and

 > But I think that asking someone to follow this rule is reasonable given
 > the little amount of extra work that it requires. I would also suggest
 > that emacs be fixed to follow the GCS.

Presumably that would require Emacs to judge whether changes were related or
not.  THat would be a nice feature!

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

Re: [PATCH] New annotation for threads

[Joel Brobecker]

>  > But I think that asking someone to follow this rule is reasonable given
>  > the little amount of extra work that it requires. I would also suggest
>  > that emacs be fixed to follow the GCS.
> 
> Presumably that would require Emacs to judge whether changes were related or
> not.  THat would be a nice feature!

I wasn't clear. What I meant to say is that changes inside the same
ChangeLog entry are much more likely to be related than not. It would
probably be more efficient if emacs did not automatically add that
empty line. It won't be right 100% of the time, but it should be
most of the time.

-- 
Joel