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

[Luis Machado <luisgpm@linux.vnet.ibm.com>]

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