From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28147 invoked by alias); 15 May 2014 19:03:28 -0000 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 Received: (qmail 27933 invoked by uid 89); 15 May 2014 19:03:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 15 May 2014 19:03:26 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4FJ3NEW017827 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 15 May 2014 15:03:23 -0400 Received: from barimba (ovpn-113-182.phx2.redhat.com [10.3.113.182]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s4FJ3Mst005075 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Thu, 15 May 2014 15:03:22 -0400 From: Tom Tromey To: "Saleem\, Mohsan" Cc: Mohsan Saleem , "gdb-patches\@sourceware.org" , "palves\@redhat.com" Subject: Re: [PATCH] fix PR-12417 References: <521E2414.40602@codesourcery.com> <52254BC6.1050105@codesourcery.com> <1378282781.96893.YahooMailNeo@web142604.mail.bf1.yahoo.com> <0377C58828D86C4588AEEC42FC3B85A71766C229@IRSMSX105.ger.corp.intel.com> <1378293943.43616.YahooMailNeo@web142603.mail.bf1.yahoo.com> <1379391306.56146.YahooMailNeo@web142602.mail.bf1.yahoo.com> <8738n91939.fsf@fleche.redhat.com> Date: Thu, 15 May 2014 19:03:00 -0000 In-Reply-To: (Mohsan Saleem's message of "Fri, 14 Feb 2014 10:40:52 +0000") Message-ID: <8738gaampx.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2014-05/txt/msg00263.txt.bz2 Finally responding to this... Mohsan> if (print_thread_events) Mohsan> - printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid)); Mohsan> + printf_unfiltered (_("[New %s \"%s\"]\n"),=20 Mohsan> + target_pid_to_str (ptid), thread_name (result)); Tom> This line is too long. Tom> Also, I think the output will be weird if the thread does not have a n= ame. Mohsan> What I'm supposed to do about line length? Normally one splits the line at the appropriate point, and then indents the continuation line appropriately. There is some discussion in the GNU Coding Standards document, plus a plethora of examples in gdb. In your patch the line looked like: printf_unfiltered (_("[New %s \"%s\"\n"), target_pid_to_str (ptid), thr= ead_name (result)); I would write this as: printf_unfiltered (_("[New %s \"%s\"\n"), target_pid_to_str (ptid), thread_name (result)); Mohsan> Any thread without a name is automatically assigned with the program Mohsan> name argv[0]. So Empty string could not be a problem here. But the new thread_name function can return "". If this happens then the output will be strange. It this cannot happen then thread_name ought to be changed. I think it can happen, though, because the default to_thread_name returns NULL. Tom> Two thoughts come to mind for the patch. Tom> First, perhaps a single function for emitting the thread description Tom> would be better. Then it could be normalized across all of gdb. Tom> Second, it would be nice to use ui-out properly in such a function, Tom> so that MI can see the thread name distinctly from the other bits. Mohsan> Sorry, I didn't got your point here. Could you please elaborate it a Mohsan> little more, as I am new to GDB. Sure, no problem. The patch has a number of hunks like this: -=C2=A0=C2=A0=C2=A0 =C2=A0 printf_filtered (_("Thread %d has target id '%s'= \n"), -=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2= =A0tp->num, tmp); +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 printf_filtered (_("Thread %d \"%s\" ha= s target id '%s'\n"), +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2= =A0tp->num, thread_name (tp), tmp); Given the fact that an empty result from thread_name will result in strange-looking output, it would probably be better to normalize the thread-name-printing parts into a single function. Second, the patch says: -=C2=A0 ui_out_text (uiout, " ("); +=C2=A0 ui_out_text (uiout, " \""); +=C2=A0 ui_out_text (uiout, thread_name (tp)); +=C2=A0 ui_out_text (uiout, "\" ("); This means that the thread name is just dropped from the MI output. However, since care is already taken here to use ui_out, it is preferable to emit the thread name in an MI-readable form. This can be done with a call to ui_out_field_string. Tom