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

[Hui Zhu <teawater at gmail dot com>]

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

Attachment: tsave-ctf.txt
Description: Text document