From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18800 invoked by alias); 1 Sep 2010 23:19:36 -0000 Received: (qmail 18788 invoked by uid 22791); 1 Sep 2010 23:19:35 -0000 X-SWARE-Spam-Status: No, hits=-4.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-outbound-2.vmware.com (HELO smtp-outbound-2.vmware.com) (65.115.85.73) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 01 Sep 2010 23:19:25 +0000 Received: from mailhost3.vmware.com (mailhost3.vmware.com [10.16.27.45]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id 3EB70F00F; Wed, 1 Sep 2010 16:19:22 -0700 (PDT) Received: from msnyder-server.eng.vmware.com (promd-2s-dhcp138.eng.vmware.com [10.20.124.138]) by mailhost3.vmware.com (Postfix) with ESMTP id 32F90CD92C; Wed, 1 Sep 2010 16:19:22 -0700 (PDT) Message-ID: <4C7EDF79.9050904@vmware.com> Date: Thu, 02 Sep 2010 01:55:00 -0000 From: Michael Snyder User-Agent: Thunderbird 2.0.0.24 (X11/20100702) MIME-Version: 1.0 To: Joel Brobecker CC: Hui Zhu , gdb-patches ml Subject: Re: [PATCH] record.c: make prec can save the execution log to a pic file References: <4C181DF2.7030604@vmware.com> <20100901231633.GA16396@adacore.com> In-Reply-To: <20100901231633.GA16396@adacore.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: 2010-09/txt/msg00078.txt.bz2 Joel Brobecker wrote: > Hui, Global Maintainers, > >> 2010-06-25 Hui Zhu >> >> * record.c (set_record_pic_cmdlist, >> show_record_pic_cmdlist): New variables. >> (set_record_pic_command, >> show_record_pic_command): New functions. >> (record_pic_function, record_pic_line, record_pic_enum, >> set_record_pic_type, record_pic_hide_nofunction, >> record_pic_hide_nosource, record_pic_hide_same): New variables. >> (record_pic_fputs): New function. >> (function_list, node_list, edge_list): New struct. >> (function_list, node_list, edge_list): New variables. >> (record_pic_cleanups, record_pic_node, >> record_pic_edge, cmd_record_pic): New functions. >> (_initialize_record): Add new commands for record pic. > > I recommend that this patch be backed out of the GDB sources, from > both the HEAD and the 7.2 branch, for the following reasons: > > (1) The implementation can be improved: Clearly document the new > functions and types introduced by this patch, at a minimum. > Unnecessary use of lablels and associated gotos, better error > messages, confusing implementation... > > (2) But more importantly, I think that the contents of the graph, > or rather the contents of each node in the graph being produced > needs to be discussed and unified. For a short description of > the contents of that graph: > http://www.sourceware.org/ml/gdb-patches/2010-09/msg00070.html > > Just using one of Hui's example output, we see: > > | node: {title: "1.c:21 main 0x80483c1 c:1"} > | node: {title: "main 22 0x80483c8 c:1"} > | node: {title: "main 25 0x80483cf c:1"} > > In the first node, the line number is at the beginning of the > title, together with the filename. In the second one, the line > number is after. > > (3) Perhaps we might also want to discuss the actual commands > that this patch introduces. > > I don't see any of these issues as big issues, and I can live with > us keeping it. Reworking the syntax followed by the titles will cause > a user-visible change, but nothing considered incompatible. Cleaning > up the code can be done as followup patches. Changing the commands > themselves, however, would probably be a problem. In terms of the > 7.2 release which is being held up, I can produce the missing > documentation patch within a day or two. > > However, I still believe that it is better to back it out for now. > First of all, I've already suggested that the code in record.c be > at least properly documented, but this never happened. Second of all, > I think that the current format used in the nodes titles is inconsistent > and makes the resulting graph less useful. What I believe we should do, > for this feature, is first go to the drawing board to describe precisely > what we want, and only then implement it. I agree. It needs more discussion.