[PATCH] gdb bug 12417

Jan Kratochvil jan.kratochvil@redhat.com
Wed Sep 19 13:09:00 GMT 2012


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



More information about the Gdb-patches mailing list