From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9564 invoked by alias); 19 Sep 2012 16:07:20 -0000 Received: (qmail 9546 invoked by uid 22791); 19 Sep 2012 16:07:18 -0000 X-SWARE-Spam-Status: No, hits=-0.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,FSL_FREEMAIL_1,FSL_FREEMAIL_2,KHOP_THREADED,RCVD_IN_DNSWL_NONE,RCVD_IN_HOSTKARMA_YE,RP_MATCHES_RCVD,TW_CP X-Spam-Check-By: sourceware.org Received: from nm15-vm3.bullet.mail.ne1.yahoo.com (HELO nm15-vm3.bullet.mail.ne1.yahoo.com) (98.138.91.145) by sourceware.org (qpsmtpd/0.43rc1) with SMTP; Wed, 19 Sep 2012 16:07:05 +0000 Received: from [98.138.226.177] by nm15.bullet.mail.ne1.yahoo.com with NNFMP; 19 Sep 2012 16:07:04 -0000 Received: from [98.138.89.254] by tm12.bullet.mail.ne1.yahoo.com with NNFMP; 19 Sep 2012 16:07:04 -0000 Received: from [127.0.0.1] by omp1046.mail.ne1.yahoo.com with NNFMP; 19 Sep 2012 16:07:04 -0000 Received: (qmail 40597 invoked by uid 60001); 19 Sep 2012 16:07:03 -0000 Received: from [110.93.212.98] by web114407.mail.gq1.yahoo.com via HTTP; Wed, 19 Sep 2012 09:07:03 PDT References: <1347974206.13036.YahooMailNeo@web114403.mail.gq1.yahoo.com> <5059CC1D.1040105@redhat.com> Message-ID: <1348070823.10464.YahooMailNeo@web114407.mail.gq1.yahoo.com> Date: Wed, 19 Sep 2012 16:07:00 -0000 From: Mohsan Saleem Reply-To: Mohsan Saleem Subject: Re: gdb bug 12417 To: Keith Seitz , Jan Kratochvil , Yao Qi Cc: "gdb-patches@sourceware.org" MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-09/txt/msg00396.txt.bz2 Thanks for your kind response. It is not an assignment. This is my ever first patch. Therefore, it contain= s to many bugs. I will=A0move 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 =3D = ""). Is it OK?=A0 Do I need to change testcase files *.exp if there are=A0regressions=A0in an= y? Thanks Mohsan Saleem ----- Original Message ----- > From: Keith Seitz > To: Mohsan Saleem > Cc: "gdb-patches@sourceware.org" > Sent: Wednesday, 19 September 2012 6:43 PM > Subject: Re: gdb bug 12417 >=20 > On 09/18/2012 06:16 AM, Mohsan Saleem wrote: >>=A0 Hi, >>=A0 Attached patch is for bug 12417, for printing the name of threads whi= le=20 > printing the information of a thread. >>=20 >=20 > Thank you for you patch submission.=A0 I do not see your name listed in=20 > gdb/MAINTAINERS -- do you have an assignment? If not, let us know and we = can=20 > help you get started with the paperwork. >=20 > In general, I'd say the patch is definitely a feature we should consider= =20 > adding. [Note: I am not a global maintainer.] >=20 >>=A0 --- /root/Desktop/gdb/thread.c=A0=A0=A0 2012-09-03 00:53:02.620254912= -0700 >>=A0 +++ ./gdb/thread.c=A0=A0=A0 2012-09-03 00:58:04.399469614 -0700 >>=A0 @@ -243,12 +243,14 @@ >> =A0=A0 struct thread_info * >> =A0=A0 add_thread_with_info (ptid_t ptid, struct private_thread_info *pr= ivate) >> =A0=A0 { >>=A0 +=A0 char *name; >> =A0 =A0=A0 struct thread_info *result =3D add_thread_silent (ptid); >>=20 >>=A0 +=A0 name =3D result->name ? result->name : target_thread_name (resul= t); >> =A0 =A0=A0 result->private =3D private; >>=20 >> =A0 =A0=A0 if (print_thread_events) >>=A0 -=A0 =A0 printf_unfiltered (_("[New %s]\n"), target_pid_to_str=20 > (ptid)); >>=A0 +=A0 =A0 printf_unfiltered (_("[New %s ] %s\n"),=20 > target_pid_to_str (ptid), name); >=20 > target_thread_name can return NULL. There's an added space typo in=20 > "[New %s ]". >=20 >>=A0 @@ -1209,8 +1211,9 @@ >> =A0 =A0 =A0=A0 if (thread_alive (tp)) >> =A0 =A0 =A0 =A0=A0 { >> =A0=A0 =A0=A0=A0 switch_to_thread (tp->ptid); >>=A0 -=A0=A0=A0 printf_filtered (_("\nThread %d (%s):\n"), >>=A0 -=A0=A0=A0 =A0=A0=A0 =A0=A0=A0=A0 tp->num, target_pid_to_str (inferio= r_ptid)); >>=A0 +=A0=A0=A0 name =3D tp->name ? tp->name : target_thread_name (tp); >>=A0 +=A0=A0=A0 printf_filtered (_("\nThread %d %s (%s):\n"), >>=A0 +=A0=A0=A0 =A0=A0=A0 =A0=A0=A0=A0 tp->num, name, target_pid_to_str (i= nferior_ptid)); >> =A0=A0 =A0=A0=A0 execute_command (cmd, from_tty); >> =A0=A0 =A0=A0=A0 strcpy (cmd, saved_cmd);=A0=A0=A0 /* Restore exact comm= and used >=20 > Likewise. >=20 >>=A0 @@ -1260,8 +1263,8 @@ >> =A0 =A0 =A0 =A0=A0 else >> =A0=A0 =A0=A0=A0 { >> =A0=A0 =A0=A0=A0 =A0 switch_to_thread (tp->ptid); >>=A0 - >>=A0 -=A0=A0=A0 =A0 printf_filtered (_("\nThread %d (%s):\n"),=20 > tp->num, >>=A0 +=A0=A0=A0 =A0 name =3D tp->name ? tp->name : target_thread_name (tp); >>=A0 +=A0=A0=A0 =A0 printf_filtered (_("\nThread %d %s (%s):\n"),=20 > tp->num, name, >> =A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0 target_pid_to_str (inferior_= ptid)); >> =A0=A0 =A0=A0=A0 =A0 execute_command (cmd, from_tty); >=20 > And again here. >=20 >>=A0 @@ -1283,7 +1286,6 @@ >> =A0 =A0 =A0=A0 { >> =A0 =A0 =A0 =A0=A0 if (ptid_equal (inferior_ptid, null_ptid)) >> =A0=A0 =A0=A0=A0 error (_("No thread selected")); >>=A0 - >> =A0 =A0 =A0 =A0=A0 if (target_has_stack) >> =A0=A0 =A0=A0=A0 { >> =A0=A0 =A0=A0=A0 =A0 if (is_exited (inferior_ptid)) >=20 > This is a superfluous whitespace change. In general, we try to keep these= types=20 > of changes in their own patches. It helps keep patches to the important c= hanges. >=20 >>=A0 @@ -1338,6 +1340,7 @@ >> =A0 =A0 =A0=A0 error (_("Invalid regexp (%s): %s"), tmp, arg); >>=20 >> =A0 =A0=A0 update_thread_list (); >>=A0 + >> =A0 =A0=A0 for (tp =3D thread_list; tp; tp =3D tp->next) >> =A0 =A0 =A0=A0 { >> =A0 =A0 =A0 =A0=A0 if (tp->name !=3D NULL && re_exec (tp->name)) >=20 > Same here. >=20 >>=A0 @@ -1354,20 +1357,20 @@ >> =A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0 tp->num, tmp); >> =A0=A0 =A0=A0=A0 =A0 match++; >> =A0=A0 =A0=A0=A0 } >>=A0 - >>=A0 +=A0 =A0 =A0 name =3D tp->name ? tp->name : target_thread_name (tp); >> =A0 =A0 =A0 =A0=A0 tmp =3D target_pid_to_str (tp->ptid); >> =A0 =A0 =A0 =A0=A0 if (tmp !=3D NULL && re_exec (tmp)) >> =A0=A0 =A0=A0=A0 { >>=A0 -=A0=A0=A0 =A0 printf_filtered (_("Thread %d has target id=20 > '%s'\n"), >>=A0 -=A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0 tp->num, tmp); >>=A0 +=A0=A0=A0 =A0 printf_filtered (_("Thread %d %s has target id=20 > '%s'\n"), >>=A0 +=A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0 tp->num, name, tmp); >> =A0=A0 =A0=A0=A0 =A0 match++; >> =A0=A0 =A0=A0=A0 } >>=20 >> =A0 =A0 =A0 =A0=A0 tmp =3D target_extra_thread_info (tp); >> =A0 =A0 =A0 =A0=A0 if (tmp !=3D NULL && re_exec (tmp)) >> =A0=A0 =A0=A0=A0 { >>=A0 -=A0=A0=A0 =A0 printf_filtered (_("Thread %d has extra info=20 > '%s'\n"), >>=A0 -=A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0 tp->num, tmp); >>=A0 +=A0=A0=A0 =A0 printf_filtered (_("Thread %d %s has extra info=20 > '%s'\n"), >>=A0 +=A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0 tp->num, name, tmp); >> =A0=A0 =A0=A0=A0 =A0 match++; >=20 > name could be NULL. >=20 >> =A0=A0 =A0=A0=A0 } >> =A0 =A0 =A0=A0 } >>=A0 @@ -1391,6 +1394,7 @@ >> =A0=A0 { >> =A0 =A0=A0 int num; >> =A0 =A0=A0 struct thread_info *tp; >>=A0 +=A0 char *name; >>=20 >> =A0 =A0=A0 num =3D value_as_long (parse_and_eval (tidstr)); >>=20 >>=A0 @@ -1405,9 +1409,12 @@ >> =A0 =A0=A0 switch_to_thread (tp->ptid); >>=20 >> =A0 =A0=A0 annotate_thread_changed (); >>=A0 +=A0 name =3D tp->name ? tp->name : target_thread_name (tp); >>=20 >> =A0 =A0=A0 ui_out_text (uiout, "[Switching to thread "); >>=A0 -=A0 ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id=20 > (inferior_ptid)); >>=A0 +=A0 ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id=20 > (inferior_ptid)); >>=A0 +=A0 ui_out_text (uiout, " "); >>=A0 +=A0 ui_out_text (uiout, name); >> =A0 =A0=A0 ui_out_text (uiout, " ("); >> =A0 =A0=A0 ui_out_text (uiout, target_pid_to_str (inferior_ptid)); >> =A0 =A0=A0 ui_out_text (uiout, ")]"); >=20 > And one more time with NULL from target_thread_name. This is also a chang= e to=20 > the MI. It will definitely need the approval of an MI or global maintaine= r, as=20 > well as updated MI documentation and tests. >=20 > inferior_command looks like it could benefit from similar treatment. I wo= nder if=20 > there are other places? >=20 > Otherwise, running the test suite, this patch causes several regressions = which=20 > 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= =20 > enabled (saw 0, expected 1) >=20 > It appears that these simply need to be massaged to account for the threa= d name=20 > output, although in one case, "(null)" is output for the thread name. >=20 > Similar changes will need to be done for the manual as well (see, for exa= mple,=20 > section 4.10, "Debugging Programs with Multiple Threads," which=20 > contains several examples that could use updating). >=20 > Keith >