[PATCH] Add CTF support to GDB [1/4] Add "-ctf" to tsave command
Abid, Hafiz
hafiz_abid@mentor.com
Wed Feb 20 10:48:00 GMT 2013
Last time, I checked, the code has some build issues. Now they are
fixed. I did a little testing and it seems to work fine. You will have
to wait for some Maintainer to review it now.
Regards,
Abid
On 19/02/13 07:05:01, Hui Zhu wrote:
> The old patch have some build issues with upstream. So I post a new
> version to handle it.
>
> Thanks,
> Hui
>
> 2013-02-19 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.
> (while_stepping_pseudocommand,
> collect_pseudocommand): Remove static.
> (trace_save_command): Add "-ctf".
> (_initialize_tracepoint): Ditto.
> * tracepoint.h (stack.h): New include.
> (while_stepping_pseudocommand,
> collect_pseudocommand): Add extern.
>
> On Mon, Feb 11, 2013 at 8:53 PM, Hui Zhu <teawater@gmail.com> wrote:
> > On Tue, Feb 5, 2013 at 6:51 AM, Hui Zhu <teawater@gmail.com> wrote:
> >> On Mon, Feb 4, 2013 at 11:33 PM, Abid, Hafiz
> <Hafiz_Abid@mentor.com> wrote:
> >>> Hi Hui,
> >>> I tested the latest patch. I get some build error due to
> uninitialized local variables.
> >>> ../../gdb/gdb/ctf.c: In function ‘ctf_save_collect_get_1’:
> >>> ../../gdb/gdb/ctf.c:636:21: error: ‘type’ may be used
> uninitialised in this function [-Werror=uninitialized]
> >>> ../../gdb/gdb/ctf.c: In function ‘ctf_save_collect_get’:
> >>> ../../gdb/gdb/ctf.c:734:28: error: ‘pc’ may be used uninitialised
> in this function [-Werror=uninitialized]
> >>> ../../gdb/gdb/ctf.c: In function ‘ctf_save_tp_find’:
> >>> ../../gdb/gdb/ctf.c:823:7: error: ‘pc’ may be used uninitialised
> in this function [-Werror=uninitialized]
> >>> ../../gdb/gdb/ctf.c: In function ‘ctf_save’:
> >>> ../../gdb/gdb/ctf.c:1323:33: error: ‘content’ may be used
> uninitialised in this function [-Werror=uninitialized]
> >>> ../../gdb/gdb/ctf.c:1307:56: error: ‘val’ may be used
> uninitialised in this function [-Werror=uninitialized]
> >>>
> >>> After fixing that, I can see that array and while-stepping are
> working OK. As I understand, bitfields are not yet supported in
> babeltrace. So that takes care of most of the issues I reported.
> >>>
> >>> Regards,
> >>> Abid
> >>
> >>
> >> Hi Abid,
> >>
> >> Thanks for your help. I just post a new version that fixed these
> issues.
> >>
> >> Best,
> >> Hui
> >
> > Hi,
> >
> > Ping.
> >
> > Thanks,
> > Hui
> >
> >>
> >> 2013-02-05 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: Hui Zhu [teawater@gmail.com]
> >>> Sent: Wednesday, January 23, 2013 1:32 PM
> >>> To: Abid, Hafiz
> >>> Cc: Tom Tromey; Zhu, Hui; gdb-patches@sourceware.org
> >>> Subject: Re: [PATCH] Add CTF support to GDB [1/4] Add "-ctf" to
> tsave command
> >>>
> >>> Hi Abid,
> >>>
> >>> I post a new version according to your comments.
> >>>
> >>> Following part have the reply for your comments.
> >>>
> >>> Thanks,
> >>> Hui
> >>>
> >>> 2013-01-23 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.
> >>>
> >>> On Fri, Jan 18, 2013 at 10:29 PM, Hafiz Abid Qadeer
> >>> <hafiz_abid@mentor.com> wrote:
> >>>> 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.
> >>>
> >>> Thanks. This issue is because old patch doesn't support
> bitfields. I
> >>> add them in the new patch. But babeltrace doesn't support gcc
> >>> bitfields. So I didn't update test for bitfields.
> >>>
> >>>>
> >>>>
> >>>>> > 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.
> >>>
> >>> Oops, sorry for my mistake. CTF doesn't support this char like
> $ or
> >>> something else.
> >>>
> >>>>
> >>>>
> >>>>>
> >>>>> >
> >>>>> >>+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.
> >>>>
> >>>
> >>> Fixed.
> >>>
> >>>>
> >>>>>
> >>>>> >
> >>>>> >>+ 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.
> >>>>>
> >>>>
>
More information about the Gdb-patches
mailing list