[patch] Watchpoints: support for thread <thread_num> parameters

[patch] Watchpoints: support for thread <thread_num> parameters

[Luis Machado]

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

Hi,

This is a first try on the implementation of the additional "thread
<thread_num>" parameter for the watchpoint command in order to make GDB
stop only on specific threads when a watchpoint is triggered.

Basically i started parsing the arguments backwards, trying to locate
tokens that identify a "thread <thread_num>" command. After that i hand
all the left parameters over to the default expression parser.

Differently than the breakpoint command, the "thread <thread_num>"
parameter for watchpoints is located at the end of the argument list due
to ambiguities that would arise when putting that parameter in between
expressions, since those could contain strings that would require a more
complex parser with context capabilities, thus being able to figure out
what is part of an expression and what is not.

What do you think? Looking forward to ideas and suggestions to improve
it. We could change the location of the "thread <thread_num>" parameter
for breakpoints as well, in order to keep it standard.

Best regards,

-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center
e-mail: luisgpm@linux.vnet.ibm.com

[-- Attachment #2: watch_thread_param.diff --]
[-- Type: text/x-patch, Size: 3022 bytes --]

2007-08-16  Luis Machado  <luisgpm@linux.vnet.ibm.com>

  * breakpoint.c: (watch_command_1): Parse additional optional 
  "thread" parameter to the watchpoint command and set the 
  "thread" member of the breakpoint struct.

Index: gdb/breakpoint.c
===================================================================
--- gdb.orig/breakpoint.c	2007-08-16 12:37:20.000000000 -0700
+++ gdb/breakpoint.c	2007-08-16 13:45:26.000000000 -0700
@@ -5834,10 +5834,70 @@
   int i, other_type_used, target_resources_ok = 0;
   enum bptype bp_type;
   int mem_cnt = 0;
+  int thread = -1;
 
   init_sal (&sal);		/* initialize to zeroes */
 
-  /* Parse arguments.  */
+  /* Make sure that we actually have parameters to parse  */
+  if (arg != NULL && strlen (arg) >= 1)
+  {
+    toklen = strlen (arg); /* Size of argument list  */
+
+    /* Points tok to the end of the argument list  */
+    tok = arg + toklen;
+
+    /* Go backwards in the parameters list. Skip the last parameter.
+       If we're expecting a 'thread <thread_num>' parameter, this should
+       be the thread identifier.  */
+    while (strlen (tok) < toklen && (*tok == ' ' || *tok == '\t'))
+      tok--;
+    while (strlen (tok) < toklen && (*tok != ' ' && *tok != '\t'))
+      tok--;
+
+    /* Go backwards in the parameters list. Skip one more parameter.
+       If we're expecting a 'thread <thread_num>' parameter, we should
+       reach a "thread" token.  */
+    while (strlen (tok) < toklen && (*tok == ' ' || *tok == '\t'))
+      tok--;
+
+    end_tok = tok;
+
+    while (strlen (tok) < toklen && (*tok != ' ' && *tok != '\t'))
+      tok--;
+
+    /* Move the pointer forward to skip the whitespace and
+       calculate the length of the token.  */
+    tok++;
+    toklen = end_tok - tok;
+
+    if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
+    {
+      /* At this point we've found a "thread" token, which means
+         the user is trying to set a watchpoint that triggers
+         only in a specific thread.  */
+      char *tmptok;
+      char *thread_token;
+
+      /* Extract the thread ID from the next token.  */
+      thread_token = tok;
+      tok = end_tok + 1;
+      tmptok = tok;
+      thread = strtol (tok, &tok, 0);
+
+      if (tok == tmptok)
+        error (_("Incorrect parameter after thread keyword."));
+
+      if (!valid_thread_id (thread))
+        error (_("Unknown thread %d."), thread);
+
+      /* truncate the string and get rid of the thread <thread_num>
+         parameter before the parameter list is parsed by the
+         evaluate_expression() function.  */
+      *thread_token = '\0';
+    }
+  }
+
+  /* Parse the rest of the arguments.  */
   innermost_block = NULL;
   exp_start = arg;
   exp = parse_exp_1 (&arg, 0, 0);
@@ -5921,6 +5981,7 @@
   b = set_raw_breakpoint (sal, bp_type);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
+  b->thread = thread;
   b->disposition = disp_donttouch;
   b->exp = exp;
   b->exp_valid_block = exp_valid_block;

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Eli Zaretskii]

> From: Luis Machado <luisgpm@linux.vnet.ibm.com>
> Date: Thu, 16 Aug 2007 18:02:58 -0300
> 
> This is a first try on the implementation of the additional "thread
> <thread_num>" parameter for the watchpoint command in order to make GDB
> stop only on specific threads when a watchpoint is triggered.

Thanks.

> Basically i started parsing the arguments backwards, trying to locate
> tokens that identify a "thread <thread_num>" command. After that i hand
> all the left parameters over to the default expression parser.

Your code assumes that <thread_num> is a literal number.  Do we want
to allow an integral expression there, or is a literal number good
enough?

In any case, this change, if and when accepted, should be accompanied
by a suitable change to the user manual.

Allow me a few comments on your code:

> +    toklen = strlen (arg); /* Size of argument list  */
> +
> +    /* Points tok to the end of the argument list  */
> +    tok = arg + toklen;

Did you try your code when the argument has trailing whitespace?
Unless I'm missing something, the last line above make `tok' point to
the null character that ends the argument, so the while loop after
that will end immediately, even if there is whitespace after the
thread ID number, and the whole parsing business will not work
correctly.

> +    while (strlen (tok) < toklen && (*tok == ' ' || *tok == '\t'))
> +      tok--;
> +    while (strlen (tok) < toklen && (*tok != ' ' && *tok != '\t'))
> +      tok--;
> +
> +    /* Go backwards in the parameters list. Skip one more parameter.
> +       If we're expecting a 'thread <thread_num>' parameter, we should
> +       reach a "thread" token.  */
> +    while (strlen (tok) < toklen && (*tok == ' ' || *tok == '\t'))
> +      tok--;
> +
> +    end_tok = tok;
> +
> +    while (strlen (tok) < toklen && (*tok != ' ' && *tok != '\t'))
> +      tok--;

You don't need to compute `strlen (tok)' every time through the loops,
because each iteration you go back exactly one character.  So you can
compute it only once, and then decrement its value inside the loop
bodies.

Alternatively, just check that you didn't yet get to the first
character of the argument:

	  while (tok > arg && (*tok != ' ' && *tok != '\t'))
	    tok--;

I like this latter alternative better.

Actually, given the trailing whitespace problem I mentioned above,
this is even better:

	  while (tok > arg && (tok[-1] != ' ' && tok[-1] != '\t'))
	    tok--;

