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


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.

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

>
>>+  /* 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.

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

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

Attachment: tsave-ctf.txt
Description: Text document


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