gdb bug 12417

gdb bug 12417

[Mohsan Saleem]

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

Hi,
Attached patch is for bug 12417, for printing the name of threads while printing the information of a thread.

Thanks,
Mohsan Saleem 

[-- Attachment #2: 12417.patch --]
[-- Type: application/octet-stream, Size: 3976 bytes --]

--- /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);
 
   annotate_new_thread ();
   return result;
@@ -1192,7 +1194,7 @@
 {
   struct thread_info *tp;
   struct cleanup *old_chain;
-  char *saved_cmd;
+  char *saved_cmd, *name;
 
   if (cmd == NULL || *cmd == '\000')
     error (_("Please specify a command following the thread ID list"));
@@ -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
 					   previously.  */
@@ -1222,7 +1225,7 @@
 static void
 thread_apply_command (char *tidlist, int from_tty)
 {
-  char *cmd;
+  char *cmd, *name;
   struct cleanup *old_chain;
   char *saved_cmd;
   struct get_number_or_range_state state;
@@ -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);
 
@@ -1283,7 +1286,6 @@
     {
       if (ptid_equal (inferior_ptid, null_ptid))
 	error (_("No thread selected"));
-
       if (target_has_stack)
 	{
 	  if (is_exited (inferior_ptid))
@@ -1327,7 +1329,7 @@
 thread_find_command (char *arg, int from_tty)
 {
   struct thread_info *tp;
-  char *tmp;
+  char *tmp, *name;
   unsigned long match = 0;
 
   if (arg == NULL || *arg == '\0')
@@ -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))
@@ -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++;
 	}
     }
@@ -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, ")]");

[PATCH] gdb bug 12417

[Mohsan Saleem]

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

Hi,
     Attached patch is for bug 12417, for printing the name of threads while printing the information of a thread. 

Thanks,
Mohsan Saleem 

[-- Attachment #2: 12417.patch --]
[-- Type: application/octet-stream, Size: 3976 bytes --]

--- /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);
 
   annotate_new_thread ();
   return result;
@@ -1192,7 +1194,7 @@
 {
   struct thread_info *tp;
   struct cleanup *old_chain;
-  char *saved_cmd;
+  char *saved_cmd, *name;
 
   if (cmd == NULL || *cmd == '\000')
     error (_("Please specify a command following the thread ID list"));
@@ -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
 					   previously.  */
@@ -1222,7 +1225,7 @@
 static void
 thread_apply_command (char *tidlist, int from_tty)
 {
-  char *cmd;
+  char *cmd, *name;
   struct cleanup *old_chain;
   char *saved_cmd;
   struct get_number_or_range_state state;
@@ -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);
 
@@ -1283,7 +1286,6 @@
     {
       if (ptid_equal (inferior_ptid, null_ptid))
 	error (_("No thread selected"));
-
       if (target_has_stack)
 	{
 	  if (is_exited (inferior_ptid))
@@ -1327,7 +1329,7 @@
 thread_find_command (char *arg, int from_tty)
 {
   struct thread_info *tp;
-  char *tmp;
+  char *tmp, *name;
   unsigned long match = 0;
 
   if (arg == NULL || *arg == '\0')
@@ -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))
@@ -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++;
 	}
     }
@@ -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, ")]");

Re: [PATCH] gdb bug 12417

[Jan Kratochvil]

On Wed, 19 Sep 2012 12:59:54 +0200, Mohsan Saleem wrote:
> Attached patch is for bug 12417, for printing the name of threads while
> printing the information of a thread. 

not yet a review but according to the file gdb/CONTRIBUTE there is missing
ChangeLog entry.

And the attachment is base64-encoded with CR-LF (\r\n) there.  It is preferred
to inline the patch as text but be careful not to line-wrap it.  Otherwise
quoted-printable attachment is better (where newlines are implicit).  Or for
base64 at least make it LF.

It would be nice to see some changes in a testcase but it does not have to be.

You should also always run the testsuite (make check) before change and after
the change if there are no regressions.  In this case there are:

-PASS: gdb.threads/thread-find.exp: find thread id 6
-PASS: gdb.threads/thread-find.exp: find thread id 5
-PASS: gdb.threads/thread-find.exp: find thread id 4 
-PASS: gdb.threads/thread-find.exp: find thread id 3 
-PASS: gdb.threads/thread-find.exp: find thread id 2 
-PASS: gdb.threads/thread-find.exp: find thread id 1 
-PASS: gdb.threads/thread-find.exp: find lwp id 6
-PASS: gdb.threads/thread-find.exp: find lwp id 5
-PASS: gdb.threads/thread-find.exp: find lwp id 4
-PASS: gdb.threads/thread-find.exp: find lwp id 3
-PASS: gdb.threads/thread-find.exp: find lwp id 2
-PASS: gdb.threads/thread-find.exp: find lwp id 1
+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
-PASS: gdb.threads/thread_events.exp: continue to threadfunc with messages enabled
+FAIL: gdb.threads/thread_events.exp: continue to threadfunc with messages enabled (saw 0, expected 1)


Thanks,
Jan


> --- /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 @@

Please use also option -p for diff, it makes the diff more clear.


>  struct thread_info *
>  add_thread_with_info (ptid_t ptid, struct private_thread_info *private)
>  {
> +  char *name;

It could be: const 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);

There is excessive space and also the square brackets [ ] should be around the
whole line:
      printf_unfiltered (_("[New %s %s]\n"), target_pid_to_str (ptid), name);


>  
>    annotate_new_thread ();
>    return result;
> @@ -1192,7 +1194,7 @@
>  {
>    struct thread_info *tp;
>    struct cleanup *old_chain;
> -  char *saved_cmd;
> +  char *saved_cmd, *name;

Again const char *.


>  
>    if (cmd == NULL || *cmd == '\000')
>      error (_("Please specify a command following the thread ID list"));
> @@ -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
>  					   previously.  */

This part does not apply to latest trunk - that is FSF GDB HEAD, please rebase
your patch to it:
	http://www.gnu.org/software/gdb/current/


> @@ -1222,7 +1225,7 @@
>  static void
>  thread_apply_command (char *tidlist, int from_tty)
>  {
> -  char *cmd;
> +  char *cmd, *name;

Again const char *.


>    struct cleanup *old_chain;
>    char *saved_cmd;
>    struct get_number_or_range_state state;
> @@ -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);
>  
> @@ -1283,7 +1286,6 @@
>      {
>        if (ptid_equal (inferior_ptid, null_ptid))
>  	error (_("No thread selected"));
> -

Excessive change.


>        if (target_has_stack)
>  	{
>  	  if (is_exited (inferior_ptid))
> @@ -1327,7 +1329,7 @@
>  thread_find_command (char *arg, int from_tty)
>  {
>    struct thread_info *tp;
> -  char *tmp;
> +  char *tmp, *name;

const char *.


>    unsigned long match = 0;
>  
>    if (arg == NULL || *arg == '\0')
> @@ -1338,6 +1340,7 @@
>      error (_("Invalid regexp (%s): %s"), tmp, arg);
>  
>    update_thread_list ();
> +  

Excessive change.


>    for (tp = thread_list; tp; tp = tp->next)
>      {
>        if (tp->name != NULL && re_exec (tp->name))
> @@ -1354,20 +1357,20 @@
>  			   tp->num, tmp);
>  	  match++;
>  	}
> -
Excessive change.


> +      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);

The change is incomplete:
(gdb) thread find .*
Thread 2 has target name 'threadit'
Thread 2 threadit has target id 'Thread 0x7ffff7807700 (LWP 31213)'
Thread 1 has target name 'threadit'
Thread 1 threadit has target id 'Thread 0x7ffff7fe5740 (LWP 31207)'

I believe there should be also (although it looks weird):
Thread 2 threadit has target name 'threadit'


Haven't you considered rather changing linux_nat_pid_to_str?

It could be supported also for gdbserver but to_thread_name is currently
already implemented only in linux-nat.c (linux_nat_thread_name) so it is not
a requirement for this patch.


>  	  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++;
>  	}
>      }
> @@ -1391,6 +1394,7 @@
>  {
>    int num;
>    struct thread_info *tp;
> +  char *name;

const char *


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

There is no change between these lines, I do not understand the diff.


> +  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, ")]");
> 

Re: [PATCH] gdb bug 12417

[Yao Qi]

On 09/19/2012 06:59 PM, Mohsan Saleem wrote:
> --- /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);

This pattern is used five times in your patch, and why don't we move it 
to a function? like,

const char *
thread_info_name (struct thread_info *ti)
{
   return ti->name ? ti->name : target_thread_name (ti);
}

