From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17245 invoked by alias); 1 Sep 2010 23:16:49 -0000 Received: (qmail 17234 invoked by uid 22791); 1 Sep 2010 23:16:48 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 01 Sep 2010 23:16:44 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 6EFF62BAC51; Wed, 1 Sep 2010 19:16:42 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id SAuS-0FgO2b8; Wed, 1 Sep 2010 19:16:42 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 347092BAC4B; Wed, 1 Sep 2010 19:16:42 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 39B68F599F; Thu, 2 Sep 2010 01:16:33 +0200 (CEST) Date: Thu, 02 Sep 2010 01:04:00 -0000 From: Joel Brobecker To: Hui Zhu Cc: gdb-patches ml Subject: Re: [PATCH] record.c: make prec can save the execution log to a pic file Message-ID: <20100901231633.GA16396@adacore.com> References: <4C181DF2.7030604@vmware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) 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/msg00076.txt.bz2 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. -- Joel