Re: gdb bug 12417

[Keith Seitz <keiths@redhat.com>]

On 09/18/2012 06:16 AM, Mohsan Saleem wrote:
> Hi,
> Attached patch is for bug 12417, for printing the name of threads while printing the information of a thread.
>

Thank you for you patch submission.  I do not see your name listed in 
gdb/MAINTAINERS -- do you have an assignment? If not, let us know and we 
can help you get started with the paperwork.

In general, I'd say the patch is definitely a feature we should consider 
adding. [Note: I am not a global maintainer.]

> --- /root/Desktop/gdb/thread.c	2012-09-03 00:53:02.620254912 -0700
> +++ ./gdb/thread.c	2012-09-03 00:58:04.399469614 -0700
> @@ -243,12 +243,14 @@
>   struct thread_info *
>   add_thread_with_info (ptid_t ptid, struct private_thread_info *private)
>   {
> +  char *name;
>     struct thread_info *result = add_thread_silent (ptid);
>
> +  name = result->name ? result->name : target_thread_name (result);
>     result->private = private;
>
>     if (print_thread_events)
> -    printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid));
> +    printf_unfiltered (_("[New %s ] %s\n"), target_pid_to_str (ptid), name);

target_thread_name can return NULL. There's an added space typo in "[New 
%s ]".

> @@ -1209,8 +1211,9 @@
>       if (thread_alive (tp))
>         {
>   	switch_to_thread (tp->ptid);
> -	printf_filtered (_("\nThread %d (%s):\n"),
> -			 tp->num, target_pid_to_str (inferior_ptid));
> +	name = tp->name ? tp->name : target_thread_name (tp);
> +	printf_filtered (_("\nThread %d %s (%s):\n"),
> +			 tp->num, name, target_pid_to_str (inferior_ptid));
>   	execute_command (cmd, from_tty);
>   	strcpy (cmd, saved_cmd);	/* Restore exact command used

Likewise.

> @@ -1260,8 +1263,8 @@
>         else
>   	{
>   	  switch_to_thread (tp->ptid);
> -
> -	  printf_filtered (_("\nThread %d (%s):\n"), tp->num,
> +	  name = tp->name ? tp->name : target_thread_name (tp);
> +	  printf_filtered (_("\nThread %d %s (%s):\n"), tp->num, name,
>   			   target_pid_to_str (inferior_ptid));
>   	  execute_command (cmd, from_tty);

And again here.

> @@ -1283,7 +1286,6 @@
>       {
>         if (ptid_equal (inferior_ptid, null_ptid))
>   	error (_("No thread selected"));
> -
>         if (target_has_stack)
>   	{
>   	  if (is_exited (inferior_ptid))

This is a superfluous whitespace change. In general, we try to keep 
these types of changes in their own patches. It helps keep patches to 
the important changes.

> @@ -1338,6 +1340,7 @@
>       error (_("Invalid regexp (%s): %s"), tmp, arg);
>
>     update_thread_list ();
> +
>     for (tp = thread_list; tp; tp = tp->next)
>       {
>         if (tp->name != NULL && re_exec (tp->name))

Same here.

> @@ -1354,20 +1357,20 @@
>   			   tp->num, tmp);
>   	  match++;
>   	}
> -
> +      name = tp->name ? tp->name : target_thread_name (tp);
>         tmp = target_pid_to_str (tp->ptid);
>         if (tmp != NULL && re_exec (tmp))
>   	{
> -	  printf_filtered (_("Thread %d has target id '%s'\n"),
> -			   tp->num, tmp);
> +	  printf_filtered (_("Thread %d %s has target id '%s'\n"),
> +			   tp->num, name, tmp);
>   	  match++;
>   	}
>
>         tmp = target_extra_thread_info (tp);
>         if (tmp != NULL && re_exec (tmp))
>   	{
> -	  printf_filtered (_("Thread %d has extra info '%s'\n"),
> -			   tp->num, tmp);
> +	  printf_filtered (_("Thread %d %s has extra info '%s'\n"),
> +			   tp->num, name, tmp);
>   	  match++;

name could be NULL.

>   	}
>       }
> @@ -1391,6 +1394,7 @@
>   {
>     int num;
>     struct thread_info *tp;
> +  char *name;
>
>     num = value_as_long (parse_and_eval (tidstr));
>
> @@ -1405,9 +1409,12 @@
>     switch_to_thread (tp->ptid);
>
>     annotate_thread_changed ();
> +  name = tp->name ? tp->name : target_thread_name (tp);
>
>     ui_out_text (uiout, "[Switching to thread ");
> -  ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inferior_ptid));
> +  ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inferior_ptid));
> +  ui_out_text (uiout, " ");
> +  ui_out_text (uiout, name);
>     ui_out_text (uiout, " (");
>     ui_out_text (uiout, target_pid_to_str (inferior_ptid));
>     ui_out_text (uiout, ")]");

And one more time with NULL from target_thread_name. This is also a 
change to the MI. It will definitely need the approval of an MI or 
global maintainer, as well as updated MI documentation and tests.

inferior_command looks like it could benefit from similar treatment. I 
wonder if there are other places?

Otherwise, running the test suite, this patch causes several regressions 
which will need fixing:
FAIL: gdb.threads/thread-find.exp: find thread id 6
FAIL: gdb.threads/thread-find.exp: find thread id 5
FAIL: gdb.threads/thread-find.exp: find thread id 4
FAIL: gdb.threads/thread-find.exp: find thread id 3
FAIL: gdb.threads/thread-find.exp: find thread id 2
FAIL: gdb.threads/thread-find.exp: find thread id 1
FAIL: gdb.threads/thread-find.exp: find lwp id 6
FAIL: gdb.threads/thread-find.exp: find lwp id 5
FAIL: gdb.threads/thread-find.exp: find lwp id 4
FAIL: gdb.threads/thread-find.exp: find lwp id 3
FAIL: gdb.threads/thread-find.exp: find lwp id 2
FAIL: gdb.threads/thread-find.exp: find lwp id 1
FAIL: gdb.threads/thread_events.exp: continue to threadfunc with 
messages enabled (saw 0, expected 1)

It appears that these simply need to be massaged to account for the 
thread name output, although in one case, "(null)" is output for the 
thread name.

Similar changes will need to be done for the manual as well (see, for 
example, section 4.10, "Debugging Programs with Multiple Threads," which 
contains several examples that could use updating).

Keith