and call this function when needed.

-- 
Yao

Re: gdb bug 12417

[Keith Seitz]

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

Re: gdb bug 12417

[Mohsan Saleem]

Thanks for your kind response.

It is not an assignment. This is my ever first patch. Therefore, it contains to many bugs.
I will move similar lines to a function. I am trying to understanding how to write ChangeLog entry.

For "null" problem I was thinking to replace it with empty string(name = ""). Is it OK? 

Do I need to change testcase files *.exp if there are regressions in any?

Thanks
Mohsan Saleem


----- Original Message -----
> From: Keith Seitz <keiths@redhat.com>
> To: Mohsan Saleem <mohsansaleem_ms@yahoo.com>
> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> Sent: Wednesday, 19 September 2012 6:43 PM
> Subject: Re: gdb bug 12417
> 
> 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
>

Re: gdb bug 12417

[Sergio Durigan Junior]

On Wednesday, September 19 2012, Mohsan Saleem wrote:

> It is not an assignment. This is my ever first patch.

Then you will need a copyright assignment.

I can send you the form to begin the process, please contact me off-list
for that.

Thanks,

-- 
Sergio

Re: gdb bug 12417

[Jan Kratochvil]

On Wed, 19 Sep 2012 18:07:03 +0200, Mohsan Saleem wrote:
> I am trying to understanding how to write ChangeLog entry.

http://www.gnu.org/prep/standards/standards.html#Change-Logs


> For "null" problem I was thinking to replace it with empty string(name = ""). Is it OK? 

Why is "(null)" happenning there?  This will say how to deal with it.


> Do I need to change testcase files *.exp if there are regressions in any?

It depends, you should check unpatched gdb.log -> patched gdb.log what are the
changes there.  Depending on the reason either improvide/fix your code patch
or update the expected text in the .exp files.


Thanks,
Jan

[PATCH] gdb bug 12417

[Mohsan Saleem]

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

Hi,
     Attached patch is for bug 12417, for printing the name of threads while printing the information of a thread. 

Thanks,
Mohsan Saleem

 
--
Mohsan Saleem

