From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16643 invoked by alias); 18 Jan 2013 14:29:50 -0000 Received: (qmail 16634 invoked by uid 22791); 18 Jan 2013 14:29:49 -0000 X-SWARE-Spam-Status: No, hits=-4.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,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; Fri, 18 Jan 2013 14:29:42 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1TwCwu-0007Pd-SU from Hafiz_Abid@mentor.com ; Fri, 18 Jan 2013 06:29:40 -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); Fri, 18 Jan 2013 06:29:40 -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; Fri, 18 Jan 2013 14:29:38 +0000 Date: Fri, 18 Jan 2013 14:29:00 -0000 From: Hafiz Abid Qadeer 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 Fri Jan 18 01:16:24 2013) Message-ID: <1358519377.21794.2@abidh-ubunto1104> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; 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-01/txt/msg00423.txt.bz2 On 18/01/13 01:16:24, Hui Zhu wrote: > Hi Abid, >=20 > Thanks for your review. >=20 > 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 -ctf=20=20 > 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. >=20 > 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. >=20 I made an array of size 5 and gave it elements values from 5 to 9. I=20=20 collected this array in trace. After trace was finished, GDB will show=20=20 correct values of all the array elements. But in babeltrace, the first=20=20 element would have value of 6 and last will have a garbage value. So it=20= =20 looked that values are off by one index. For bitfield, I had a structure like this and I observed that value of=20=20 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. > > 3. When I use while-stepping on tracepoints actions, I see some=20=20 > error in the babeltrace. >=20 > Fixed. And I think it is a good idea for test. So I updated test for > this issue. >=20 > > 4. It looks that TYPE_CODE_FLT is not supported which cause the=20=20 > following warning when I use collect $reg on the tracepoint actions. > > "warning: error saving tracepoint 2 "$st0" to CTF file: type is not=20= =20 > support." >=20 > Yes. current patch is still not support all the type of GDB. >=20 > > > > Below are some comments on the code. I see many tab characters in=20=20 > the patch. It may be problem in my editor but something to keep an=20=20 > eye on. > > > >>+#define CTF_PACKET_SIZE 4096 > > It may be my ignorance but is this size sufficient? Should it be=20=20 > possible to increase the limit using some command? >=20 > 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=20= =20 you may want to implement in future. >=20 > > > >>+ /* 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.=20=20 > Probably they need a more helpful comment. > > >=20 > 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; >=20 > >> +error saving tracepoint %d \"%s\" to CTF file: type is not=20=20 > support."), > > 'supported' instead of 'support'. >=20 > Fixed. >=20 > > > >>+ 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=20=20 > this file. >=20 > 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=20=20 > (collect->ctf_str, > >>+ i,=20=20 > "dollar"); > > This will change expression like $eip in gdb to dollar_eip in ctf.=20=20 > Does CTF forbid these characters? >=20 > No. In that case, the question will be why we do this change from $eip to=20=20 dollar_eip. >=20 > > > >>+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 think it=20= =20 > can be passed directly to the ctf_save_collect_get_1. >=20 > 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=20= =20 argument can be changed to const. >=20 > > > >>+ 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 in=20=20 > the tmp local variable which is not used after the loop. >=20 > Fixed. >=20 > > > >>+\"%s\" of tracepoint %d rename to \"%s\" in CTF file."), > > I think 'is renamed' will be better instead of rename here. >=20 > Fixed. >=20 > > > >>+ if (try_count > 1 || 4 + 4 + 4 =3D=3D tcs.content_size) > > what is the significance of this 4 + 4 + 4 >=20 > Change it to CONTENT_HEADER_SIZE >=20 > > > >>+traceframe %d of tracepoint %d need save data that bigger than=20=20 > packet size %d.\n\ > > should be "needs to save data that is bigger than the packet size" >=20 > Fixed. >=20 > > > >>+traceframe %d is dropped because try to get the value of \"%s\"=20=20 > got error: %s"), > > This probably needs to re-phrased. >=20 > Fixed. >=20 > > > > Also many comments can be improved grammatically. This will make=20=20 > them easier to understand. Please let me know if I need any help=20=20 > there. > > > > Thanks, > > Abid >=20 > Post a new version according to your comments. >=20 > Thanks, > Hui >=20 > 2013-01-18 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. > (collect_pseudocommand): Remove static. > (trace_save_command): Add "-ctf". > (_initialize_tracepoint): Ditto. > * tracepoint.h (stack.h): New include. > (collect_pseudocommand): Add extern. >=20 > > > > ________________________________________ > > From: gdb-patches-owner@sourceware.org=20=20 > [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" to=20=20 > 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=20=20 > *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 (type)) =3D=3D= =20=20 > 0)) > > return; > > } > > > > Thanks, > > Hui > > > > On Tue, Jan 8, 2013 at 9:40 AM, Hui Zhu 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 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, so=20=20 > 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 do is=20=20 > 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 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=20=20 > 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=20=20 > instead use > >>> the result of check_typedef. This means the stub had to resolve=20=20 > 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 (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 =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 cases,=20=20 > 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, 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=20=20 > like you > >>> Tom> already have the symbol and you want its value; constructing=20= =20 > 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=20=20 > parsing approach > >>> Tom> is particularly fragile if you have shadowing. > >>> > >>> Hui> Function ctf_save_collect_get will parse the collect string=20=20 > 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=20=20 > 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 tracepoint.c. > >>> Tom> I think it would be better to share the code if that is=20=20 > possible. > >>> > >>> Hui> I tried to share code with function add_local_symbols. But=20=20 > 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=20=20 > 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.=20=20= =20 > 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 > >> > >> * 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