(You will need to adjust the rest of code to bump `tok' one position
forward after you exit the loop, to account for the -1 above.)

> +      tmptok = tok;
> +      thread = strtol (tok, &tok, 0);
> +
> +      if (tok == tmptok)
> +        error (_("Incorrect parameter after thread keyword."));

There's a much better way of checking whether `tok' is a literal
number: look at the pointer returned by `strtol' in its 2nd arg, and
if it points to something other than a whitespace character, err out:

     char *endp;
     thread = strtol (tok, &endp, 0);
     if (*endp != ' ' && *endp != '\t')
       error (_("Invalid thread ID specification %s."), tok);

Thanks again for working on this.

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Luis Machado]

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

Hi,

> Your code assumes that <thread_num> is a literal number.  Do we want
> to allow an integral expression there, or is a literal number good
> enough?

I believe the thread command assumes a literal number as well. Were you
thinking about more complex expressions defining the thread ID? Like a
variable or an arithmetic expression?

> In any case, this change, if and when accepted, should be accompanied
> by a suitable change to the user manual.

Agreed. We might as well keep the parameter format the same as for
breakpoint commands.

> Did you try your code when the argument has trailing whitespace?
> Unless I'm missing something, the last line above make `tok' point to
> the null character that ends the argument, so the while loop after
> that will end immediately, even if there is whitespace after the
> thread ID number, and the whole parsing business will not work
> correctly.

I did try the code with trailing white spaces and it seemed to work, but
maybe i didn't pick a specific case that would break the code. Anyway,
i've changed that in the new version to start before the null character,
so if we have any trailing white spaces or tabs, it'll handle them
correctly until the token shows up.

> You don't need to compute `strlen (tok)' every time through the loops,
> because each iteration you go back exactly one character.  So you can
> compute it only once, and then decrement its value inside the loop
> bodies.

Yes, that was something Bauermann suggested in the first version. That's
updated in the new version of the patch. I exchanged those strlen()'s
for "tok > arg" conditions.

> There's a much better way of checking whether `tok' is a literal
> number: look at the pointer returned by `strtol' in its 2nd arg, and
> if it points to something other than a whitespace character, err out...

This was also addressed in the new version. Looks better and cleaner.

I've also addressed some coding standard issues with identations and
spaces in comments.

Thanks for the suggestions Eli.

How does this new version look?

Best regards,

-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center
e-mail: luisgpm@linux.vnet.ibm.com

[-- Attachment #2: watch_thread_param.diff --]
[-- Type: text/x-patch, Size: 3534 bytes --]

2007-08-16  Luis Machado  <luisgpm@linux.vnet.ibm.com>

  * breakpoint.c: (watch_command_1): Parse additional optional 
  "thread" parameter to the watchpoint command and set the 
  "thread" member of the breakpoint struct.

Index: gdb/breakpoint.c
===================================================================
--- gdb.orig/breakpoint.c	2007-08-17 07:09:37.000000000 -0700
+++ gdb/breakpoint.c	2007-08-17 08:24:19.000000000 -0700
@@ -5826,7 +5826,7 @@
   struct frame_info *prev_frame = NULL;
   char *exp_start = NULL;
   char *exp_end = NULL;
-  char *tok, *end_tok;
+  char *tok, *id_tok_start, *end_tok;
   int toklen;
   char *cond_start = NULL;
   char *cond_end = NULL;
@@ -5834,10 +5834,72 @@
   int i, other_type_used, target_resources_ok = 0;
   enum bptype bp_type;
   int mem_cnt = 0;
+  int thread = -1;
 
   init_sal (&sal);		/* initialize to zeroes */
 
-  /* Parse arguments.  */
+  /* Make sure that we actually have parameters to parse.  */
+  if (arg != NULL && strlen (arg) >= 1)
+    {
+      toklen = strlen (arg); /* Size of argument list.  */
+
+      /* Points tok to the end of the argument list.  */
+      tok = arg + toklen - 1;
+
+      /* Go backwards in the parameters list. Skip the last parameter.
+         If we're expecting a 'thread <thread_num>' parameter, this should
+         be the thread identifier.  */
+      while (tok > arg && (*tok == ' ' || *tok == '\t'))
+        tok--;
+      while (tok > arg && (*tok != ' ' && *tok != '\t'))
+        tok--;
+
+      /* Points end_tok to the beginning of the last token.  */
+      id_tok_start = tok + 1;
+
+      /* Go backwards in the parameters list. Skip one more parameter.
+         If we're expecting a 'thread <thread_num>' parameter, we should
+         reach a "thread" token.  */
+      while (tok > arg && (*tok == ' ' || *tok == '\t'))
+        tok--;
+
+      end_tok = tok;
+
+      while (tok > arg && (*tok != ' ' && *tok != '\t'))
+        tok--;
+
+      /* Move the pointer forward to skip the whitespace and
+         calculate the length of the token.  */
+      tok++;
+      toklen = end_tok - tok;
+
+      if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
+        {
+          /* At this point we've found a "thread" token, which means
+             the user is trying to set a watchpoint that triggers
+             only in a specific thread.  */
+          char *endp;
+
+          /* Extract the thread ID from the next token.  */
+          thread = strtol (id_tok_start, &endp, 0);
+
+          /* Check if the user provided a valid numeric value for the
+             thread ID.  */
+          if (*endp != ' ' && *endp != '\t' && *endp != '\0')
+            error (_("Invalid thread ID specification %s."), id_tok_start);
+
+          /* Check if the thread actually exists.  */
+          if (!valid_thread_id (thread))
+            error (_("Unknown thread %d."), thread);
+
+          /* Truncate the string and get rid of the thread <thread_num>
+             parameter before the parameter list is parsed by the
+             evaluate_expression() function.  */
+          *tok = '\0';
+        }
+    }
+
+  /* Parse the rest of the arguments.  */
   innermost_block = NULL;
   exp_start = arg;
   exp = parse_exp_1 (&arg, 0, 0);
@@ -5921,6 +5983,7 @@
   b = set_raw_breakpoint (sal, bp_type);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
+  b->thread = thread;
   b->disposition = disp_donttouch;
   b->exp = exp;
   b->exp_valid_block = exp_valid_block;

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Eli Zaretskii]

> From: Luis Machado <luisgpm@linux.vnet.ibm.com>
> Cc: gdb-patches@sourceware.org
> Date: Fri, 17 Aug 2007 12:46:56 -0300
> 
> How does this new version look?

Fine, I'm happy now.

Is this all that needs to be done to have thread-specific watchpoints?
Do they work after this patch as you want them to?

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Daniel Jacobowitz]

On Fri, Aug 17, 2007 at 09:41:38PM +0300, Eli Zaretskii wrote:
> Is this all that needs to be done to have thread-specific watchpoints?
> Do they work after this patch as you want them to?

In my opinion, not really.  On some systems, you can tell the target
layer which threads the watchpoint should be inserted for and spare
yourself extra stops in "wrong" threads.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Daniel Jacobowitz]

On Mon, Aug 20, 2007 at 11:03:49AM -0300, Luis Machado wrote:
> On Fri, 2007-08-17 at 14:49 -0400, Daniel Jacobowitz wrote:
> > On Fri, Aug 17, 2007 at 09:41:38PM +0300, Eli Zaretskii wrote:
> > > Is this all that needs to be done to have thread-specific watchpoints?
> > > Do they work after this patch as you want them to?
> > 
> > In my opinion, not really.  On some systems, you can tell the target
> > layer which threads the watchpoint should be inserted for and spare
> > yourself extra stops in "wrong" threads.
> 
> What do you have in mind regarding the whole feature? Will we head
> towards a different solution?

The work you've done will still be necessary.  I have no plans to work
on the bigger picture, myself, but thread specificity needs to be
communicated to lower layers somehow.

Consider: in the current status quo the watchpoint would be inserted
in every thread, using up precious hardware debug resources and
triggering unnecessarily.  We'll just resume in threads other than the
indicated one, so we shouldn't set up the watchpoint anywhere else.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Luis Machado]

On Fri, 2007-08-17 at 14:49 -0400, Daniel Jacobowitz wrote:
> On Fri, Aug 17, 2007 at 09:41:38PM +0300, Eli Zaretskii wrote:
> > Is this all that needs to be done to have thread-specific watchpoints?
> > Do they work after this patch as you want them to?
> 
> In my opinion, not really.  On some systems, you can tell the target
> layer which threads the watchpoint should be inserted for and spare
> yourself extra stops in "wrong" threads.

What do you have in mind regarding the whole feature? Will we head
towards a different solution?

-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center
e-mail: luisgpm@linux.vnet.ibm.com

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Luis Machado]

Hi,

> Consider: in the current status quo the watchpoint would be inserted
> in every thread, using up precious hardware debug resources and
> triggering unnecessarily.  We'll just resume in threads other than the
> indicated one, so we shouldn't set up the watchpoint anywhere else.

Makes sense. So instead of having GDB insert watchpoints in all the
threads, we would insert it just in the one we specified in the command.
That could make use of the changes to the watchpoint command, but then
we're discarding Jeff's patches, right?

We would also need to do that cleanup in each thread's debugging
registers due to the kernel cloning the debug registers' values from the
parent process.

Regards,

-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center
e-mail: luisgpm@linux.vnet.ibm.com

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Daniel Jacobowitz]

On Mon, Aug 20, 2007 at 12:23:09PM -0300, Luis Machado wrote:
> Makes sense. So instead of having GDB insert watchpoints in all the
> threads, we would insert it just in the one we specified in the command.
> That could make use of the changes to the watchpoint command, but then
> we're discarding Jeff's patches, right?

No, this still requires Jeff's patches as a starting point.  I hope
to respond to them today.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Joel Brobecker]

> > Makes sense. So instead of having GDB insert watchpoints in all the
> > threads, we would insert it just in the one we specified in the command.
> > That could make use of the changes to the watchpoint command, but then
> > we're discarding Jeff's patches, right?
> 
> No, this still requires Jeff's patches as a starting point.  I hope
> to respond to them today.

Another reason why I believe these patches would still be necessary
is the fact that some targets might not support thread-specific
hardware watchpoints, right?

-- 
Joel

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Luis Machado]

On Thu, 2007-08-23 at 17:12 -0400, Joel Brobecker wrote:
> > > Makes sense. So instead of having GDB insert watchpoints in all the
> > > threads, we would insert it just in the one we specified in the command.
> > > That could make use of the changes to the watchpoint command, but then
> > > we're discarding Jeff's patches, right?
> > 
> > No, this still requires Jeff's patches as a starting point.  I hope
> > to respond to them today.
> 
> Another reason why I believe these patches would still be necessary
> is the fact that some targets might not support thread-specific
> hardware watchpoints, right?
> 

For that case it's a good idea to have something like the additional
parameter. In time, which ones have such behaviour?

Best regards,
-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center
e-mail: luisgpm@linux.vnet.ibm.com

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Daniel Jacobowitz]

On Fri, Aug 17, 2007 at 12:46:56PM -0300, Luis Machado wrote:
> 2007-08-16  Luis Machado  <luisgpm@linux.vnet.ibm.com>
> 
>   * breakpoint.c: (watch_command_1): Parse additional optional 
>   "thread" parameter to the watchpoint command and set the 
>   "thread" member of the breakpoint struct.

Like Eli, I'm happy enough with this version - but it needs
documentation and a test case.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Luis Machado]

Daniel, Eli,

Thanks for reviewing.

I'll provide the necessary bits of documentation and a testcase for this
one, then i'll attach those to the patch.

Regards,
On Thu, 2007-10-11 at 15:35 -0400, Daniel Jacobowitz wrote:
> On Fri, Aug 17, 2007 at 12:46:56PM -0300, Luis Machado wrote:
> > 2007-08-16  Luis Machado  <luisgpm@linux.vnet.ibm.com>
> > 
> >   * breakpoint.c: (watch_command_1): Parse additional optional 
> >   "thread" parameter to the watchpoint command and set the 
> >   "thread" member of the breakpoint struct.
> 
> Like Eli, I'm happy enough with this version - but it needs
> documentation and a test case.
> 
-- 
Luis Machado
IBM Linux Technology Center
e-mail: luisgpm@linux.vnet.ibm.com

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Luis Machado]

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

Hi folks,

Here is the patch together with the testcase and the documentation bits.
Please let me know if there are any improvements/corrections suitable
for this one.

Best regards,
Luis

On Thu, 2007-10-11 at 17:29 -0300, Luis Machado wrote:
> Daniel, Eli,
> 
> Thanks for reviewing.
> 
> I'll provide the necessary bits of documentation and a testcase for this
> one, then i'll attach those to the patch.
> 
> Regards,
> On Thu, 2007-10-11 at 15:35 -0400, Daniel Jacobowitz wrote:
> > On Fri, Aug 17, 2007 at 12:46:56PM -0300, Luis Machado wrote:
> > > 2007-08-16  Luis Machado  <luisgpm@linux.vnet.ibm.com>
> > > 
> > >   * breakpoint.c: (watch_command_1): Parse additional optional 
> > >   "thread" parameter to the watchpoint command and set the 
> > >   "thread" member of the breakpoint struct.
> > 
> > Like Eli, I'm happy enough with this version - but it needs
> > documentation and a test case.
> > 


[-- Attachment #2: watch_thread_param.diff --]
[-- Type: text/x-patch, Size: 9398 bytes --]

2007-11-13  Luis Machado  <luisgpm@br.ibm.com>

      * breakpoint.c: (watch_command_1): Parse additional optional 
      "thread" parameter to the watchpoint command and set the 
      "thread" member of the breakpoint struct.
      * doc/gdb.texinfo: Add new parameter's description.
      * testsuite/gdb.base/watch_thread_num.c: New testcase source file.
      * testsuite/gdb.base/watch_thread_num.exp: New testcase expect file.

Index: gdb/breakpoint.c
===================================================================
--- gdb.orig/breakpoint.c	2007-11-12 11:22:15.000000000 -0800
+++ gdb/breakpoint.c	2007-11-12 11:22:18.000000000 -0800
@@ -5882,7 +5882,7 @@
   struct frame_info *prev_frame = NULL;
   char *exp_start = NULL;
   char *exp_end = NULL;
-  char *tok, *end_tok;
+  char *tok, *id_tok_start, *end_tok;
   int toklen;
   char *cond_start = NULL;
   char *cond_end = NULL;
@@ -5890,10 +5890,72 @@
   int i, other_type_used, target_resources_ok = 0;
   enum bptype bp_type;
   int mem_cnt = 0;
+  int thread = -1;
 
   init_sal (&sal);		/* initialize to zeroes */
 
-  /* Parse arguments.  */
+  /* Make sure that we actually have parameters to parse.  */
+  if (arg != NULL && strlen (arg) >= 1)
+    {
+      toklen = strlen (arg); /* Size of argument list.  */
+
+      /* Points tok to the end of the argument list.  */
+      tok = arg + toklen - 1;
+
+      /* Go backwards in the parameters list. Skip the last parameter.
+         If we're expecting a 'thread <thread_num>' parameter, this should
+         be the thread identifier.  */
+      while (tok > arg && (*tok == ' ' || *tok == '\t'))
+        tok--;
+      while (tok > arg && (*tok != ' ' && *tok != '\t'))
+        tok--;
+
+      /* Points end_tok to the beginning of the last token.  */
+      id_tok_start = tok + 1;
+
+      /* Go backwards in the parameters list. Skip one more parameter.
+         If we're expecting a 'thread <thread_num>' parameter, we should
+         reach a "thread" token.  */
+      while (tok > arg && (*tok == ' ' || *tok == '\t'))
+        tok--;
+
+      end_tok = tok;
+
+      while (tok > arg && (*tok != ' ' && *tok != '\t'))
+        tok--;
+
+      /* Move the pointer forward to skip the whitespace and
+         calculate the length of the token.  */
+      tok++;
+      toklen = end_tok - tok;
+
+      if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
+        {
+          /* At this point we've found a "thread" token, which means
+             the user is trying to set a watchpoint that triggers
+             only in a specific thread.  */
+          char *endp;
+
+          /* Extract the thread ID from the next token.  */
+          thread = strtol (id_tok_start, &endp, 0);
+
+          /* Check if the user provided a valid numeric value for the
+             thread ID.  */
+          if (*endp != ' ' && *endp != '\t' && *endp != '\0')
+            error (_("Invalid thread ID specification %s."), id_tok_start);
+
+          /* Check if the thread actually exists.  */
+          if (!valid_thread_id (thread))
+            error (_("Unknown thread %d."), thread);
+
+          /* Truncate the string and get rid of the thread <thread_num>
+             parameter before the parameter list is parsed by the
+             evaluate_expression() function.  */
+          *tok = '\0';
+        }
+    }
+
+  /* Parse the rest of the arguments.  */
   innermost_block = NULL;
   exp_start = arg;
   exp = parse_exp_1 (&arg, 0, 0);
@@ -5986,6 +6048,7 @@
   b = set_raw_breakpoint (sal, bp_type);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
+  b->thread = thread;
   b->disposition = disp_donttouch;
   b->exp = exp;
   b->exp_valid_block = exp_valid_block;
Index: gdb/testsuite/gdb.base/watch_thread_num.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb/testsuite/gdb.base/watch_thread_num.c	2007-11-13 06:35:30.000000000 -0800
@@ -0,0 +1,63 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2002, 2003, 2004 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.
+
+   This file is copied from schedlock.c.  */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+
+void *thread_function (void *arg); /* Pointer to function executed by each thread */
+
+#define NUM 5
+
+static unsigned int shared_var = 1;
+
+int main () {
+    int res;
+    pthread_t threads[NUM];
+    void *thread_result;
+    long i;
+
+    for (i = 0; i < NUM; i++)
+      {
+        res = pthread_create (&threads[i],
+                             NULL,
+                             thread_function,
+			     (void *) i);
+      }
+
+    thread_result = thread_function ((void *) i);
+
+    exit (EXIT_SUCCESS);
+}
+
+void *thread_function (void *arg) {
+    int my_number = (long) arg;
+    /* Don't run forever.  Run just short of it :)  */
+    while (shared_var > 0)
+      {
+        shared_var++;
+	usleep (1); /* Loop increment.  */
+      }
+
+    pthread_exit (NULL);
+}
+
Index: gdb/testsuite/gdb.base/watch_thread_num.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb/testsuite/gdb.base/watch_thread_num.exp	2007-11-12 11:26:37.000000000 -0800
@@ -0,0 +1,67 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2007
+# Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+# watch-thread_num.exp   Test thread <thread_num> parameter for
+#                        watch commands.
+#
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+set testfile watch_thread_num
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+# What compiler are we using?
+#
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug libs=-lpthread}] != "" } {
+     untested watch_thread_num.exp
+     return -1
+}
+
+# use this to debug:
+#log_user 1
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto main] } then {
+   fail "run to main"
+   return
+}
+
+gdb_test "watch shared_var thread 0" "Unknown thread 0\." "Watchpoint on invalid thread"
+gdb_test "watch shared_var thread" "A syntax error in expression, near `thread'\." "Invalid watch syntax"
+
+gdb_test "Next 5" ""
+
+gdb_test "watch shared_var thread 2" "Hardware watchpoint 2: shared_var" "Watchpoint on shared variable"
+gdb_test "info breakpoint 2" "stop only in thread 2"
+
+for {set i 0} {$i < 10} {incr i 1} {
+gdb_test "continue" "Hardware watchpoint 2: shared_var.*" "Watchpoint triggered"
+gdb_test "thread" "\\\[Current thread is 2 \\\(Thread $hex \\\(LWP $decimal\\\)\\\)\\\]" "Check thread that triggered"
+}
+
Index: gdb/doc/gdb.texinfo
===================================================================
--- gdb.orig/doc/gdb.texinfo	2007-11-12 11:22:15.000000000 -0800
+++ gdb/doc/gdb.texinfo	2007-11-12 11:22:18.000000000 -0800
@@ -3210,7 +3210,7 @@
 catch errors where you have no clue what part of your program is the
 culprit.)
 
-On some systems, such as HP-UX, @sc{gnu}/Linux and most other
+On some systems, such as HP-UX, PowerPC, @sc{gnu}/Linux and most other
 x86-based targets, @value{GDBN} includes support for hardware
 watchpoints, which do not slow down the running of your program.
 
@@ -3351,6 +3351,13 @@
 In multi-threaded programs, watchpoints will detect changes to the
 watched expression from every thread.
 
+@kindex watch thread thread_num
+@item watch @var{expr} thread thread_num
+Set a watchpoint that will break when @var{expr} is either read from
+or written into by the thread identified by @var{thread_num}. If @var{expr}
+is modified by any other threads not matching @var{thread_num}, @value{GDBN}
+will not break. Note that this will only work with Hardware Watchpoints.
+
 @quotation
 @emph{Warning:} In multi-threaded programs, software watchpoints
 have only limited usefulness.  If @value{GDBN} creates a software

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Andreas Schwab]

Luis Machado <luisgpm@linux.vnet.ibm.com> writes:

> +  /* Make sure that we actually have parameters to parse.  */
> +  if (arg != NULL && strlen (arg) >= 1)
     if (arg != NULL && arg[0] != '\0')

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Eli Zaretskii]

> From: Luis Machado <luisgpm@linux.vnet.ibm.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sourceware.org
> Date: Tue, 13 Nov 2007 11:50:11 -0200
> 
> Here is the patch together with the testcase and the documentation bits.

Thanks!

> Please let me know if there are any improvements/corrections suitable
> for this one.

The documentation part is okay with me, provided that you fix the
following two small gotchas:

> +@item watch @var{expr} thread thread_num

thread_num should be in @var here.  Also, I'd suggest to use threadnum
or just num, to avoid the underscore (that doesn't go well together
with the @var markup).

> +Set a watchpoint that will break when @var{expr} is either read from
> +or written into by the thread identified by @var{thread_num}. If @var{expr}
                                                               ^^
> +is modified by any other threads not matching @var{thread_num}, @value{GDBN}
> +will not break. Note that this will only work with Hardware Watchpoints.
                 ^^
Two spaces after a period that ends a sentence, please.

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Luis Machado]

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

Thanks!

> The documentation part is okay with me, provided that you fix the
> following two small gotchas:

Got those fixed and changed thread_num to threadnum.

Regards,
Luis

[-- Attachment #2: watch_thread_param.diff --]
[-- Type: text/x-patch, Size: 9400 bytes --]

2007-11-14  Luis Machado  <luisgpm@br.ibm.com>

      * breakpoint.c: (watch_command_1): Parse additional optional 
      "thread" parameter to the watchpoint command and set the 
      "thread" member of the breakpoint struct.
      * doc/gdb.texinfo: Add new parameter's description.
      * testsuite/gdb.base/watch_thread_num.c: New testcase source file.
      * testsuite/gdb.base/watch_thread_num.exp: New testcase expect file.

Index: gdb/breakpoint.c
===================================================================
--- gdb.orig/breakpoint.c	2007-11-12 11:22:15.000000000 -0800
+++ gdb/breakpoint.c	2007-11-13 07:57:36.000000000 -0800
@@ -5882,7 +5882,7 @@
   struct frame_info *prev_frame = NULL;
   char *exp_start = NULL;
   char *exp_end = NULL;
-  char *tok, *end_tok;
+  char *tok, *id_tok_start, *end_tok;
   int toklen;
   char *cond_start = NULL;
   char *cond_end = NULL;
@@ -5890,10 +5890,72 @@
   int i, other_type_used, target_resources_ok = 0;
   enum bptype bp_type;
   int mem_cnt = 0;
+  int thread = -1;
 
   init_sal (&sal);		/* initialize to zeroes */
 
-  /* Parse arguments.  */
+  /* Make sure that we actually have parameters to parse.  */
+  if (arg != NULL && arg[0] != '\0')
+    {
+      toklen = strlen (arg); /* Size of argument list.  */
+
+      /* Points tok to the end of the argument list.  */
+      tok = arg + toklen - 1;
+
+      /* Go backwards in the parameters list. Skip the last parameter.
+         If we're expecting a 'thread <thread_num>' parameter, this should
+         be the thread identifier.  */
+      while (tok > arg && (*tok == ' ' || *tok == '\t'))
+        tok--;
+      while (tok > arg && (*tok != ' ' && *tok != '\t'))
+        tok--;
+
+      /* Points end_tok to the beginning of the last token.  */
+      id_tok_start = tok + 1;
+
+      /* Go backwards in the parameters list. Skip one more parameter.
+         If we're expecting a 'thread <thread_num>' parameter, we should
+         reach a "thread" token.  */
+      while (tok > arg && (*tok == ' ' || *tok == '\t'))
+        tok--;
+
+      end_tok = tok;
+
+      while (tok > arg && (*tok != ' ' && *tok != '\t'))
+        tok--;
+
+      /* Move the pointer forward to skip the whitespace and
+         calculate the length of the token.  */
+      tok++;
+      toklen = end_tok - tok;
+
+      if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
+        {
+          /* At this point we've found a "thread" token, which means
+             the user is trying to set a watchpoint that triggers
+             only in a specific thread.  */
+          char *endp;
+
+          /* Extract the thread ID from the next token.  */
+          thread = strtol (id_tok_start, &endp, 0);
+
+          /* Check if the user provided a valid numeric value for the
+             thread ID.  */
+          if (*endp != ' ' && *endp != '\t' && *endp != '\0')
+            error (_("Invalid thread ID specification %s."), id_tok_start);
+
+          /* Check if the thread actually exists.  */
+          if (!valid_thread_id (thread))
+            error (_("Unknown thread %d."), thread);
+
+          /* Truncate the string and get rid of the thread <thread_num>
+             parameter before the parameter list is parsed by the
+             evaluate_expression() function.  */
+          *tok = '\0';
+        }
+    }
+
+  /* Parse the rest of the arguments.  */
   innermost_block = NULL;
   exp_start = arg;
   exp = parse_exp_1 (&arg, 0, 0);
@@ -5986,6 +6048,7 @@
   b = set_raw_breakpoint (sal, bp_type);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
+  b->thread = thread;
   b->disposition = disp_donttouch;
   b->exp = exp;
   b->exp_valid_block = exp_valid_block;
Index: gdb/testsuite/gdb.base/watch_thread_num.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb/testsuite/gdb.base/watch_thread_num.c	2007-11-13 06:35:30.000000000 -0800
@@ -0,0 +1,63 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2002, 2003, 2004 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.
+
+   This file is copied from schedlock.c.  */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+
+void *thread_function (void *arg); /* Pointer to function executed by each thread */
+
+#define NUM 5
+
+static unsigned int shared_var = 1;
+
+int main () {
+    int res;
+    pthread_t threads[NUM];
+    void *thread_result;
+    long i;
+
+    for (i = 0; i < NUM; i++)
+      {
+        res = pthread_create (&threads[i],
+                             NULL,
+                             thread_function,
+			     (void *) i);
+      }
+
+    thread_result = thread_function ((void *) i);
+
+    exit (EXIT_SUCCESS);
+}
+
+void *thread_function (void *arg) {
+    int my_number = (long) arg;
+    /* Don't run forever.  Run just short of it :)  */
+    while (shared_var > 0)
+      {
+        shared_var++;
+	usleep (1); /* Loop increment.  */
+      }
+
+    pthread_exit (NULL);
+}
+
Index: gdb/testsuite/gdb.base/watch_thread_num.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb/testsuite/gdb.base/watch_thread_num.exp	2007-11-12 11:26:37.000000000 -0800
@@ -0,0 +1,67 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2007
+# Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+# watch-thread_num.exp   Test thread <thread_num> parameter for
+#                        watch commands.
+#
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+set testfile watch_thread_num
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+# What compiler are we using?
+#
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug libs=-lpthread}] != "" } {
+     untested watch_thread_num.exp
+     return -1
+}
+
+# use this to debug:
+#log_user 1
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto main] } then {
+   fail "run to main"
+   return
+}
+
+gdb_test "watch shared_var thread 0" "Unknown thread 0\." "Watchpoint on invalid thread"
+gdb_test "watch shared_var thread" "A syntax error in expression, near `thread'\." "Invalid watch syntax"
+
+gdb_test "Next 5" ""
+
+gdb_test "watch shared_var thread 2" "Hardware watchpoint 2: shared_var" "Watchpoint on shared variable"
+gdb_test "info breakpoint 2" "stop only in thread 2"
+
+for {set i 0} {$i < 10} {incr i 1} {
+gdb_test "continue" "Hardware watchpoint 2: shared_var.*" "Watchpoint triggered"
+gdb_test "thread" "\\\[Current thread is 2 \\\(Thread $hex \\\(LWP $decimal\\\)\\\)\\\]" "Check thread that triggered"
+}
+
Index: gdb/doc/gdb.texinfo
===================================================================
--- gdb.orig/doc/gdb.texinfo	2007-11-12 11:22:15.000000000 -0800
+++ gdb/doc/gdb.texinfo	2007-11-14 04:11:06.000000000 -0800
@@ -3210,7 +3210,7 @@
 catch errors where you have no clue what part of your program is the
 culprit.)
 
