From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31618 invoked by alias); 20 Feb 2013 10:48:16 -0000 Received: (qmail 31571 invoked by uid 22791); 20 Feb 2013 10:48:12 -0000 X-SWARE-Spam-Status: No, hits=-4.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,MIME_QP_LONG_LINE,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_CP X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 20 Feb 2013 10:48:05 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1U87DY-0000OV-BD from Hafiz_Abid@mentor.com ; Wed, 20 Feb 2013 02:48:04 -0800 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Wed, 20 Feb 2013 02:48:04 -0800 Received: from abidh-ubunto1104 (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server (TLS) id 14.1.289.1; Wed, 20 Feb 2013 10:48:01 +0000 Date: Wed, 20 Feb 2013 10:48:00 -0000 From: "Abid, Hafiz" Subject: Re: [PATCH] Add CTF support to GDB [1/4] Add "-ctf" to tsave command To: Hui Zhu CC: "Abid, Hafiz" , Tom Tromey , "Zhu, Hui" , "gdb-patches@sourceware.org" In-Reply-To: (from teawater@gmail.com on Tue Feb 19 07:05:01 2013) Message-ID: <1361357280.2259.2@abidh-ubunto1104> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8"; delsp=Yes; format=Flowed Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-02/txt/msg00529.txt.bz2 Last time, I checked, the code has some build issues. Now they are=20=20 fixed. I did a little testing and it seems to work fine. You will have=20=20 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. >=20 > Thanks, > Hui >=20 > 2013-02-19 Hui Zhu >=20 > * 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. >=20 > On Mon, Feb 11, 2013 at 8:53 PM, Hui Zhu wrote: > > On Tue, Feb 5, 2013 at 6:51 AM, Hui Zhu wrote: > >> On Mon, Feb 4, 2013 at 11:33 PM, Abid, Hafiz=20=20 > wrote: > >>> Hi Hui, > >>> I tested the latest patch. I get some build error due to=20=20 > uninitialized local variables. > >>> ../../gdb/gdb/ctf.c: In function =E2=80=98ctf_save_collect_get_1=E2= =80=99: > >>> ../../gdb/gdb/ctf.c:636:21: error: =E2=80=98type=E2=80=99 may be used= =20=20 > uninitialised in this function [-Werror=3Duninitialized] > >>> ../../gdb/gdb/ctf.c: In function =E2=80=98ctf_save_collect_get=E2=80= =99: > >>> ../../gdb/gdb/ctf.c:734:28: error: =E2=80=98pc=E2=80=99 may be used u= ninitialised=20=20 > in this function [-Werror=3Duninitialized] > >>> ../../gdb/gdb/ctf.c: In function =E2=80=98ctf_save_tp_find=E2=80=99: > >>> ../../gdb/gdb/ctf.c:823:7: error: =E2=80=98pc=E2=80=99 may be used un= initialised=20=20 > in this function [-Werror=3Duninitialized] > >>> ../../gdb/gdb/ctf.c: In function =E2=80=98ctf_save=E2=80=99: > >>> ../../gdb/gdb/ctf.c:1323:33: error: =E2=80=98content=E2=80=99 may be = used=20=20 > uninitialised in this function [-Werror=3Duninitialized] > >>> ../../gdb/gdb/ctf.c:1307:56: error: =E2=80=98val=E2=80=99 may be used= =20=20 > uninitialised in this function [-Werror=3Duninitialized] > >>> > >>> After fixing that, I can see that array and while-stepping are=20=20 > working OK. As I understand, bitfields are not yet supported in=20=20 > 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=20=20 > issues. > >> > >> Best, > >> Hui > > > > Hi, > > > > Ping. > > > > Thanks, > > Hui > > > >> > >> 2013-02-05 Hui Zhu > >> > >> * 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=20=20 > 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 > >>> > >>> * 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 > >>> 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=20=20 > > >>>>> wrote: > >>>>> > Hi Hui, > >>>>> > I tested your patch and found a few problems. I used 'tsave=20=20 > -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=20=20 > bitfields. > >>>>> > >>>>> Could you give me some example about this 2 issues? > >>>>> And I just fixed some type issue with while-stepping. I think=20=20 > maybe > >>>>> they were fixed in the new patch. > >>>>> > >>>> I made an array of size 5 and gave it elements values from 5 to=20=20 > 9. I > >>>> collected this array in trace. After trace was finished, GDB=20=20 > will show > >>>> correct values of all the array elements. But in babeltrace, the=20= =20 > first > >>>> element would have value of 6 and last will have a garbage=20=20 > value. So it > >>>> looked that values are off by one index. > >>>> > >>>> For bitfield, I had a structure like this and I observed that=20=20 > 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=20=20 > 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=20=20 > some error in > >>>>> > the babeltrace. > >>>>> > >>>>> Fixed. And I think it is a good idea for test. So I updated=20=20 > test for > >>>>> this issue. > >>>>> > >>>>> > 4. It looks that TYPE_CODE_FLT is not supported which cause=20=20 > the > >>>>> > following warning when I use collect $reg on the tracepoint=20=20 > actions. > >>>>> > "warning: error saving tracepoint 2 "$st0" to CTF file: type=20=20 > 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=20=20 > characters in the > >>>>> > patch. It may be problem in my editor but something to keep=20=20 > an eye on. > >>>>> > > >>>>> >>+#define CTF_PACKET_SIZE 4096 > >>>>> > It may be my ignorance but is this size sufficient? Should it=20= =20 > be > >>>>> > possible to increase the limit using some command? > >>>>> > >>>>> Yes, add a command to change current ctf_packet_size is a good=20=20 > idea. > >>>>> Do you mind I add it after CTF patch get commit? Then we can=20=20 > keep > >>>>> focus on the current function of CTF patch. > >>>> > >>>> I dont have any problem with fixed size. I was just giving an=20=20 > 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=20=20 > 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=20=20 > variables. > >>>>> > Probably they need a more helpful comment. > >>>>> > > >>>>> > >>>>> I update it to: > >>>>> /* This is the temp value of CONTENT_SIZE when GDB write a=20=20 > event to > >>>>> CTF file. > >>>>> If this event save success, CURRENT_CONTENT_SIZE will set=20=20 > to > >>>>> CONTENT_SIZE. */ > >>>>> size_t current_content_size; > >>>>> > >>>>> >> +error saving tracepoint %d \"%s\" to CTF file: type is not=20=20 > 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=20=20 > 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 =3D alloca (strlen (dirname) + 1 > >>>>> + strlen (CTF_DATASTREAM_NAME) + 1); > >>>>> > > >>>>> >>+ case '$': > >>>>> >>+ collect->ctf_str > >>>>> >>+ =3D ctf_save_metadata_change_char > >>>>> >> (collect->ctf_str, > >>>>> >>+ i,=20=20 > "dollar"); > >>>>> > This will change expression like $eip in gdb to dollar_eip in=20= =20 > ctf. Does > >>>>> > CTF forbid these characters? > >>>>> > >>>>> No. > >>>> > >>>> In that case, the question will be why we do this change from=20=20 > $eip to > >>>> dollar_eip. > >>> > >>> Oops, sorry for my mistake. CTF doesn't support this char like=20=20 > $ 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 =3D cb_data; > >>>>> >>+ char *name; > >>>>> >>+ > >>>>> >>+ name =3D 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=20=20 > think it can > >>>>> > be passed directly to the ctf_save_collect_get_1. > >>>>> > >>>>> This is because print_name is a const but=20=20 > 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=20=20 > ctf_save_collect_get_1's > >>>> argument can be changed to const. > >>>> > >>> > >>> Fixed. > >>> > >>>> > >>>>> > >>>>> > > >>>>> >>+ tmp =3D alloca (strlen (collect->ctf_str) + 30); > >>>>> >>+ strcpy (tmp, collect->ctf_str); > >>>>> >>+ while (1) > >>>>> >>+ { > >>>>> >>+ struct ctf_save_collect_s *collect2; > >>>>> >>+ int i =3D 0; > >>>>> >>+ > >>>>> >>+ for (collect2 =3D tps->collect; collect2; > >>>>> >>+ collect2 =3D collect2->next) > >>>>> >>+ { > >>>>> >>+ if (collect2->ctf_str > >>>>> >>+ && strcmp (collect2->ctf_str, tmp) =3D=3D 0) > >>>>> >>+ break; > >>>>> >>+ } > >>>>> >>+ if (collect2 =3D=3D 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=20= =20 > 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 =3D=3D 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=20=20 > than packet > >>>>> >> size %d.\n\ > >>>>> > should be "needs to save data that is bigger than the packet=20=20 > size" > >>>>> > >>>>> Fixed. > >>>>> > >>>>> > > >>>>> >>+traceframe %d is dropped because try to get the value of=20=20 > \"%s\" got > >>>>> >> error: %s"), > >>>>> > This probably needs to re-phrased. > >>>>> > >>>>> Fixed. > >>>>> > >>>>> > > >>>>> > Also many comments can be improved grammatically. This will=20=20 > make them > >>>>> > easier to understand. Please let me know if I need any help=20=20 > there. > >>>>> > > >>>>> > Thanks, > >>>>> > Abid > >>>>> > >>>>> Post a new version according to your comments. > >>>>> > >>>>> Thanks, > >>>>> Hui > >>>>> > >>>>> 2013-01-18 Hui Zhu > >>>>> > >>>>> * 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=20=20 > [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"=20=20 > 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=20=20 > type *type) > >>>>> > { > >>>>> > struct ctf_save_type_s *t; > >>>>> > > >>>>> > for (t =3D tcsp->type; t; t =3D t->next) > >>>>> > { > >>>>> > if (t->type =3D=3D type > >>>>> > || (TYPE_NAME (t->type) && TYPE_NAME (type) > >>>>> > && strcmp (TYPE_NAME (t->type), TYPE_NAME=20=20 > (type)) =3D=3D 0)) > >>>>> > return; > >>>>> > } > >>>>> > > >>>>> > Thanks, > >>>>> > Hui > >>>>> > > >>>>> > On Tue, Jan 8, 2013 at 9:40 AM, Hui Zhu =20=20 > wrote: > >>>>> >> Hi Tom, > >>>>> >> > >>>>> >> Thanks for your review. > >>>>> >> > >>>>> >> On Fri, Jan 4, 2013 at 5:36 AM, Tom Tromey=20=20 > wrote: > >>>>> >>>>>>>> "Hui" =3D=3D Hui Zhu 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=20=20 > example, I > >>>>> >>> looked at this struct I quoted above. > >>>>> >>> > >>>>> >>> Every new structure, field, and function ought to have a=20=20 > comment. > >>>>> >> > >>>>> >> OK. I added comments for them in the new patch. > >>>>> >> > >>>>> >>> > >>>>> >>> > >>>>> >>> Hui> + case TYPE_CODE_ARRAY: > >>>>> >>> Hui> + for (; TYPE_CODE (type) =3D=3D TYPE_CODE_ARRAY; > >>>>> >>> Hui> + type =3D 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,=20= =20 > 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=20=20 > non-obvious > >>>>> >>> to > >>>>> >>> gdb developers. > >>>>> >>> > >>>>> >>> Tom> check_typedef; though if your intent is to peel just a=20= =20 > single > >>>>> >>> layer, > >>>>> >>> Tom> then it is a bit trickier -- I think the best you can=20=20 > do is > >>>>> >>> always call > >>>>> >>> Tom> it, then use TYPE_TARGET_TYPE if it is non-NULL or the=20= =20 > result of > >>>>> >>> Tom> check_typedef otherwise. > >>>>> >>> > >>>>> >>> Hui> If use check_typedef, this part will generate the=20=20 > 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=20=20 > using it > >>>>> >>> before > >>>>> >>> using TYPE_TARGET_TYPE. Otherwise with stubs I think you=20=20 > will see > >>>>> >>> crashes -- check_typedef is what sets this field. > >>>>> >>> > >>>>> >>> If you then use TYPE_TARGET_TYPE and get NULL, you ought to=20= =20 > instead > >>>>> >>> use > >>>>> >>> the result of check_typedef. This means the stub had to=20=20 > 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) =3D=3D TYPE_CODE_ARRAY; > >>>>> >> type =3D TYPE_TARGET_TYPE (type) ? TYPE_TARGET_TYPE= =20=20 > (type) > >>>>> >> : check_typedef=20=20 > (type)) > >>>>> >> ; > >>>>> >> > >>>>> >>> > >>>>> >>> Hui> If use TYPE_TARGET_TYPE, it will generate following=20=20 > 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 =3D 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=20=20 > 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 =3D get_current_frame (); > >>>>> >>> Hui> + if (!frame) > >>>>> >>> Hui> + error (_("get current frame fail")); > >>>>> >>> Hui> + frame =3D 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,=20=20 > can it? > >>>>> >>> > >>>>> >>> For the second error, how about "could not find previous=20=20 > frame"? > >>>>> >> > >>>>> >> Fixed. > >>>>> >> > >>>>> >>> > >>>>> >>> Hui> + warning (_("\ > >>>>> >>> Hui> +Not save \"%s\" of tracepoint %d to ctf file because=20=20 > 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=20=20 > seems like you > >>>>> >>> Tom> already have the symbol and you want its value;=20=20 > 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=20=20 > the parsing > >>>>> >>> approach > >>>>> >>> Tom> is particularly fragile if you have shadowing. > >>>>> >>> > >>>>> >>> Hui> Function ctf_save_collect_get will parse the collect=20=20 > 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=20=20 > the comments > >>>>> >> of ctf_save_collect_get to: > >>>>> >> /* Get var that want to collect from STR and put them to=20=20 > 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=20=20 > ctf_save_tp_s > >>>>> >> *tps, > >>>>> >> char *str) > >>>>> >> > >>>>> >> How about this? > >>>>> >> > >>>>> >>> > >>>>> >>> Tom> Hmm, a lot of this code looks like code from=20=20 > tracepoint.c. > >>>>> >>> Tom> I think it would be better to share the code if that=20=20 > is possible. > >>>>> >>> > >>>>> >>> Hui> I tried to share code with function=20=20 > 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=20=20 > 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=20=20 > collect->expr. Or > >>>>> >>> maybe > >>>>> >>> Hui> this collect will be free because when setup this=20=20 > 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 > >>>>> >> > >>>>> >> * 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. > >>>>> > >>>> >=20