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

[Michael Snyder <msnyder@vmware.com>]

Joel Brobecker wrote:
> 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.

I agree.  It needs more discussion.