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

["Abid, Hafiz" <Hafiz_Abid at mentor dot com>]

Hi Hui,
I tested your patch and found a few problems. I used 'tsave -ctf output' and then used babeltrace to get a text dump of the output. 

1. In case of array, the tracing results are off by one.
2. Struct members values are not shown correctly in case of bitfields.
3. When I use while-stepping on tracepoints actions, I see some error in the babeltrace.
4. It looks that TYPE_CODE_FLT is not supported which cause the following warning when I use collect $reg on the tracepoint actions.
"warning: error saving tracepoint 2 "$st0" to CTF file: type is not support."

Below are some comments on the code. I see many tab characters in the patch. It may be problem in my editor but something to keep an eye on.

>+#define CTF_PACKET_SIZE		4096
It may be my ignorance but is this size sufficient? Should it be possible to increase the limit using some command?

>+  /* This is the content size of current packet.  */
>+  size_t content_size;
...
>+  /* This is the content size of current packet and event that is
>+     being written to file.
>+     Check size use it.  */
>+  size_t current_content_size;
I don't fully understand the difference between these 2 variables. Probably they need a more helpful comment.

> +error saving tracepoint %d \"%s\" to CTF file: type is not support."),
'supported' instead of 'support'.

>+	      sprintf (regname, "$%s", name);
>+  sprintf (file_name, "%s/%s", dirname, CTF_METADATA_NAME);
>+  sprintf (file_name, "%s/%s", dirname, CTF_DATASTREAM_NAME);
Please use xsnprintf. There are also a bunch of snprintf calls in this file.

>+		    case '$':
>+		      collect->ctf_str
>+			  = ctf_save_metadata_change_char (collect->ctf_str,
>+							   i, "dollar");
This will change expression like $eip in gdb to dollar_eip in ctf. Does CTF forbid these characters?

>+static void
>+tsv_save_do_loc_arg_collect (const char *print_name,
>+			     struct symbol *sym,
>+			     void *cb_data)
>+{
>+  struct loc_arg_collect_data *p = cb_data;
>+  char *name;
>+
>+  name = alloca (strlen (print_name) + 1);
>+  strcpy (name, print_name);
>+  ctf_save_collect_get_1 (p->tcsp, p->tps, name);
>+}
Is there any real need to make a copy of the print_name? I think it can be passed directly to the ctf_save_collect_get_1.

>+ tmp = alloca (strlen (collect->ctf_str) + 30);
>+	  strcpy (tmp, collect->ctf_str);
>+	  while (1)
>+	    {
>+	      struct ctf_save_collect_s *collect2;
>+	      int i = 0;
>+
>+	      for (collect2 = tps->collect; collect2;
>+		   collect2 = collect2->next)
>+		{
>+		  if (collect2->ctf_str
>+		      && strcmp (collect2->ctf_str, tmp) == 0)
>+		    break;
>+		}
>+	      if (collect2 == NULL)
>+		break;
>+
>+	      snprintf (tmp, strlen (collect->ctf_str) + 30,
>+			"%s_%d", collect->ctf_str, i++);
>+	    }
What is the purpose of this loop? It only writes a new string in the tmp local variable which is not used after the loop.

>+\"%s\" of tracepoint %d rename to \"%s\" in CTF file."),
I think 'is renamed' will be better instead of rename here.

>+	  if (try_count > 1 || 4 + 4 + 4 == tcs.content_size)
what is the significance of this 4 + 4 + 4

>+traceframe %d of tracepoint %d need save data that bigger than packet size %d.\n\
should be "needs to save data that is bigger than the packet size"

>+traceframe %d is dropped because try to get the value of \"%s\" got error: %s"),
This probably needs to re-phrased.

Also many comments can be improved grammatically. This will make them easier to understand. Please let me know if I need any help there.

Thanks,
Abid

________________________________________
From: gdb-patches-owner@sourceware.org [gdb-patches-owner@sourceware.org] on behalf of Hui Zhu [teawater@gmail.com]
Sent: Monday, January 14, 2013 5:18 AM
To: Tom Tromey
Cc: Zhu, Hui; gdb-patches@sourceware.org
Subject: Re: [PATCH] Add CTF support to GDB [1/4] Add "-ctf" to tsave command

Hi Tom,

