This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.
>
> 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 have 2 questions about use ui_file:
1. I can add new interface about fseek to ui_file?
2. I found that write functions of stdio_file don't check write fail.
Is that right, or I miss something?
For example:
  /* Calling error crashes when we are called from the exception framework.  */
  if (fwrite (buf, length_buf, 1, stdio->file))
    {
      /* Nothing.  */
    }

Thanks,
Hui

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]