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

[Tom Tromey <tromey@redhat.com>]

>>>>> "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.

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.

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.

Hui> +	ctf_save_type_name_write (tcsp, TYPE_FIELD_TYPE (type, biggest_id));

Here too.

Hui> +      ctf_save_fwrite_format (tcsp->metadata_fd, "%s", TYPE_NAME (type));

What if TYPE_NAME is NULL?

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

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.

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.

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.

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.

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?

Hui> +  char name[strlen (print_name) + 1];

I think you need an explicit alloca here.
Or xmalloc + xfree, which is probably better.

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.

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.

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.

Hui> +static void
Hui> +ctf_save_do_nothing (void *p)
Hui> +{
Hui> +}

Use null_cleanup instead.

Hui> +	  if (collect->expr)
Hui> +	    free_current_contents (&collect->expr);

Why free_current_contents here?
That seems weird.

Hui> +  char file_name[strlen (dirname) + 1 + strlen (CTF_DATASTREAM_NAME) + 1];

alloca or malloc.

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?

Hui> +extern void ctf_save (char *dirname);

Why not const?
This applies in a few spots in the patch.

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.

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.

Hui>  trace_save_command (char *args, int from_tty)
Hui>  {
Tom