Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [commit] fix for "info threads" printing multiple headers
@ 2011-02-22  1:57 Michael Snyder
  2011-02-22  9:17 ` Pedro Alves
  2011-02-22 17:37 ` Tom Tromey
  0 siblings, 2 replies; 13+ messages in thread
From: Michael Snyder @ 2011-02-22  1:57 UTC (permalink / raw)
  To: gdb-patches

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

This fixes the issue that Pedro raised.


[-- Attachment #2: infothread.txt --]
[-- Type: text/plain, Size: 10040 bytes --]

2011-02-21  Michael Snyder  <msnyder@vmware.com>

	* gdbthread.h (print_thread_info): Change prototype.
	* thread.c (print_thread_info): Accept char* instead of int for
	requested_threads argument.  Use new function number_is_in_list
	to determine which threads to list.
	(info_threads_command): Pass char* to print_thread_info.
	* cli/cli-utils.c (number_is_in_list): New function.
	* cli/cli-utils.h (number_is_in_list): Export.
	* mi/mi-main.c (mi_cmd_thread_info): Pass char* to 
	print_thread_info.
	(print_one_inferior): Ditto.
	(mi_cmd_list_thread_groups): Ditto.

2011-02-21  Michael Snyder  <msnyder@vmware.com>

	* gdb.threads/thread-find.exp: Update patterns for changes in
	output of "info threads" command.

Index: gdbthread.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbthread.h,v
retrieving revision 1.62
diff -u -p -u -p -r1.62 gdbthread.h
--- gdbthread.h	19 Jan 2011 17:21:36 -0000	1.62
+++ gdbthread.h	21 Feb 2011 23:30:40 -0000
@@ -377,7 +377,7 @@ extern struct cmd_list_element *thread_c
    `set print thread-events'.  */
 extern int print_thread_events;
 
-extern void print_thread_info (struct ui_out *uiout, int thread,
+extern void print_thread_info (struct ui_out *uiout, char *threads,
 			       int pid);
 
 extern struct cleanup *make_cleanup_restore_current_thread (void);
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.135
diff -u -p -u -p -r1.135 thread.c
--- thread.c	19 Feb 2011 01:24:55 -0000	1.135
+++ thread.c	21 Feb 2011 23:30:40 -0000
@@ -44,6 +44,7 @@
 #include "annotate.h"
 #include "cli/cli-decode.h"
 #include "gdb_regex.h"
+#include "cli/cli-utils.h"
 
 /* Definition of struct thread_info exported to gdbthread.h.  */
 
@@ -755,7 +756,7 @@ finish_thread_state_cleanup (void *arg)
 }
 
 /* Prints the list of threads and their details on UIOUT.
-   This is a version of 'info_thread_command' suitable for
+   This is a version of 'info_threads_command' suitable for
    use from MI.
    If REQUESTED_THREAD is not -1, it's the GDB id of the thread
    that should be printed.  Otherwise, all threads are
@@ -766,7 +767,7 @@ finish_thread_state_cleanup (void *arg)
    is printed if it belongs to the specified process.  Otherwise,
    an error is raised.  */
 void
-print_thread_info (struct ui_out *uiout, int requested_thread, int pid)
+print_thread_info (struct ui_out *uiout, char *requested_threads, int pid)
 {
   struct thread_info *tp;
   ptid_t current_ptid;
@@ -791,7 +792,7 @@ print_thread_info (struct ui_out *uiout,
 
       for (tp = thread_list; tp; tp = tp->next)
 	{
-	  if (requested_thread != -1 && tp->num != requested_thread)
+	  if (!number_is_in_list (requested_threads, tp->num))
 	    continue;
 
 	  if (pid != -1 && PIDGET (tp->ptid) != pid)
@@ -805,10 +806,11 @@ print_thread_info (struct ui_out *uiout,
 
       if (n_threads == 0)
 	{
-	  if (requested_thread == -1)
+	  if (requested_threads == NULL || *requested_threads == '\0')
 	    ui_out_message (uiout, 0, _("No threads.\n"));
 	  else
-	    ui_out_message (uiout, 0, _("No thread %d.\n"), requested_thread);
+	    ui_out_message (uiout, 0, _("No threads match '%s'.\n"),
+			    requested_threads);
 	  do_cleanups (old_chain);
 	  return;
 	}
@@ -827,12 +829,12 @@ print_thread_info (struct ui_out *uiout,
       struct cleanup *chain2;
       int core;
 
-      if (requested_thread != -1 && tp->num != requested_thread)
+      if (!number_is_in_list (requested_threads, tp->num))
 	continue;
 
       if (pid != -1 && PIDGET (tp->ptid) != pid)
 	{
-	  if (requested_thread != -1)
+	  if (requested_threads != NULL && *requested_threads != '\0')
 	    error (_("Requested thread not found in requested process"));
 	  continue;
 	}
@@ -935,7 +937,7 @@ print_thread_info (struct ui_out *uiout,
      the "info threads" command.  */
   do_cleanups (old_chain);
 
-  if (pid == -1 && requested_thread == -1)
+  if (pid == -1 && requested_threads == NULL)
     {
       gdb_assert (current_thread != -1
 		  || !thread_list
@@ -966,23 +968,7 @@ No selected thread.  See `help thread'.\
 static void
 info_threads_command (char *arg, int from_tty)
 {
-  int tid = -1;
-
-  if (arg == NULL || *arg == '\0')
-    {
-      print_thread_info (uiout, -1, -1);
-      return;
-    }
-
-  while (arg != NULL && *arg != '\0')
-    {
-      tid = get_number_or_range (&arg);
-
-      if (tid <= 0)
-	error (_("invalid thread id %d"), tid);
-
-      print_thread_info (uiout, tid, -1);
-    }
+  print_thread_info (uiout, arg, -1);
 }
 
 /* Switch from one thread to another.  */
Index: cli/cli-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-utils.c,v
retrieving revision 1.8
diff -u -p -u -p -r1.8 cli-utils.c
--- cli/cli-utils.c	21 Feb 2011 18:13:17 -0000	1.8
+++ cli/cli-utils.c	21 Feb 2011 23:30:40 -0000
@@ -161,6 +161,27 @@ get_number_or_range (char **pp)
   return last_retval;
 }
 
