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] |
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. > 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. > >>+ /* 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. > >>+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. > >>+ 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.
Attachment:
tsave-ctf.txt
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |