This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Add CTF support to GDB [1/4] Add "-ctf" to tsave command
- From: Tom Tromey <tromey at redhat dot com>
- To: Hui Zhu <teawater at gmail dot com>
- Cc: Hui Zhu <hui_zhu at mentor dot com>, gdb-patches at sourceware dot org
- Date: Thu, 03 Jan 2013 14:36:21 -0700
- Subject: Re: [PATCH] Add CTF support to GDB [1/4] Add "-ctf" to tsave command
- References: <50AC3217.6040608@mentor.com> <878v9k5g46.fsf@fleche.redhat.com> <CANFwon0irfa-iHcF0a8=sAzPfa1TCeu2UdsDvY785Z7+se8mYg@mail.gmail.com>
>>>>> "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.
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.
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.
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"?
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".
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.
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.
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.
Tom