This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: gdb bug 12417


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
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]