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

[Hafiz Abid Qadeer <hafiz_abid at mentor dot com>]

On 18/01/13 01:16:24, Hui Zhu wrote:
> Hi Abid,
>
> Thanks for your review.
>
> On Mon, Jan 14, 2013 at 10:27 PM, Abid, Hafiz <Hafiz_Abid@mentor.com>  
> wrote:
> > 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.
>
> Could you give me some example about this 2 issues?
> And I just fixed some type issue with while-stepping.  I think maybe
> they were fixed in the new patch.
I made an array of size 5 and gave it elements values from 5 to 9. I  
collected this array in trace. After trace was finished, GDB will show  
correct values of all the array elements. But in babeltrace, the first  
element would have value of 6 and last will have a garbage value. So it  
looked that values are off by one index.

For bitfield, I had a structure like this and I observed that value of  
b was not correct in babeltrace.
struct test_main
{
        int a;
        int b: 16;
        int c: 16;
};

I will send you my test application offline.

> > 3. When I use while-stepping on tracepoints actions, I see some  
> error in the babeltrace.
>
> Fixed.  And I think it is a good idea for test.  So I updated test for
> this issue.
>
> > 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."
>
> Yes.  current patch is still not support all the type of GDB.
>
> >
> > 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?
>
> Yes, add a command to change current ctf_packet_size is a good idea.
> Do you mind I add it after CTF patch get commit?  Then we can keep
> focus on the current function of CTF patch.
I dont have any problem with fixed size. I was just giving an idea that  
you may want to implement in future.

> >
> >>+  /* 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.
> >
>
> I update it to:
>   /* This is the temp value of CONTENT_SIZE when GDB write a event to
>      CTF file.
>      If this event save success, CURRENT_CONTENT_SIZE will set to
>      CONTENT_SIZE.  */
>   size_t current_content_size;
>
> >> +error saving tracepoint %d \"%s\" to CTF file: type is not  
> support."),
> > 'supported' instead of 'support'.
>
> Fixed.
>
> >
> >>+            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.
>
> The size of file_name is alloca as the right size for both this
> string.  So I think this part doesn't need xsnprintf.
>   file_name = alloca (strlen (dirname) + 1
> 		      + strlen (CTF_DATASTREAM_NAME) + 1);
> >
> >>+                  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?
>
> No.
In that case, the question will be why we do this change from $eip to  
dollar_eip.

> >
> >>+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.
>
> This is because print_name is a const but ctf_save_collect_get_1's
> argument name need to be a string that is not a const.
> Added comments for that.
You probably would have done a cast or perhaps ctf_save_collect_get_1's  
argument can be changed to const.

> >
> >>+ 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.
>
> Fixed.
>
> >
> >>+\"%s\" of tracepoint %d rename to \"%s\" in CTF file."),
> > I think 'is renamed' will be better instead of rename here.
>
> Fixed.
>
> >
> >>+        if (try_count > 1 || 4 + 4 + 4 == tcs.content_size)
> > what is the significance of this 4 + 4 + 4
>
> Change it to CONTENT_HEADER_SIZE
>
> >
> >>+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"
>
> Fixed.
>
> >
> >>+traceframe %d is dropped because try to get the value of \"%s\"  
> got error: %s"),
> > This probably needs to re-phrased.
>
> Fixed.
>
> >
> > 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
>
> Post a new version according to your comments.
>
> Thanks,
> Hui
>
> 2013-01-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.
> 	* breakpoint.c (tracepoint_count): Remove static.
> 	* 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.
>
> >
> > ________________________________________
> > 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.