-On some systems, such as HP-UX, @sc{gnu}/Linux and most other
+On some systems, such as HP-UX, PowerPC, @sc{gnu}/Linux and most other
 x86-based targets, @value{GDBN} includes support for hardware
 watchpoints, which do not slow down the running of your program.
 
@@ -3351,6 +3351,13 @@
 In multi-threaded programs, watchpoints will detect changes to the
 watched expression from every thread.
 
+@kindex watch thread thread_num
+@item watch @var{expr} thread @var{threadnum}
+Set a watchpoint that will break when @var{expr} is either read from
+or written into by the thread identified by @var{threadnum}.  If @var{expr}
+is modified by any other threads not matching @var{threadnum}, @value{GDBN}
+will not break.  Note that this will only work with Hardware Watchpoints.
+
 @quotation
 @emph{Warning:} In multi-threaded programs, software watchpoints
 have only limited usefulness.  If @value{GDBN} creates a software

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Luis Machado]

Hi guys,

If there isn't anything against this patch, i presume it's OK to commit
it? I addressed the last couple documentation issues catched by Eli.

Thanks,
Luis

On Wed, 2007-11-14 at 10:20 -0200, Luis Machado wrote:
> Thanks!
> 
> > The documentation part is okay with me, provided that you fix the
> > following two small gotchas:
> 
> Got those fixed and changed thread_num to threadnum.
> 
> Regards,
> Luis

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Daniel Jacobowitz]

On Wed, Nov 14, 2007 at 10:20:01AM -0200, Luis Machado wrote:
> 2007-11-14  Luis Machado  <luisgpm@br.ibm.com>
> 
>       * breakpoint.c: (watch_command_1): Parse additional optional 
>       "thread" parameter to the watchpoint command and set the 
>       "thread" member of the breakpoint struct.
>       * doc/gdb.texinfo: Add new parameter's description.
>       * testsuite/gdb.base/watch_thread_num.c: New testcase source file.
>       * testsuite/gdb.base/watch_thread_num.exp: New testcase expect file.

Remember to put the doc/ and testsuite/ bits in their respective
changelogs.  This patch is OK.

> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2002, 2003, 2004 Free Software Foundation, Inc.

Add 2007 here.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Watchpoints: support for thread <thread_num> parameters

[Luis Machado]

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

On Sun, 2007-12-16 at 16:48 -0500, Daniel Jacobowitz wrote:
> On Wed, Nov 14, 2007 at 10:20:01AM -0200, Luis Machado wrote:
> > 2007-11-14  Luis Machado  <luisgpm@br.ibm.com>
> > 
> >       * breakpoint.c: (watch_command_1): Parse additional optional 
> >       "thread" parameter to the watchpoint command and set the 
> >       "thread" member of the breakpoint struct.
> >       * doc/gdb.texinfo: Add new parameter's description.
> >       * testsuite/gdb.base/watch_thread_num.c: New testcase source file.
> >       * testsuite/gdb.base/watch_thread_num.exp: New testcase expect file.
> 
> Remember to put the doc/ and testsuite/ bits in their respective
> changelogs.  This patch is OK.
> 
> > +/* This testcase is part of GDB, the GNU debugger.
> > +
> > +   Copyright 2002, 2003, 2004 Free Software Foundation, Inc.
> 
> Add 2007 here.
> 

Thanks for reviewing this. I've checked in the updated version.

Regards,
-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center

[-- Attachment #2: watch_thread_param.diff --]
[-- Type: text/x-patch, Size: 9368 bytes --]

2007-12-17  Luis Machado  <luisgpm@br.ibm.com>

      * breakpoint.c: (watch_command_1): Parse additional optional
      "thread" parameter to the watchpoint command and set the 
      "thread" member of the breakpoint struct.
      * doc/gdb.texinfo: Add new parameter's description.
      * testsuite/gdb.base/watch_thread_num.c: New testcase source file.
      * testsuite/gdb.base/watch_thread_num.exp: New testcase expect file.

Index: gdb/breakpoint.c
===================================================================
--- gdb.orig/breakpoint.c	2007-12-17 03:18:56.000000000 -0800
+++ gdb/breakpoint.c	2007-12-17 03:41:12.000000000 -0800
@@ -5606,7 +5606,7 @@
   struct frame_info *prev_frame = NULL;
   char *exp_start = NULL;
   char *exp_end = NULL;
-  char *tok, *end_tok;
+  char *tok, *id_tok_start, *end_tok;
   int toklen;
   char *cond_start = NULL;
   char *cond_end = NULL;
@@ -5614,10 +5614,72 @@
   int i, other_type_used, target_resources_ok = 0;
   enum bptype bp_type;
   int mem_cnt = 0;
+  int thread = -1;
 
   init_sal (&sal);		/* initialize to zeroes */
 
-  /* Parse arguments.  */
+  /* Make sure that we actually have parameters to parse.  */
+  if (arg != NULL && arg[0] != '\0')
+    {
+      toklen = strlen (arg); /* Size of argument list.  */
+
+      /* Points tok to the end of the argument list.  */
+      tok = arg + toklen - 1;
+
+      /* Go backwards in the parameters list. Skip the last parameter.
+         If we're expecting a 'thread <thread_num>' parameter, this should
+         be the thread identifier.  */
+      while (tok > arg && (*tok == ' ' || *tok == '\t'))
+        tok--;
+      while (tok > arg && (*tok != ' ' && *tok != '\t'))
+        tok--;
+
+      /* Points end_tok to the beginning of the last token.  */
+      id_tok_start = tok + 1;
+
+      /* Go backwards in the parameters list. Skip one more parameter.
+         If we're expecting a 'thread <thread_num>' parameter, we should
+         reach a "thread" token.  */
+      while (tok > arg && (*tok == ' ' || *tok == '\t'))
+        tok--;
+
+      end_tok = tok;
+
+      while (tok > arg && (*tok != ' ' && *tok != '\t'))
+        tok--;
+
+      /* Move the pointer forward to skip the whitespace and
+         calculate the length of the token.  */
+      tok++;
+      toklen = end_tok - tok;
+
+      if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0)
+        {
+          /* At this point we've found a "thread" token, which means
+             the user is trying to set a watchpoint that triggers
+             only in a specific thread.  */
+          char *endp;
+
+          /* Extract the thread ID from the next token.  */
+          thread = strtol (id_tok_start, &endp, 0);
+
+          /* Check if the user provided a valid numeric value for the
+             thread ID.  */
+          if (*endp != ' ' && *endp != '\t' && *endp != '\0')
+            error (_("Invalid thread ID specification %s."), id_tok_start);
+
+          /* Check if the thread actually exists.  */
+          if (!valid_thread_id (thread))
+            error (_("Unknown thread %d."), thread);
+
+          /* Truncate the string and get rid of the thread <thread_num>
+             parameter before the parameter list is parsed by the
+             evaluate_expression() function.  */
+          *tok = '\0';
+        }
+    }
+
+  /* Parse the rest of the arguments.  */
   innermost_block = NULL;
   exp_start = arg;
   exp = parse_exp_1 (&arg, 0, 0);
@@ -5710,6 +5772,7 @@
   b = set_raw_breakpoint (sal, bp_type);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
+  b->thread = thread;
   b->disposition = disp_donttouch;
   b->exp = exp;
   b->exp_valid_block = exp_valid_block;
