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


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




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