This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: gdb bug 12417
- From: Mohsan Saleem <mohsansaleem_ms at yahoo dot com>
- To: Keith Seitz <keiths at redhat dot com>, Jan Kratochvil <jan dot kratochvil at redhat dot com>, Yao Qi <yao at codesourcery dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Wed, 19 Sep 2012 09:07:03 -0700 (PDT)
- Subject: Re: gdb bug 12417
- References: <A62F3BCAE6F6B540A2F2C0E7E4387B9505B5AA72@EU-MBX-01.mgc.mentorg.com> <1347974206.13036.YahooMailNeo@web114403.mail.gq1.yahoo.com> <5059CC1D.1040105@redhat.com>
- Reply-to: Mohsan Saleem <mohsansaleem_ms at yahoo dot 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
>