Index: gdb/testsuite/gdb.base/watch_thread_num.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb/testsuite/gdb.base/watch_thread_num.c	2007-12-17 03:41:12.000000000 -0800
@@ -0,0 +1,63 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2002, 2003, 2004, 2007 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.
+
+   This file is copied from schedlock.c.  */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+
+void *thread_function (void *arg); /* Pointer to function executed by each thread */
+
+#define NUM 5
+
+static unsigned int shared_var = 1;
+
+int main () {
+    int res;
+    pthread_t threads[NUM];
+    void *thread_result;
+    long i;
+
+    for (i = 0; i < NUM; i++)
+      {
+        res = pthread_create (&threads[i],
+                             NULL,
+                             thread_function,
+			     (void *) i);
+      }
+
+    thread_result = thread_function ((void *) i);
+
+    exit (EXIT_SUCCESS);
+}
+
+void *thread_function (void *arg) {
+    int my_number = (long) arg;
+    /* Don't run forever.  Run just short of it :)  */
+    while (shared_var > 0)
+      {
+        shared_var++;
+	usleep (1); /* Loop increment.  */
+      }
+
+    pthread_exit (NULL);
+}
+
Index: gdb/testsuite/gdb.base/watch_thread_num.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb/testsuite/gdb.base/watch_thread_num.exp	2007-12-17 03:41:12.000000000 -0800
@@ -0,0 +1,64 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2007
+# Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+# watch-thread_num.exp   Test thread <thread_num> parameter for
+#                        watch commands.
+#
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+set testfile watch_thread_num
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+# What compiler are we using?
+#
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug libs=-lpthread}] != "" } {
+     untested watch_thread_num.exp
+     return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto main] } then {
+   fail "run to main"
+   return
+}
+
+gdb_test "watch shared_var thread 0" "Unknown thread 0\." "Watchpoint on invalid thread"
+gdb_test "watch shared_var thread" "A syntax error in expression, near `thread'\." "Invalid watch syntax"
+
+gdb_test "Next 5" ""
+
+gdb_test "watch shared_var thread 2" "Hardware watchpoint 2: shared_var" "Watchpoint on shared variable"
+gdb_test "info breakpoint 2" "stop only in thread 2"
+
+for {set i 0} {$i < 10} {incr i 1} {
+gdb_test "continue" "Hardware watchpoint 2: shared_var.*" "Watchpoint triggered"
+gdb_test "thread" "\\\[Current thread is 2 \\\(Thread $hex \\\(LWP $decimal\\\)\\\)\\\]" "Check thread that triggered"
+}
+
Index: gdb/doc/gdb.texinfo
===================================================================
--- gdb.orig/doc/gdb.texinfo	2007-12-17 03:18:27.000000000 -0800
+++ gdb/doc/gdb.texinfo	2007-12-17 03:41:12.000000000 -0800
@@ -3216,7 +3216,7 @@
 catch errors where you have no clue what part of your program is the
 culprit.)
 
-On some systems, such as HP-UX, @sc{gnu}/Linux and most other
+On some systems, such as HP-UX, PowerPC, @sc{gnu}/Linux and most other
 x86-based targets, @value{GDBN} includes support for hardware
 watchpoints, which do not slow down the running of your program.
 
@@ -3357,6 +3357,13 @@
 In multi-threaded programs, watchpoints will detect changes to the
 watched expression from every thread.
 
+@kindex watch thread thread_num
+@item watch @var{expr} thread @var{threadnum}
+Set a watchpoint that will break when @var{expr} is either read from
+or written into by the thread identified by @var{threadnum}.  If @var{expr}
+is modified by any other threads not matching @var{threadnum}, @value{GDBN}
+will not break.  Note that this will only work with Hardware Watchpoints.
+
 @quotation
 @emph{Warning:} In multi-threaded programs, software watchpoints
 have only limited usefulness.  If @value{GDBN} creates a software

[RFC/RFA] testsuite/gdb.base/watch_thread_num.exp: Fix test for systems having hidden threads

[Pierre Muller]

This test currently fails on cygwin target
and it does by timeout's which means that it takes a lot of time...
The reason of the failure is that 
thread #2 is a thread created internally by cygwin,
for posix emulation, and thus never
changes the value of the variable shared_var.
  
  I modified the test so that it does
not suppose that thread 2 is one of the explicitly generated
threads, but instead adds a breakpoint to
thread_function, and reads the thread number of the first
explicitly generated thread.
  I also added an iteration number to the final test loop.

  I am mainly unsure about the details
required for the ChangeLog entry...

  I only tested the patch on Cygwin target where it gives
29 passes, but I expect it to give the same results
on other targets that had no failures before.

  One possible problem is about the $expect_out(1,string)
because this is undefined if the first pattern is not found.
But I suppose that this is not unique to my patch,
as lots of gdb_expect calls only have braces
to define a group that will become the $expect_out return values
in only one regular expression.

  Is there a coding rule for this?

Pierre Muller


testsuite directory ChangeLog entry

2007-12-19  Pierre Muller  <muller@ics-u-strasbg.fr>

	* (gdb.base/watch_thread_num.exp): Add breakpoint at
	thread_function and record first explicitly generated
	thread number. 
	Use that thread number for thread specific watchpoint test.
	Add iteration number to repetitive tests.

$ cat threadwatch.patch
Index: gdb/testsuite/gdb.base/watch_thread_num.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/watch_thread_num.exp,v
retrieving revision 1.1
diff -u -p -r1.1 watch_thread_num.exp
--- gdb/testsuite/gdb.base/watch_thread_num.exp 17 Dec 2007 12:32:23 -00001.1
+++ gdb/testsuite/gdb.base/watch_thread_num.exp 19 Dec 2007 08:45:41 -0000
@@ -54,11 +54,28 @@ gdb_test "watch shared_var thread" "A sy

 gdb_test "Next 5" ""

-gdb_test "watch shared_var thread 2" "Hardware watchpoint 2: shared_var" "Watch
point on shared variable"
-gdb_test "info breakpoint 2" "stop only in thread 2"
+gdb_test "break thread_function" "Breakpoint \[0-9\].*" \
+  "Set breakpoint at thread_function"

-for {set i 0} {$i < 10} {incr i 1} {
-gdb_test "continue" "Hardware watchpoint 2: shared_var.*" "Watchpoint triggered
"
-gdb_test "thread" "\\\[Current thread is 2 \\\(Thread $hex \\\(LWP $decimal\\\)
\\\)\\\]" "Check thread that triggered"
+gdb_test "continue" ".*Breakpoint 2.*" "Stopped in thread_function"
+
+send_gdb "thread\n"
+gdb_expect {
+  -re ".*Current thread is (\[0-9\]*).*$gdb_prompt $" { pass "Thread command" }
+  -re ".*$gdb_prompt $" { fail "Thread command" }
+  timeout { fail "(timeout) Thread command" }
+}
+
+set thread_num "$expect_out(1,string)"
+
+gdb_test "disable 2" "" "Disable breakpoint 2"
+gdb_test "watch shared_var thread $thread_num" "Hardware watchpoint 3: shared_v
ar" "Watchpoint on shared variable"
+gdb_test "info breakpoint 3" "stop only in thread $thread_num"
+
+for {set i 1} {$i <= 10} {incr i 1} {
+gdb_test "continue" "Hardware watchpoint 3: shared_var.*" \
+  "Watchpoint triggered iteration $i"
+gdb_test "thread" ".*Current thread is $thread_num .*" \
+  "Check thread that triggered iteration $i"
 }

Re: [RFC/RFA] testsuite/gdb.base/watch_thread_num.exp: Fix test for systems having hidden threads

[Luis Machado]

Hi Pierre,

On Wed, 2007-12-19 at 13:59 +0100, Pierre Muller wrote:
> This test currently fails on cygwin target
> and it does by timeout's which means that it takes a lot of time...
> The reason of the failure is that 
> thread #2 is a thread created internally by cygwin,
> for posix emulation, and thus never
> changes the value of the variable shared_var.

Thanks for pointing this out. I wasn't aware of that fact.
  
> 	* (gdb.base/watch_thread_num.exp): Add breakpoint at
> 	thread_function and record first explicitly generated
> 	thread number. 
> 	Use that thread number for thread specific watchpoint test.
> 	Add iteration number to repetitive tests.

Looks OK. Maybe just "Use thread number for testing" and "Add iteration
number" will do on those two phrases.

> +gdb_test "disable 2" "" "Disable breakpoint 2"

Maybe a comment on why this is being explicitly disabled because of
Cygwin?

The other portions of the patch look OK to me.

The patch has 29 passes on PPC as well.

Regards,

-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center

RE: [RFC/RFA] testsuite/gdb.base/watch_thread_num.exp: Fix test for systems having hidden threads

[Pierre Muller]

