Re: gdb bug 12417

[Mohsan Saleem <mohsansaleem_ms@yahoo.com>]

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
>