[-- Attachment #2: diff.patch --]
[-- Type: application/octet-stream, Size: 5400 bytes --]

diff -ruN ./gdb_old/ChangeLog ./gdb_new/ChangeLog
--- ./gdb_old/ChangeLog	2012-09-24 13:17:34.000000000 +0500
+++ ./gdb_new/ChangeLog	2012-09-24 22:25:44.465441936 +0500
@@ -1,3 +1,12 @@
+2012-09-24  Mohsan Saleem  <mohsansaleem_ms@yahoo.com>
+
+	* thread.c (thread_name): New function. To get thread name
+	(add_thread_with_info): Update to print thread name.
+	(thread_apply_all_command): Likewise.
+	(thread_apply_command): Likewise.
+	(thread_find_command): Likewise.
+	(do_captured_thread_select): Likewise.
+
 2012-09-21  Steve Ellcey  <sellcey@mips.com>
 
 	* configure.ac: Add mips*-mti-elf* target.
diff -ruN ./gdb_old/gdb/testsuite/gdb.threads/thread-find.exp ./gdb_new/gdb/testsuite/gdb.threads/thread-find.exp
--- ./gdb_old/gdb/testsuite/gdb.threads/thread-find.exp	2012-09-24 13:17:38.000000000 +0500
+++ ./gdb_new/gdb/testsuite/gdb.threads/thread-find.exp	2012-09-24 21:55:44.737537299 +0500
@@ -186,17 +186,17 @@
 
 if { [info exists thread6] } then {
     gdb_test "thread find $thread6" \
-	"Thread 6 has .*$thread6.*" "find thread id 6"
+	"Thread 6 threadname_6 has .*$thread6.*" "find thread id 6"
     gdb_test "thread find $thread5" \
-	"Thread 5 has .*$thread5.*" "find thread id 5"
+	"Thread 5 threadname_5 has .*$thread5.*" "find thread id 5"
     gdb_test "thread find $thread4" \
-	"Thread 4 has .*$thread4.*" "find thread id 4"
+	"Thread 4 threadname_4 has .*$thread4.*" "find thread id 4"
     gdb_test "thread find $thread3" \
-	"Thread 3 has .*$thread3.*" "find thread id 3"
+	"Thread 3 threadname_3 has .*$thread3.*" "find thread id 3"
     gdb_test "thread find $thread2" \
-	"Thread 2 has .*$thread2.*" "find thread id 2"
+	"Thread 2 threadname_2 has .*$thread2.*" "find thread id 2"
     gdb_test "thread find $thread1" \
-	"Thread 1 has .*$thread1.*" "find thread id 1"
+	"Thread 1 threadname_1 has .*$thread1.*" "find thread id 1"
 }
 
 #
@@ -224,17 +224,17 @@
 
 if { [info exists lwp6] } then {
     gdb_test "thread find $lwp6" \
-	"Thread 6 has .*$lwp6.*" "find lwp id 6"
+	"Thread 6 threadname_6 has .*$lwp6.*" "find lwp id 6"
     gdb_test "thread find $lwp5" \
-	"Thread 5 has .*$lwp5.*" "find lwp id 5"
+	"Thread 5 threadname_5 has .*$lwp5.*" "find lwp id 5"
     gdb_test "thread find $lwp4" \
-	"Thread 4 has .*$lwp4.*" "find lwp id 4"
+	"Thread 4 threadname_4 has .*$lwp4.*" "find lwp id 4"
     gdb_test "thread find $lwp3" \
-	"Thread 3 has .*$lwp3.*" "find lwp id 3"
+	"Thread 3 threadname_3 has .*$lwp3.*" "find lwp id 3"
     gdb_test "thread find $lwp2" \
-	"Thread 2 has .*$lwp2.*" "find lwp id 2"
+	"Thread 2 threadname_2 has .*$lwp2.*" "find lwp id 2"
     gdb_test "thread find $lwp1" \
-	"Thread 1 has .*$lwp1.*" "find lwp id 1"
+	"Thread 1 threadname_1 has .*$lwp1.*" "find lwp id 1"
 }
 
 # Test no match.
diff -ruN ./gdb_old/gdb/thread.c ./gdb_new/gdb/thread.c
--- ./gdb_old/gdb/thread.c	2012-09-24 13:17:37.000000000 +0500
+++ ./gdb_new/gdb/thread.c	2012-09-24 21:54:46.689540365 +0500
@@ -64,6 +64,7 @@
 static void thread_apply_command (char *, int);
 static void restore_current_thread (ptid_t);
 static void prune_threads (void);
+static const char* thread_name (struct thread_info *);
 
 struct thread_info*
 inferior_thread (void)
@@ -73,6 +74,17 @@
   return tp;
 }
 
+const char*
+thread_name (struct thread_info *ti)
+{
+  const char* name;
+  name = ti->name ? ti->name : target_thread_name(ti);
+  if (name)
+    return name;
+  else
+    return "";
+}
+
 void
 delete_step_resume_breakpoint (struct thread_info *tp)
 {
@@ -236,7 +248,7 @@
   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), thread_name (result));
 
   annotate_new_thread ();
   return result;
@@ -1198,8 +1210,8 @@
       {
 	switch_to_thread (tp->ptid);
 
-	printf_filtered (_("\nThread %d (%s):\n"),
-			 tp->num, target_pid_to_str (inferior_ptid));
+	printf_filtered (_("\nThread %d %s (%s):\n"), tp->num,
+			 thread_name (tp), target_pid_to_str (inferior_ptid));
 	execute_command (cmd, from_tty);
 	strcpy (cmd, saved_cmd);	/* Restore exact command used
 					   previously.  */
@@ -1250,7 +1262,7 @@
 	{
 	  switch_to_thread (tp->ptid);
 
-	  printf_filtered (_("\nThread %d (%s):\n"), tp->num,
+	  printf_filtered (_("\nThread %d %s (%s):\n"), tp->num, thread_name (tp),
 			   target_pid_to_str (inferior_ptid));
 	  execute_command (cmd, from_tty);
 
@@ -1347,16 +1359,16 @@
       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, thread_name (tp), 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, thread_name (tp), tmp);
 	  match++;
 	}
     }
@@ -1397,6 +1409,8 @@
 
   ui_out_text (uiout, "[Switching to thread ");
   ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inferior_ptid));
+  ui_out_text (uiout, " ");
+  ui_out_text (uiout, thread_name (tp));
   ui_out_text (uiout, " (");
   ui_out_text (uiout, target_pid_to_str (inferior_ptid));
   ui_out_text (uiout, ")]");

Re: [PATCH] gdb bug 12417

[Sergio Durigan Junior]

Hello Mohsan,

Thanks for the patch.  Not really a technical review, but rather
formatting nits.

On Monday, September 24 2012, Mohsan Saleem wrote:

> diff -ruN ./gdb_old/ChangeLog ./gdb_new/ChangeLog
> --- ./gdb_old/ChangeLog	2012-09-24 13:17:34.000000000 +0500
> +++ ./gdb_new/ChangeLog	2012-09-24 22:25:44.465441936 +0500
> @@ -1,3 +1,12 @@
> +2012-09-24  Mohsan Saleem  <mohsansaleem_ms@yahoo.com>
> +
> +	* thread.c (thread_name): New function. To get thread name
                                               ^^

Two spaces after period.  Missing period at the end of the sentence.
You also don't need to explain what `thread_name' does, therefore you
can get rid of the "To get thread name" part.

> +	(add_thread_with_info): Update to print thread name.
> +	(thread_apply_all_command): Likewise.
> +	(thread_apply_command): Likewise.
> +	(thread_find_command): Likewise.
> +	(do_captured_thread_select): Likewise.

Since you are solving a PR, it is recommended that you put its reference
on the ChangeLog message, like this:

2012-09-24  Mohsan Saleem  <mohsansaleem_ms@yahoo.com>

	PR threads/12417
	...
	
> diff -ruN ./gdb_old/gdb/testsuite/gdb.threads/thread-find.exp ./gdb_new/gdb/testsuite/gdb.threads/thread-find.exp
> --- ./gdb_old/gdb/testsuite/gdb.threads/thread-find.exp	2012-09-24 13:17:38.000000000 +0500
> +++ ./gdb_new/gdb/testsuite/gdb.threads/thread-find.exp	2012-09-24 21:55:44.737537299 +0500
> @@ -186,17 +186,17 @@
>  
>  if { [info exists thread6] } then {
>      gdb_test "thread find $thread6" \
> -	"Thread 6 has .*$thread6.*" "find thread id 6"
> +	"Thread 6 threadname_6 has .*$thread6.*" "find thread id 6"

I have some comments about the printing style, see below.

> diff -ruN ./gdb_old/gdb/thread.c ./gdb_new/gdb/thread.c
> --- ./gdb_old/gdb/thread.c	2012-09-24 13:17:37.000000000 +0500
> +++ ./gdb_new/gdb/thread.c	2012-09-24 21:54:46.689540365 +0500
> @@ -64,6 +64,7 @@
>  static void thread_apply_command (char *, int);
>  static void restore_current_thread (ptid_t);
>  static void prune_threads (void);
> +static const char* thread_name (struct thread_info *);
                    ^

The `*' must be together with `thread_name', not `char':

    static const char *thread_name (struct thread_info *);

> +const char*
             ^

Should be:

     const char *

> +thread_name (struct thread_info *ti)
> +{
> +  const char* name;
               ^

Should be:

     const char *name;

> +  name = ti->name ? ti->name : target_thread_name(ti);

The standard is to explict check for NULL.  Also, there must be one
space between the function name and the open paren.

     name = ti->name != NULL ? ti-> name : target_thread_name (ti);

> +  if (name)
> +    return name;
> +  else
> +    return "";

You could do:

    return name != NULL ? name : "";

This whole function could also be shortened, but I think it's OK as-is.

>  void
>  delete_step_resume_breakpoint (struct thread_info *tp)
>  {
> @@ -236,7 +248,7 @@
>    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), thread_name (result));
>  
>    annotate_new_thread ();
>    return result;
> @@ -1198,8 +1210,8 @@
>        {
>  	switch_to_thread (tp->ptid);
>  
> -	printf_filtered (_("\nThread %d (%s):\n"),
> -			 tp->num, target_pid_to_str (inferior_ptid));
> +	printf_filtered (_("\nThread %d %s (%s):\n"), tp->num,
> +			 thread_name (tp), target_pid_to_str (inferior_ptid));

I would like to hear others' opinions, but I guess this line could be
printed differently.  Currently it is:

        Thread 2 thread_name2 (12323)...

It could now be:

        Thread thread_name2 [#2] (12323)...

I think it gives more importance to the name.  The same comments applies
to all other commands.  But please, don't change anything yet, wait
until a maintainer states his opinions.

WDYT?

-- 
Sergio