+/* Accept a number and a string-form list of numbers such as is 
+   accepted by get_number_or_range.  Return TRUE if the number is
+   in the list.
+
+   By definition, an empty list includes all numbers.  This is to 
+   be interpreted as typing a command such as "delete break" with 
+   no arguments.  */
+
+int
+number_is_in_list (char *list, int number)
+{
+  if (list == NULL || *list == '\0')
+    return 1;
+
+  while (list != NULL && *list != '\0')
+    if (get_number_or_range (&list) == number)
+      return 1;
+
+  return 0;
+}
+
 /* See documentation in cli-utils.h.  */
 
 char *
Index: cli/cli-utils.h
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-utils.h,v
retrieving revision 1.9
diff -u -p -u -p -r1.9 cli-utils.h
--- cli/cli-utils.h	21 Feb 2011 18:13:17 -0000	1.9
+++ cli/cli-utils.h	21 Feb 2011 23:30:40 -0000
@@ -45,6 +45,16 @@ extern int get_number (char **);
 
 extern int get_number_or_range (char **);
 
+/* Accept a number and a string-form list of numbers such as is 
+   accepted by get_number_or_range.  Return TRUE if the number is
+   in the list.
+
+   By definition, an empty list includes all numbers.  This is to 
+   be interpreted as typing a command such as "delete break" with 
+   no arguments.  */
+
+extern int number_is_in_list (char *list, int number);
+
 /* Skip leading whitespace characters in INP, returning an updated
    pointer.  If INP is NULL, return NULL.  */
 