I found a bug when I use test to test this patch.
So I post a new version to fix this bug.
The change of this patch is change the same type check to:
static void
ctf_save_type_define_write (struct ctf_save_s *tcsp, struct type *type)
{
  struct ctf_save_type_s *t;

  for (t = tcsp->type; t; t = t->next)
    {
      if (t->type == type
          || (TYPE_NAME (t->type) && TYPE_NAME (type)
              && strcmp (TYPE_NAME (t->type), TYPE_NAME (type)) == 0))
        return;
    }

Thanks,
Hui

On Tue, Jan 8, 2013 at 9:40 AM, Hui Zhu <teawater@gmail.com> wrote:
> Hi Tom,
>
> Thanks for your review.
>
> On Fri, Jan 4, 2013 at 5:36 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>>
>> 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> I added some comments in the new patches.
>>
>> I looked at the new patches and did not see comments.  For example, I
>> looked at this struct I quoted above.
>>
>> Every new structure, field, and function ought to have a comment.
>
> OK.  I added comments for them in the new patch.
>
>>
>>
>> Hui> +    case TYPE_CODE_ARRAY:
>> Hui> +      for (; TYPE_CODE (type) == TYPE_CODE_ARRAY;
>> Hui> +     type = TYPE_TARGET_TYPE (type))
>> Hui> +  ;
>>
>> Tom> You probably want some check_typedef calls in there.
>>
>> Hui> Because typedef will be handle as a type in this part, so this part
>> Hui> doesn't need check_typedef.
>>
>> That seems peculiar to me, but I don't really know CTF.
>> In this case you need a comment, since the result will be non-obvious to
>> gdb developers.
>>
>> Tom> check_typedef; though if your intent is to peel just a single layer,
>> Tom> then it is a bit trickier -- I think the best you can do is always call
>> Tom> it, then use TYPE_TARGET_TYPE if it is non-NULL or the result of
>> Tom> check_typedef otherwise.
>>
>> Hui> If use check_typedef, this part will generate the define that
>> Hui> different with the type descriptor of the code.
>>
>> You need to call check_typedef before you can even examine
>> TYPE_TARGET_TYPE of a typedef.  This is what I meant by using it before
>> using TYPE_TARGET_TYPE.  Otherwise with stubs I think you will see
>> crashes -- check_typedef is what sets this field.
>>
>> If you then use TYPE_TARGET_TYPE and get NULL, you ought to instead use
>> the result of check_typedef.  This means the stub had to resolve to a
>> typedef in a different objfile.
>
> I change it to following part:
>     case TYPE_CODE_ARRAY:
>       /* This part just to get the real name of this array.
>          This part should keep typedef if it can.  */
>       for (; TYPE_CODE (type) == TYPE_CODE_ARRAY;
>            type = TYPE_TARGET_TYPE (type) ? TYPE_TARGET_TYPE (type)
>                                           : check_typedef (type))
>         ;
>
>>
>> Hui> If use TYPE_TARGET_TYPE, it will generate following metadata:
>> Hui> typedef char test_t1;
>> Hui> typedef test_t1 test_t2;
>> Hui> typedef test_t2 test_t3;
>>
>> I suppose there should be a test case doing this.
>
> OK. I will write a test for all this function.
>
>>
>> Hui> +      case TYPE_CODE_PTR:
>> Hui> +  align_size = TYPE_LENGTH (type);
>> Hui> +  break;
>>
>> Tom> Surely the alignment rules are ABI dependent.
>> Tom> I would guess that what you have will work in many cases, but definitely
>> Tom> not all of them.
>>
>> Hui> All the type will be handle and record in function
>> Hui> ctf_save_type_check_and_write.
>> Hui> The size align will be handle in this function too.
>>
>> I don't think this really addresses the issue.
>> Not all platforms use the alignment rules currently coded in
>> ctf_save_type_check_and_write.  But maybe it doesn't matter.
>>
>> 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"));
>> Tom>
>> Tom> These messages could be improved.
>>
>> Actually, I don't think get_current_frame can return NULL, can it?
>>
>> For the second error, how about "could not find previous frame"?
>
> Fixed.
>
>>
>> Hui> +      warning (_("\
>> Hui> +Not save \"%s\" of tracepoint %d to ctf file because get its
>> Hui> value fail: %s"),
>> Hui> +         str, tps->tp->base.number, e.message);
>> Tom>
>> Tom> Likewise.
>>
>> Hui> Could you help me with this part?  :)
>>
>> How about "error saving tracepoint %d to CTF file %s: %s".
>
> It is more better.  I updated them all.
>
>>
>> Tom> Although, this approach just seems weird, since it seems like you
>> Tom> already have the symbol and you want its value; constructing and parsing
>> Tom> an expression to get this is very roundabout.
>> Tom>
>> Tom> I'm not sure I really understand the goal here; but the parsing approach
>> Tom> is particularly fragile if you have shadowing.
>>
>> Hui> Function ctf_save_collect_get will parse the collect string and add
>> Hui> them to struct.
>> Hui> Each tracepoint will call this function just once.
>>
>> Ok, I don't know the answer here.
>
> I am sorry that this part is not very clear.  So I update the comments
> of ctf_save_collect_get to:
> /* Get var that want to collect from STR and put them to TPS->collect.
>    This function will not be call when GDB add a new TP.  */
>
> static void
> ctf_save_collect_get (struct ctf_save_s *tcsp, struct ctf_save_tp_s *tps,
>                       char *str)
>
> How about this?
>
>>
>> Tom> Hmm, a lot of this code looks like code from tracepoint.c.
>> Tom> I think it would be better to share the code if that is possible.
>>
>> Hui> I tried to share code with function add_local_symbols.  But it is not
>> Hui> a big function and use different way to get block.
>>
>> I wonder why, and whether this means that the different ways of saving
>> will in fact write out different data.
>
> I added function add_local_symbols_1 for that.
>
>>
>> Hui> +    if (collect->expr)
>> Hui> +      free_current_contents (&collect->expr);
>>
>> Tom> Why free_current_contents here?
>> Tom> That seems weird.
>>
>> Hui> If this collect is $_ret, it will not have collect->expr.  Or maybe
>> Hui> this collect will be free because when setup this collect get
>> Hui> error.  So check it before free it.
>>
>> You can just write  xfree (collect->expr).
>> You don't need a NULL check here.
>> This applies to all those xfree calls.
>>
>
> OK.  Fixed.
>
> I post a new version.  Please help me review it.
>
> Thanks,
> Hui
>
> 2013-01-08  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 (stack.h): New include.
>         (collect_pseudocommand): Add extern.