Re: [PATCH] Add CTF support to GDB [1/4] Add "-ctf" to tsave command

[Hui Zhu <teawater at gmail dot com>]

On Tue, Dec 18, 2012 at 10:26 PM, Hui Zhu <teawater@gmail.com> wrote:
> On Fri, Dec 14, 2012 at 7:36 PM, Hui Zhu <teawater@gmail.com> wrote:
>> On Fri, Nov 30, 2012 at 4:06 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>>> "Hui" == Hui Zhu <hui_zhu@mentor.com> writes:
>>>
>>> Hui> This patch is for the CTF write.
>>> Hui> It add "-ctf" to tsave command.  With this option, tsave can save
>>> Hui> current trace frame to CTF file format.
>>>
>>> Hui> +struct ctf_save_collect_s
>>> Hui> +{
>>> Hui> +  struct ctf_save_collect_s *next;
>>> Hui> +  char *str;
>>> Hui> +  char *ctf_str;
>>> Hui> +  int align_size;
>>> Hui> +  struct expression *expr;
>>> Hui> +  struct type *type;
>>> Hui> +  int is_ret;
>>> Hui> +};
>>>
>>> Like Hafiz said -- comments would be nice.
>>
>> I added some comments in the new patches.
>>
>>>
>>> Hui> +static void
>>> Hui> +ctf_save_fwrite (FILE *fd, const gdb_byte *buf, size_t size)
>>> Hui> +{
>>> Hui> +  if (fwrite (buf, size, 1, fd) != 1)
>>> Hui> +    error (_("Unable to write file for saving trace data (%s)"),
>>> Hui> +     safe_strerror (errno));
>>>
>>> Why not use the existing ui_file code?
>>>
>>> Then you could remove this function plus several others.
>>>
>>> Maybe it is because you need fseek, but that seems like a simple
>>> addition to ui_file.
>>
>> I still not update this part because fseek patch is still not OK.
>> And after discussion with Pedro, I was really worry about change to
>> ui_file will make CTF write doesn't have error check.  Could you help
>> me with it?
>>
>>>
>>> Hui> +    case TYPE_CODE_ARRAY:
>>> Hui> +      for (; TYPE_CODE (type) == TYPE_CODE_ARRAY;
>>> Hui> +     type = TYPE_TARGET_TYPE (type))
>>> Hui> +  ;
>>>
>>> You probably want some check_typedef calls in there.
>>
>> Because typedef will be handle as a type in this part, so this part
>> doesn't need check_typedef.
>>
>>>
>>> Hui> +  ctf_save_type_name_write (tcsp, TYPE_FIELD_TYPE (type, biggest_id));
>>>
>>> Here too.
>>
>> I think this part is same with array.
>>
>>>
>>> Hui> +      ctf_save_fwrite_format (tcsp->metadata_fd, "%s", TYPE_NAME (type));
>>>
>>> What if TYPE_NAME is NULL?
>>
>> Add code handle it  like TYPE_CODE_STRUCT.
>>
>>>
>>> Hui> +static void
>>> Hui> +ctf_save_type_size_write (struct ctf_save_s *tcsp, struct type *type)
>>> Hui> +{
>>> Hui> +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
>>> Hui> +    {
>>> Hui> +      for (; TYPE_CODE (type) == TYPE_CODE_ARRAY;
>>> Hui> +     type = TYPE_TARGET_TYPE (type))
>>>
>>> check_typedef
>>
>> This is function will call itself to post all the define of type to file.
>> So It don't need check_typedef.
>>
>>>
>>> Hui> +  if (TYPE_NAME (type) && (strcmp (TYPE_NAME (type), "uint32_t") == 0
>>> Hui> +                     || strcmp (TYPE_NAME (type), "uint64_t") == 0))
>>> Hui> +    return;
>>>
>>> check_typedef.
>>>
>>> Also it seems like this clause should go in the TYPE_CODE_INT case.
>>>
>>> Hui> +
>>> Hui> +  switch (TYPE_CODE (type))
>>> Hui> +    {
>>> Hui> +      case TYPE_CODE_TYPEDEF:
>>> Hui> +  ctf_save_fwrite_format (tcsp->metadata_fd, "typedef ");
>>> Hui> +  ctf_save_var_define_write (tcsp, TYPE_TARGET_TYPE (type),
>>>
>>> check_typedef; though if your intent is to peel just a single layer,
>>> then it is a bit trickier -- I think the best you can do is always call
>>> it, then use TYPE_TARGET_TYPE if it is non-NULL or the result of
>>> check_typedef otherwise.
>>
>> If use check_typedef, this part will generate the define that
>> different with the type descriptor of the code.
>>
>> For example:
>> Following is the define in the code:
>> typedef char test_t1;
>> typedef test_t1 test_t2;
>> typedef test_t2 test_t3;
>>
>> If use TYPE_TARGET_TYPE, it will generate following metadata:
>> typedef char test_t1;
>> typedef test_t1 test_t2;
>> typedef test_t2 test_t3;
>>
>> If use check_typedef, it will generate following metadata:
>> typedef char test_t1;
>> typedef char test_t2;
>> typedef char test_t3;
>>
>>>
>>> Hui> +    tcsp->tab = tab;
>>> [...]
>>> Hui> +    tcsp->tab = old_tab;
>>>
>>> No idea if it matters, but if an exception is thrown during the '...'
>>> code, then the 'tab' field will be left set improperly.
>>
>> Please don't worry about this part.
>> This tab always be set to local value in stack.  So even if it is
>> drop, it will not affect anything.
>>
>> For example:
>> char tab[256];
>> const char *old_tab;
>>
>> old_tab = tcsp->tab;
>> snprintf (tab, 256, "%s\t", old_tab);
>> tcsp->tab = tab;
>> [...]
>> tcsp->tab = old_tab;
>>
>>>
>>> Hui> +      case TYPE_CODE_PTR:
>>> Hui> +  align_size = TYPE_LENGTH (type);
>>> Hui> +  break;
>>>
>>> Surely the alignment rules are ABI dependent.
>>> I would guess that what you have will work in many cases, but definitely
>>> not all of them.
>>
>> All the type will be handle and record in function
>> ctf_save_type_check_and_write.
>> The size align will be handle in this function too.
>>
>>>
>>> Hui> +    frame = get_current_frame ();
>>> Hui> +    if (!frame)
>>> Hui> +      error (_("get current frame fail"));
>>> Hui> +    frame = get_prev_frame (frame);
>>> Hui> +    if (!frame)
>>> Hui> +      error (_("get prev frame fail"));
>>>
>>> These messages could be improved.
>>>
>>> Hui> +      warning (_("\
>>> Hui> +Not save \"%s\" of tracepoint %d to ctf file because get its value fail: %s"),
>>> Hui> +         str, tps->tp->base.number, e.message);
>>>
>>> Likewise.
>>
>> Could you help me with this part?  :)
>>
>>>
>>> Hui> +      /* XXX: no sure about variable_length
>>> Hui> +   and need set is_variable_length if need.  */
>>> Hui> +      collect->align_size = align_size;
>>> Hui> +      if (collect->align_size > tps->align_size)
>>> Hui> +        tps->align_size = collect->align_size;
>>>
>>> No new FIXME comments.
>>> Can you find the answer to this question and either fix the code or drop
>>> the comment?
>>
>> Fixed.
>>
>>>
>>> Hui> +  char name[strlen (print_name) + 1];
>>>
>>> I think you need an explicit alloca here.
>>> Or xmalloc + xfree, which is probably better.
>>
>> Fixed.
>>
>>>
>>> Although, this approach just seems weird, since it seems like you
>>> already have the symbol and you want its value; constructing and parsing
>>> an expression to get this is very roundabout.
>>>
>>> I'm not sure I really understand the goal here; but the parsing approach
>>> is particularly fragile if you have shadowing.
>>
>> Function ctf_save_collect_get will parse the collect string and add
>> them to struct.
>> Each tracepoint will call this function just once.
>>
>> The code that try to get the value is in function ctf_save:
>>               back_chain = make_cleanup (null_cleanup, NULL);
>>               TRY_CATCH (e, RETURN_MASK_ERROR)
>>                 {
>>                   val = evaluate_expression (collect->expr);
>>                   content = value_contents (val);
>>                 }
>> Could you tell me some better way?
>>
>>>
>>> Hui> +        iterate_over_block_local_vars (block,
>>> Hui> +                                       tsv_save_do_loc_arg_collect,
>>> Hui> +                                       &cb_data);
>>>
>>> Why just iterate over this block and not the enclosing ones as well?
>>>
>>> Hmm, a lot of this code looks like code from tracepoint.c.
>>> I think it would be better to share the code if that is possible.
>>
>> I tried to share code with function add_local_symbols.  But it is not
>> a big function and use different way to get block.
>>
>>>
>>> Hui> +static char *
>>> Hui> +ctf_save_metadata_change_char (struct ctf_save_s *tcsp, char *ctf_str,
>>> Hui> +                         int i, const char *str)
>>> Hui> +{
>>> Hui> +  char *new;
>>> Hui> +
>>> Hui> +  new = xmalloc (strlen (ctf_str) + (i ? 1 : 0) + strlen (str) + 1 - 1);
>>> Hui> +  ctf_str[i] = '\0';
>>> Hui> +  sprintf (new, "%s%s%s_%s", ctf_str, (i ? "_" : ""), str, ctf_str + i + 1);
>>>
>>> Just use xstrprintf.
>>
>> Fixed.
>>
>>>
>>> Hui> +static void
>>> Hui> +ctf_save_do_nothing (void *p)
>>> Hui> +{
>>> Hui> +}
>>>
>>> Use null_cleanup instead.
>>
>> Fixed.
>>
>>>
>>> Hui> +    if (collect->expr)
>>> Hui> +      free_current_contents (&collect->expr);
>>>
>>> Why free_current_contents here?
>>> That seems weird.
>>
>> If this collect is $_ret, it will not have collect->expr.
>> Or maybe this collect will be free because when setup this collect get error.
>> So check it before free it.
>>
>>>
>>> Hui> +  char file_name[strlen (dirname) + 1 + strlen (CTF_DATASTREAM_NAME) + 1];
>>>
>>> alloca or malloc.
>>
>> Fixed.
>>
>>>
>>> Hui> +  sprintf (file_name, "%s/%s", dirname, CTF_METADATA_NAME);
>>> Hui> +  tcs.metadata_fd = fopen (file_name, "w");
>>> Hui> +  if (!tcs.metadata_fd)
>>> Hui> +    error (_("Unable to open file '%s' for saving trace data (%s)"),
>>> Hui> +     file_name, safe_strerror (errno));
>>> Hui> +  ctf_save_metadata_header (&tcs);
>>> Hui> +
>>> Hui> +  sprintf (file_name, "%s/%s", dirname, CTF_DATASTREAM_NAME);
>>> Hui> +  tcs.datastream_fd = fopen (file_name, "w");
>>> Hui> +  if (!tcs.datastream_fd)
>>> Hui> +    error (_("Unable to open file '%s' for saving trace data (%s)"),
>>> Hui> +     file_name, safe_strerror (errno));
>>>
>>> On error these files will be left partially written.
>>> Is that intentional?
>>
>> What my thought about this part is even if GDB get error when CTF
>> save, the data before this error is OK.  So in clean up function
>> ctf_save_cleanup, it will not remove this file.  And it will write the
>> data that don't have error, function ctf_save_metadata (tcsp).
>>
>>>
>>> Hui> +extern void ctf_save (char *dirname);
>>>
>>> Why not const?
>>> This applies in a few spots in the patch.
>>
>> Fixed.
>>
>>>
>>> Hui> @@ -2465,6 +2466,7 @@ void
>>> Hui>  mi_cmd_trace_save (char *command, char **argv, int argc)
>>> [...]
>>> Hui> +      if (strcmp (argv[0], "-ctf") == 0)
>>> Hui> +  generate_ctf = 1;
>>>
>>> The 'usage' line needs an update.
>>
>> Fixed.
>>
>>>
>>> Hui> +  if (generate_ctf)
>>> Hui> +    ctf_save (filename);
>>> Hui> +  else
>>> Hui> +    trace_save (filename, target_saves);
>>>
>>> I don't understand why CTF isn't just an option to trace_save -- share
>>> the trace-dependent work, separate out the formatting.
>>>
>>
>> I separate read and write CTF support function because:
>> CTF format is a complex format.  I tried to support all of it but I failed.
>> Then I changed to use libbabeltrace, it works very good with read CTF.
>>  But it doesn't supply API for CTF write.  And I discussion the
>> developer of libbabeltrace, they said they don't have plan for that.
>> So I add CTF write function inside GDB with my hand.  And because CTF
>> is design for tracepoint support.  So it is really fit with tsave
>> command.  So I add it as an option.
>>
>>> Hui>  trace_save_command (char *args, int from_tty)
>>> Hui>  {
>>> Tom
>>
>> Thanks for your comments.  I post a new version.
>>
>> Best,
>> Hui
>
> Hi Tom,
>
> I post a  patch that updated according to the update of trunk.
>
> Thanks,
> Hui
>
>
> 2012-12-18  Hui Zhu  <hui_zhu@mentor.com>
>
>         * Makefile.in (REMOTE_OBS): Add ctf.o.
>         (SFILES): Add ctf.c.
>         (HFILES_NO_SRCDIR): Add ctf.h.
>         * ctf.c, ctf.h: New files.
>         * mi/mi-main.c (ctf.h): New include.
>         (mi_cmd_trace_save): Add "-ctf".
>         * tracepoint.c (ctf.h): New include.
>         (collect_pseudocommand): Remove static.
>         (trace_save_command): Add "-ctf".
>         (_initialize_tracepoint): Ditto.
>         * tracepoint.h (collect_pseudocommand): Add extern.

Hi Tom,

When I tried to fixed the format issue of target ctf patch, I found
that this patch has some format issue too.
So I post a new version that fixed these issues.

Thanks,
Hui

2012-12-20  Hui Zhu  <hui_zhu@mentor.com>

	* Makefile.in (REMOTE_OBS): Add ctf.o.
	(SFILES): Add ctf.c.
	(HFILES_NO_SRCDIR): Add ctf.h.
	* ctf.c, ctf.h: New files.
	* mi/mi-main.c (ctf.h): New include.
	(mi_cmd_trace_save): Add "-ctf".
	* tracepoint.c (ctf.h): New include.
	(collect_pseudocommand): Remove static.
	(trace_save_command): Add "-ctf".
	(_initialize_tracepoint): Ditto.
	* tracepoint.h (collect_pseudocommand): Add extern.

Attachment: tsave-ctf.txt
Description: Text document