Index: mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.192
diff -u -p -u -p -r1.192 mi-main.c
--- mi/mi-main.c	25 Jan 2011 17:00:27 -0000	1.192
+++ mi/mi-main.c	21 Feb 2011 23:30:40 -0000
@@ -503,15 +503,10 @@ mi_cmd_thread_list_ids (char *command, c
 void
 mi_cmd_thread_info (char *command, char **argv, int argc)
 {
-  int thread = -1;
-  
   if (argc != 0 && argc != 1)
     error ("Invalid MI command");
 
-  if (argc == 1)
-    thread = atoi (argv[0]);
-
-  print_thread_info (uiout, thread, -1);
+  print_thread_info (uiout, argv[0], -1);
 }
 
 struct collect_cores_data
@@ -607,7 +602,7 @@ print_one_inferior (struct inferior *inf
 	}
 
       if (top_data->recurse)
-	print_thread_info (uiout, -1, inferior->pid);
+	print_thread_info (uiout, NULL, inferior->pid);
 
       do_cleanups (back_to);
     }
@@ -872,7 +867,7 @@ mi_cmd_list_thread_groups (char *command
       if (!inf)
 	error ("Non-existent thread group id '%d'", id);
       
-      print_thread_info (uiout, -1, inf->pid);
+      print_thread_info (uiout, NULL, inf->pid);
     }
   else
     {
Index: testsuite/gdb.threads/thread-find.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/thread-find.exp,v
retrieving revision 1.2
diff -u -p -u -p -r1.2 thread-find.exp
--- testsuite/gdb.threads/thread-find.exp	17 Feb 2011 22:08:12 -0000	1.2
+++ testsuite/gdb.threads/thread-find.exp	21 Feb 2011 23:30:40 -0000
@@ -303,28 +303,28 @@ set see5 0
 set see6 0
 
 gdb_test_multiple "info threads 2 4 6" "info threads 2 4 6" {
-    -re ". 1 \[^\r\n\]*\"threadname_1\" \[^\r\n\]*" {
-	set see1 1
-	exp_continue
-    }
-    -re ". 2 \[^\r\n\]*\"threadname_2\" \[^\r\n\]*" {
-	set see2 1
+    -re ". 6 \[^\r\n\]*\"threadname_6\" \[^\r\n\]*" {
+	set see6 1
 	exp_continue
     }
-    -re ". 3 \[^\r\n\]*\"threadname_3\" \[^\r\n\]*" {
-	set see3 1
+    -re ". 5 \[^\r\n\]*\"threadname_5\" \[^\r\n\]*" {
+	set see5 1
 	exp_continue
     }
     -re ". 4 \[^\r\n\]*\"threadname_4\" \[^\r\n\]*" {
 	set see4 1
 	exp_continue
     }
-    -re ". 5 \[^\r\n\]*\"threadname_5\" \[^\r\n\]*" {
-	set see5 1
+    -re ". 3 \[^\r\n\]*\"threadname_3\" \[^\r\n\]*" {
+	set see3 1
 	exp_continue
     }
-    -re ". 6 \[^\r\n\]*\"threadname_6\" \[^\r\n\]*" {
-	set see6 1
+    -re ". 2 \[^\r\n\]*\"threadname_2\" \[^\r\n\]*" {
+	set see2 1
+	exp_continue
+    }
+    -re ". 1 \[^\r\n\]*\"threadname_1\" \[^\r\n\]*" {
+	set see1 1
 	exp_continue
     }
     -re "$gdb_prompt $" {
@@ -348,28 +348,28 @@ set see5 0
 set see6 0
 
 gdb_test_multiple "info threads 3-5" "info threads 3-5" {
-    -re ". 1 .*\"threadname_1\" \[^\r\n\]*" {
-	set see1 1
-	exp_continue
-    }
-    -re ". 2 .*\"threadname_2\" \[^\r\n\]*" {
-	set see2 1
+    -re ". 6 .*\"threadname_6\" \[^\r\n\]*" {
+	set see6 1
 	exp_continue
     }
-    -re ". 3 .*\"threadname_3\" \[^\r\n\]*" {
-	set see3 1
+    -re ". 5 .*\"threadname_5\" \[^\r\n\]*" {
+	set see5 1
 	exp_continue
     }
     -re ". 4 .*\"threadname_4\" \[^\r\n\]*" {
 	set see4 1
 	exp_continue
     }
-    -re ". 5 .*\"threadname_5\" \[^\r\n\]*" {
-	set see5 1
+    -re ". 3 .*\"threadname_3\" \[^\r\n\]*" {
+	set see3 1
 	exp_continue
     }
-    -re ". 6 .*\"threadname_6\" \[^\r\n\]*" {
-	set see6 1
+    -re ". 2 .*\"threadname_2\" \[^\r\n\]*" {
+	set see2 1
+	exp_continue
+    }
+    -re ". 1 .*\"threadname_1\" \[^\r\n\]*" {
+	set see1 1
 	exp_continue
     }
     -re "$gdb_prompt $" {

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22  1:57 [commit] fix for "info threads" printing multiple headers Michael Snyder
@ 2011-02-22  9:17 ` Pedro Alves
  2011-02-22 10:04   ` Pedro Alves
                     ` (2 more replies)
  2011-02-22 17:37 ` Tom Tromey
  1 sibling, 3 replies; 13+ messages in thread
From: Pedro Alves @ 2011-02-22  9:17 UTC (permalink / raw)
  To: gdb-patches; +Cc: Michael Snyder

Thanks!  A few comments below.  If you don't want to
fix these, I'll try to find a bit later on myself.

On Monday 21 February 2011 23:40:06, Michael Snyder wrote:
> +int
> +number_is_in_list (char *list, int number)
> +{
> +  if (list == NULL || *list == '\0')
> +    return 1;
> +
> +  while (list != NULL && *list != '\0')
> +    if (get_number_or_range (&list) == number)
> +      return 1;
> +
> +  return 0;
> +}

- why the 'list != NULL' check in the while loop?
get_number_or_range never puts NULL in the output
argument, and NULL lists are short circuited above.

- get_number_or_range mantains an internal state machine
using static variables.  I think that as long as you
always pass in the same list string, and the list spec
string is correctly formed, you're not hitting stale
state inside get_number_or_range.  It'd be nicer
if get_number_or_range (or a variant which get_number_or_range
would then be implemented on top of) took an additional
struct pointer that pointed to a struct that held
all the currently static state.  This function would then
always pass in a pointer into such a newly initialized
object on the stack.

- get_number_or_range returns '0' on error.  You can
see a bunch of its old callers in breakpoint.c handling
that '0' case specially.  It comes from get_number_trailer:

	  /* Return zero, which caller must interpret as error.  */
	  retval = 0;

I suspect that we'll want to make than an "error" call or
to adjust the interface of these functions to allow
returning an error indication unambiguous with zero
the number, as soon as we want to reuse this function
in places where zero makes sense as valid number...

Examples of not handling the special zero:

(gdb) info threads a 1
  Id   Target Id         Frame 
* 1    Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0
(gdb)

(gdb) info threads a-1
No threads match 'a-1'.

(gdb) info threads a -1
  Id   Target Id         Frame 
* 1    Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0
(gdb) 

The last one only "works" by accident due to the extra space.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22  9:17 ` Pedro Alves
@ 2011-02-22 10:04   ` Pedro Alves
  2011-02-22 17:45   ` Tom Tromey
  2011-02-22 18:31   ` Michael Snyder
  2 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2011-02-22 10:04 UTC (permalink / raw)
  To: gdb-patches; +Cc: Michael Snyder

One think I kept wanting to mention and forgetting, is that the
old version was really ineficient against remote targets,
because for "info threads 1-N", you'd pull the thread
list out of the remote target (update_thread_list) N times!
"set debug remote 1; info threads 1-10000" was fun to watch...

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22  1:57 [commit] fix for "info threads" printing multiple headers Michael Snyder
  2011-02-22  9:17 ` Pedro Alves
@ 2011-02-22 17:37 ` Tom Tromey
  2011-02-22 18:36   ` Michael Snyder
  1 sibling, 1 reply; 13+ messages in thread
From: Tom Tromey @ 2011-02-22 17:37 UTC (permalink / raw)
  To: Michael Snyder; +Cc: gdb-patches

>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:

Michael> -  if (argc == 1)
Michael> -    thread = atoi (argv[0]);
Michael> -
Michael> -  print_thread_info (uiout, thread, -1);
Michael> +  print_thread_info (uiout, argv[0], -1);

This changes the arguments accepted by the MI command.

Maybe it isn't a big deal; but I thought it was at least worth pointing
out.  I'm not sure how much argument checking MI does -- by the looks of
the pre-patch source, not much.

Tom


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22  9:17 ` Pedro Alves
  2011-02-22 10:04   ` Pedro Alves
@ 2011-02-22 17:45   ` Tom Tromey
  2011-02-22 18:37     ` Michael Snyder
  2011-02-22 18:31   ` Michael Snyder
  2 siblings, 1 reply; 13+ messages in thread
From: Tom Tromey @ 2011-02-22 17:45 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches, Michael Snyder

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Thanks!  A few comments below.  If you don't want to
Pedro> fix these, I'll try to find a bit later on myself.

Pedro> - get_number_or_range mantains an internal state machine
Pedro> using static variables.  I think that as long as you
Pedro> always pass in the same list string, and the list spec
Pedro> string is correctly formed, you're not hitting stale
Pedro> state inside get_number_or_range.  It'd be nicer
Pedro> if get_number_or_range (or a variant which get_number_or_range
Pedro> would then be implemented on top of) took an additional
Pedro> struct pointer that pointed to a struct that held
Pedro> all the currently static state.

I almost did this when moving stuff to cli-utils, but decided against it
on the basis of least change.

If Michael doesn't want to implement this, I'd be happy to.
Just let me know.

Tom


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22  9:17 ` Pedro Alves
  2011-02-22 10:04   ` Pedro Alves
  2011-02-22 17:45   ` Tom Tromey
@ 2011-02-22 18:31   ` Michael Snyder
  2011-02-22 18:41     ` Pedro Alves
  2 siblings, 1 reply; 13+ messages in thread
From: Michael Snyder @ 2011-02-22 18:31 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

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

Pedro Alves wrote:
> Thanks!  A few comments below.  If you don't want to
> fix these, I'll try to find a bit later on myself.
> 
> On Monday 21 February 2011 23:40:06, Michael Snyder wrote:
>> +int
>> +number_is_in_list (char *list, int number)
>> +{
>> +  if (list == NULL || *list == '\0')
>> +    return 1;
>> +
>> +  while (list != NULL && *list != '\0')
>> +    if (get_number_or_range (&list) == number)
>> +      return 1;
>> +
>> +  return 0;
>> +}
> 
> - why the 'list != NULL' check in the while loop?
> get_number_or_range never puts NULL in the output
> argument, and NULL lists are short circuited above.

Habit and symmetry.  I'll change it.

> 
> - get_number_or_range mantains an internal state machine
> using static variables.  I think that as long as you
> always pass in the same list string, and the list spec
> string is correctly formed, you're not hitting stale
> state inside get_number_or_range.  It'd be nicer
> if get_number_or_range (or a variant which get_number_or_range
> would then be implemented on top of) took an additional
> struct pointer that pointed to a struct that held
> all the currently static state.  This function would then
> always pass in a pointer into such a newly initialized
> object on the stack.
> 
> - get_number_or_range returns '0' on error.  You can
> see a bunch of its old callers in breakpoint.c handling
> that '0' case specially.  It comes from get_number_trailer:
> 
> 	  /* Return zero, which caller must interpret as error.  */
> 	  retval = 0;
> 
> I suspect that we'll want to make than an "error" call or
> to adjust the interface of these functions to allow
> returning an error indication unambiguous with zero
> the number, as soon as we want to reuse this function
> in places where zero makes sense as valid number...
> 
> Examples of not handling the special zero:
> 
> (gdb) info threads a 1
>   Id   Target Id         Frame 
> * 1    Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0
> (gdb)
> 
> (gdb) info threads a-1
> No threads match 'a-1'.
> 
> (gdb) info threads a -1
>   Id   Target Id         Frame 
> * 1    Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0
> (gdb) 
> 
> The last one only "works" by accident due to the extra space.


OK, test for zero added, how's this?






[-- Attachment #2: zero.txt --]
[-- Type: text/plain, Size: 859 bytes --]

2011-02-22  Michael Snyder  <msnyder@vmware.com>

	* cli/cli-utils.c (number_is_in_list): Check for zero return.

Index: cli/cli-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-utils.c,v
retrieving revision 1.9
diff -u -p -u -p -r1.9 cli-utils.c
--- cli/cli-utils.c	21 Feb 2011 23:40:46 -0000	1.9
+++ cli/cli-utils.c	22 Feb 2011 18:24:22 -0000
@@ -175,10 +175,15 @@ number_is_in_list (char *list, int numbe
   if (list == NULL || *list == '\0')
     return 1;
 
-  while (list != NULL && *list != '\0')
-    if (get_number_or_range (&list) == number)
-      return 1;
+  while (*list != '\0')
+    {
+      int gotnum = get_number_or_range (&list);
 
+      if (gotnum == 0)
+	error (_("Args must be numbers or '$' variables."));
+      if (gotnum == number)
+	return 1;
+    }
   return 0;
 }
 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22 17:37 ` Tom Tromey
@ 2011-02-22 18:36   ` Michael Snyder
  2011-02-22 19:00     ` Tom Tromey
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Snyder @ 2011-02-22 18:36 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

Tom Tromey wrote:
>>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:
> 
> Michael> -  if (argc == 1)
> Michael> -    thread = atoi (argv[0]);
> Michael> -
> Michael> -  print_thread_info (uiout, thread, -1);
> Michael> +  print_thread_info (uiout, argv[0], -1);
> 
> This changes the arguments accepted by the MI command.
> 
> Maybe it isn't a big deal; but I thought it was at least worth pointing
> out.  I'm not sure how much argument checking MI does -- by the looks of
> the pre-patch source, not much.

Hmmm?  My intention was to change only what was passed to 
print_thread_info.  How does it change the syntax of the command?


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22 17:45   ` Tom Tromey
@ 2011-02-22 18:37     ` Michael Snyder
  2011-02-22 18:49       ` Pedro Alves
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Snyder @ 2011-02-22 18:37 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Pedro Alves, gdb-patches

Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> Thanks!  A few comments below.  If you don't want to
> Pedro> fix these, I'll try to find a bit later on myself.
> 
> Pedro> - get_number_or_range mantains an internal state machine
> Pedro> using static variables.  I think that as long as you
> Pedro> always pass in the same list string, and the list spec
> Pedro> string is correctly formed, you're not hitting stale
> Pedro> state inside get_number_or_range.  It'd be nicer
> Pedro> if get_number_or_range (or a variant which get_number_or_range
> Pedro> would then be implemented on top of) took an additional
> Pedro> struct pointer that pointed to a struct that held
> Pedro> all the currently static state.
> 
> I almost did this when moving stuff to cli-utils, but decided against it
> on the basis of least change.
> 
> If Michael doesn't want to implement this, I'd be happy to.
> Just let me know.

Seems like it would only be useful if the function had to be thread safe.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22 18:31   ` Michael Snyder
@ 2011-02-22 18:41     ` Pedro Alves
  2011-02-22 18:55       ` Michael Snyder
  0 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2011-02-22 18:41 UTC (permalink / raw)
  To: gdb-patches; +Cc: Michael Snyder

On Tuesday 22 February 2011 18:25:27, Michael Snyder wrote:
> Pedro Alves wrote:
>
> > Examples of not handling the special zero:
> > 
> > (gdb) info threads a 1
> >   Id   Target Id         Frame 
> > * 1    Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0
> > (gdb)
> > 
> > (gdb) info threads a-1
> > No threads match 'a-1'.
> > 
> > (gdb) info threads a -1
> >   Id   Target Id         Frame 
> > * 1    Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0
> > (gdb) 
> > 
> > The last one only "works" by accident due to the extra space.
> 
> 
> OK, test for zero added, how's this?

Looks good.

I was hoping for turning the above into testcases,
making sure they fail.  Can I trick you into
doing it?  "No" is fine.  :-)

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22 18:37     ` Michael Snyder
@ 2011-02-22 18:49       ` Pedro Alves
  0 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2011-02-22 18:49 UTC (permalink / raw)
  To: gdb-patches; +Cc: Michael Snyder, Tom Tromey

On Tuesday 22 February 2011 18:36:26, Michael Snyder wrote:
> Seems like it would only be useful if the function had to be thread safe.

No, it's useful when something like this happens:

get_number_or_range("1-10", ...);
...
(in_range is left set)
...
QUIT or something like that that breaks the loop.

(new command invocation, with different arg, which happens
to be malformed, for being negative)

get_number_or_range("-1", ...);

Returns 2 instead of throwing an error, which would be
the correct thing to do.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22 18:41     ` Pedro Alves
@ 2011-02-22 18:55       ` Michael Snyder
  0 siblings, 0 replies; 13+ messages in thread
From: Michael Snyder @ 2011-02-22 18:55 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

Pedro Alves wrote:
> On Tuesday 22 February 2011 18:25:27, Michael Snyder wrote:
>> Pedro Alves wrote:
>>
>>> Examples of not handling the special zero:
>>>
>>> (gdb) info threads a 1
>>>   Id   Target Id         Frame 
>>> * 1    Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0
>>> (gdb)
>>>
>>> (gdb) info threads a-1
>>> No threads match 'a-1'.
>>>
>>> (gdb) info threads a -1
>>>   Id   Target Id         Frame 
>>> * 1    Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0
>>> (gdb) 
>>>
>>> The last one only "works" by accident due to the extra space.
>>
>> OK, test for zero added, how's this?
> 
> Looks good.

Committing.

> I was hoping for turning the above into testcases,
> making sure they fail.  Can I trick you into
> doing it?  "No" is fine.  :-)
> 

Consider it done.  ;-)


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22 18:36   ` Michael Snyder
@ 2011-02-22 19:00     ` Tom Tromey
  2011-02-22 20:05       ` Michael Snyder
  0 siblings, 1 reply; 13+ messages in thread
From: Tom Tromey @ 2011-02-22 19:00 UTC (permalink / raw)
  To: Michael Snyder; +Cc: gdb-patches

>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:

Michael> -  if (argc == 1)
Michael> -    thread = atoi (argv[0]);
Michael> -
Michael> -  print_thread_info (uiout, thread, -1);
Michael> +  print_thread_info (uiout, argv[0], -1);

Tom> This changes the arguments accepted by the MI command.

Tom> Maybe it isn't a big deal; but I thought it was at least worth pointing
Tom> out.  I'm not sure how much argument checking MI does -- by the looks of
Tom> the pre-patch source, not much.

Michael> Hmmm?  My intention was to change only what was passed to
Michael> print_thread_info.  How does it change the syntax of the command?

The command is documented to accept a thread number, but now it also
accepts a range.

Tom


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [commit] fix for "info threads" printing multiple headers
  2011-02-22 19:00     ` Tom Tromey
@ 2011-02-22 20:05       ` Michael Snyder
  0 siblings, 0 replies; 13+ messages in thread
From: Michael Snyder @ 2011-02-22 20:05 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

Tom Tromey wrote:
>>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:
> 
> Michael> -  if (argc == 1)
> Michael> -    thread = atoi (argv[0]);
> Michael> -
> Michael> -  print_thread_info (uiout, thread, -1);
> Michael> +  print_thread_info (uiout, argv[0], -1);
> 
> Tom> This changes the arguments accepted by the MI command.
> 
> Tom> Maybe it isn't a big deal; but I thought it was at least worth pointing
> Tom> out.  I'm not sure how much argument checking MI does -- by the looks of
> Tom> the pre-patch source, not much.
> 
> Michael> Hmmm?  My intention was to change only what was passed to
> Michael> print_thread_info.  How does it change the syntax of the command?
> 
> The command is documented to accept a thread number, but now it also
> accepts a range.

Oooohhh, I hadn't thought of that.


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2011-02-22 19:02 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-22  1:57 [commit] fix for "info threads" printing multiple headers Michael Snyder
2011-02-22  9:17 ` Pedro Alves
2011-02-22 10:04   ` Pedro Alves
2011-02-22 17:45   ` Tom Tromey
2011-02-22 18:37     ` Michael Snyder
2011-02-22 18:49       ` Pedro Alves
2011-02-22 18:31   ` Michael Snyder
2011-02-22 18:41     ` Pedro Alves
2011-02-22 18:55       ` Michael Snyder
2011-02-22 17:37 ` Tom Tromey
2011-02-22 18:36   ` Michael Snyder
2011-02-22 19:00     ` Tom Tromey
2011-02-22 20:05       ` Michael Snyder

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox