Re: [PATCH] record.c: make prec can save the execution log to a pic file

[Joel Brobecker <brobecker@adacore.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