> -----Original Message-----
> Hi Pierre,
> 
> On Wed, 2007-12-19 at 13:59 +0100, Pierre Muller wrote:
> > This test currently fails on cygwin target
> > and it does by timeout's which means that it takes a lot of time...
> > The reason of the failure is that
> > thread #2 is a thread created internally by cygwin,
> > for posix emulation, and thus never
> > changes the value of the variable shared_var.
> 
> Thanks for pointing this out. I wasn't aware of that fact.
> 
> > 	* (gdb.base/watch_thread_num.exp): Add breakpoint at
> > 	thread_function and record first explicitly generated
> > 	thread number.
> > 	Use that thread number for thread specific watchpoint test.
> > 	Add iteration number to repetitive tests.
> 
> Looks OK. Maybe just "Use thread number for testing" and "Add iteration
> number" will do on those two phrases.
> > +gdb_test "disable 2" "" "Disable breakpoint 2"
> 
> Maybe a comment on why this is being explicitly disabled because of
> Cygwin?
  Breakpoint 2 is now the breakpoint at thread_function,
which is call for each newly created thread,
but we only care to have one of those threads, that is the reason why
I disabled it after getting one valid thread number.
 
> The other portions of the patch look OK to me.
> 
> The patch has 29 passes on PPC as well.
Great, thanks for testing.

Pierre

Re: [RFC/RFA] testsuite/gdb.base/watch_thread_num.exp: Fix test for systems having hidden threads

['Daniel Jacobowitz']

On Wed, Dec 19, 2007 at 01:59:13PM +0100, Pierre Muller wrote:
> This test currently fails on cygwin target
> and it does by timeout's which means that it takes a lot of time...
> The reason of the failure is that 
> thread #2 is a thread created internally by cygwin,
> for posix emulation, and thus never
> changes the value of the variable shared_var.

Thanks.  I saw a bunch of timeouts in this test too:

(gdb) PASS: gdb.base/watch_thread_num.exp: Invalid watch syntax
Next 5
41              res = pthread_create (&threads[i],
(gdb) PASS: gdb.base/watch_thread_num.exp: Next 5
watch shared_var thread 2
Unknown thread 2.
(gdb) FAIL: gdb.base/watch_thread_num.exp: Watchpoint on shared
variable

They aren't really related, but I think your patch will fix them too.
Patch is OK.

Some other things I noticed with this test:

> --- gdb/testsuite/gdb.base/watch_thread_num.exp 17 Dec 2007 12:32:23 -00001.1
> +++ gdb/testsuite/gdb.base/watch_thread_num.exp 19 Dec 2007 08:45:41 -0000

This is a threads test, it should be in gdb.threads.

> +for {set i 1} {$i <= 10} {incr i 1} {
> +gdb_test "continue" "Hardware watchpoint 3: shared_var.*" \
> +  "Watchpoint triggered iteration $i"

This test won't work on any target without hardware watchpoints.  It
should either handle that (search for can-use-hw-watchpoints) or skip
if the test is fundamentally broken without hardware watchpoints.
Probably the latter.  See watchthreads.exp.

-- 
Daniel Jacobowitz
CodeSourcery

RE: [RFC/RFA] testsuite/gdb.base/watch_thread_num.exp: Fix test for systems having hidden threads

[Pierre Muller]

> -----Original Message-----
> From: 'Daniel Jacobowitz' [mailto:drow@false.org]
> Sent: Wednesday, December 19, 2007 3:04 PM
> To: Pierre Muller
> Cc: luisgpm@linux.vnet.ibm.com; gdb-patches@sourceware.org
> Subject: Re: [RFC/RFA] testsuite/gdb.base/watch_thread_num.exp: Fix
> test for systems having hidden threads
> 
> On Wed, Dec 19, 2007 at 01:59:13PM +0100, Pierre Muller wrote:
> > This test currently fails on cygwin target
> > and it does by timeout's which means that it takes a lot of time...
> > The reason of the failure is that
> > thread #2 is a thread created internally by cygwin,
> > for posix emulation, and thus never
> > changes the value of the variable shared_var.
> 
> Thanks.  I saw a bunch of timeouts in this test too:
> 
> (gdb) PASS: gdb.base/watch_thread_num.exp: Invalid watch syntax
> Next 5
> 41              res = pthread_create (&threads[i],
> (gdb) PASS: gdb.base/watch_thread_num.exp: Next 5
> watch shared_var thread 2
> Unknown thread 2.
> (gdb) FAIL: gdb.base/watch_thread_num.exp: Watchpoint on shared
> variable
> 
> They aren't really related, but I think your patch will fix them too.
> Patch is OK.

  Thanks, I committed my change in the same directory.

 
> Some other things I noticed with this test:
> 
> > --- gdb/testsuite/gdb.base/watch_thread_num.exp 17 Dec 2007 12:32:23
> -00001.1
> > +++ gdb/testsuite/gdb.base/watch_thread_num.exp 19 Dec 2007 08:45:41
> -0000
> 
> This is a threads test, it should be in gdb.threads.

  It should probably also use gdb_compile_pthreads
to try to compile the source.
 
> > +for {set i 1} {$i <= 10} {incr i 1} {
> > +gdb_test "continue" "Hardware watchpoint 3: shared_var.*" \
> > +  "Watchpoint triggered iteration $i"
> 
> This test won't work on any target without hardware watchpoints.  It
> should either handle that (search for can-use-hw-watchpoints) or skip
> if the test is fundamentally broken without hardware watchpoints.
> Probably the latter.  See watchthreads.exp.

  I found this, in gdb.base/watchpoint.exp:

    # Disable hardware watchpoints if necessary.
    if [target_info exists gdb,no_hardware_watchpoints] {
        gdb_test "set can-use-hw-watchpoints 0" "" ""
    }

but I still didn't really understand what those "target_info exists"
commands really are:
  - how are they set?
  - does it make it possible to run the testsuite with hardware watchpoints
disabled?

  gdb.threads/watchthreads.exp contains this:

# This test verifies that a watchpoint is detected in the proper thread
# so the test is only meaningful on a system with hardware watchpoints.
if [target_info exists gdb,no_hardware_watchpoints] {
    return 0;
}

But where and how is gdb,no_hardware_watchpoints set?

Pierre Muller

Re: [RFC/RFA] testsuite/gdb.base/watch_thread_num.exp: Fix test for systems having hidden threads

['Daniel Jacobowitz']

On Wed, Dec 19, 2007 at 03:51:14PM +0100, Pierre Muller wrote:
>   It should probably also use gdb_compile_pthreads
> to try to compile the source.

That's right.

>   I found this, in gdb.base/watchpoint.exp:
> 
>     # Disable hardware watchpoints if necessary.
>     if [target_info exists gdb,no_hardware_watchpoints] {
>         gdb_test "set can-use-hw-watchpoints 0" "" ""
>     }
> 
> but I still didn't really understand what those "target_info exists"
> commands really are:
>   - how are they set?

By the DejaGNU "board" file.  Normally you just use unix.exp if you
say "runtest"; other board files are used for cross testing or for
custom configurations.

>   - does it make it possible to run the testsuite with hardware watchpoints
> disabled?

Yes, but you need to use a board file.

>   gdb.threads/watchthreads.exp contains this:
> 
> # This test verifies that a watchpoint is detected in the proper thread
> # so the test is only meaningful on a system with hardware watchpoints.
> if [target_info exists gdb,no_hardware_watchpoints] {
>     return 0;
> }
> 
> But where and how is gdb,no_hardware_watchpoints set?

Same thing; in a board file.  For instance, the gdbserver example on
the wiki.

-- 
Daniel Jacobowitz
CodeSourcery