This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] record.c: make prec can save the execution log to a pic file
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Hui Zhu <teawater at gmail dot com>
- Cc: gdb-patches ml <gdb-patches at sourceware dot org>
- Date: Wed, 1 Sep 2010 16:16:33 -0700
- Subject: Re: [PATCH] record.c: make prec can save the execution log to a pic file
- References: <AANLkTilO5PKpExEeH53xYI-R3Bm6zWRxNN9MW6SEbGuJ@mail.gmail.com> <4C181DF2.7030604@vmware.com> <AANLkTimQMtNbbCfHK08bRMmovS1wXWSjvL1yAARl-NlJ@mail.gmail.com> <AANLkTikyhrgZhAjgkI_FBE3tvEaV0ybKSYLTv4GV8ZLD@mail.gmail.com> <AANLkTimAH67D1DHWYkS4PYMAGh3rNEad1CCdUJYVSnU1@mail.gmail.com> <AANLkTilqJurquvxC8wM2bAfzAFzHBeKR8G_VddTwlRAo@mail.gmail.com>
Hui, Global Maintainers,
> 2010-06-25 Hui Zhu <teawater@gmail.com>
>
> * 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