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

[Hui Zhu <teawater at gmail dot com>]

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