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] |
On Fri, Dec 14, 2012 at 7:36 PM, Hui Zhu <teawater@gmail.com> wrote: > On Fri, Nov 30, 2012 at 4:06 AM, Tom Tromey <tromey@redhat.com> wrote: >>>>>>> "Hui" == Hui Zhu <hui_zhu@mentor.com> writes: >> >> Hui> This patch is for the CTF write. >> Hui> It add "-ctf" to tsave command. With this option, tsave can save >> Hui> current trace frame to CTF file format. >> >> 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. > > I added some comments in the new patches. > >> >> Hui> +static void >> Hui> +ctf_save_fwrite (FILE *fd, const gdb_byte *buf, size_t size) >> Hui> +{ >> Hui> + if (fwrite (buf, size, 1, fd) != 1) >> Hui> + error (_("Unable to write file for saving trace data (%s)"), >> Hui> + safe_strerror (errno)); >> >> Why not use the existing ui_file code? >> >> Then you could remove this function plus several others. >> >> Maybe it is because you need fseek, but that seems like a simple >> addition to ui_file. > > I still not update this part because fseek patch is still not OK. > And after discussion with Pedro, I was really worry about change to > ui_file will make CTF write doesn't have error check. Could you help > me with it? > >> >> Hui> + case TYPE_CODE_ARRAY: >> Hui> + for (; TYPE_CODE (type) == TYPE_CODE_ARRAY; >> Hui> + type = TYPE_TARGET_TYPE (type)) >> Hui> + ; >> >> You probably want some check_typedef calls in there. > > Because typedef will be handle as a type in this part, so this part > doesn't need check_typedef. > >> >> Hui> + ctf_save_type_name_write (tcsp, TYPE_FIELD_TYPE (type, biggest_id)); >> >> Here too. > > I think this part is same with array. > >> >> Hui> + ctf_save_fwrite_format (tcsp->metadata_fd, "%s", TYPE_NAME (type)); >> >> What if TYPE_NAME is NULL? > > Add code handle it like TYPE_CODE_STRUCT. > >> >> Hui> +static void >> Hui> +ctf_save_type_size_write (struct ctf_save_s *tcsp, struct type *type) >> Hui> +{ >> Hui> + if (TYPE_CODE (type) == TYPE_CODE_ARRAY) >> Hui> + { >> Hui> + for (; TYPE_CODE (type) == TYPE_CODE_ARRAY; >> Hui> + type = TYPE_TARGET_TYPE (type)) >> >> check_typedef > > This is function will call itself to post all the define of type to file. > So It don't need check_typedef. > >> >> Hui> + if (TYPE_NAME (type) && (strcmp (TYPE_NAME (type), "uint32_t") == 0 >> Hui> + || strcmp (TYPE_NAME (type), "uint64_t") == 0)) >> Hui> + return; >> >> check_typedef. >> >> Also it seems like this clause should go in the TYPE_CODE_INT case. >> >> Hui> + >> Hui> + switch (TYPE_CODE (type)) >> Hui> + { >> Hui> + case TYPE_CODE_TYPEDEF: >> Hui> + ctf_save_fwrite_format (tcsp->metadata_fd, "typedef "); >> Hui> + ctf_save_var_define_write (tcsp, TYPE_TARGET_TYPE (type), >> >> check_typedef; though if your intent is to peel just a single layer, >> then it is a bit trickier -- I think the best you can do is always call >> it, then use TYPE_TARGET_TYPE if it is non-NULL or the result of >> check_typedef otherwise. > > If use check_typedef, this part will generate the define that > different with the type descriptor of the code. > > For example: > Following is the define in the code: > typedef char test_t1; > typedef test_t1 test_t2; > typedef test_t2 test_t3; > > If use TYPE_TARGET_TYPE, it will generate following metadata: > typedef char test_t1; > typedef test_t1 test_t2; > typedef test_t2 test_t3; > > If use check_typedef, it will generate following metadata: > typedef char test_t1; > typedef char test_t2; > typedef char test_t3; > >> >> Hui> + tcsp->tab = tab; >> [...] >> Hui> + tcsp->tab = old_tab; >> >> No idea if it matters, but if an exception is thrown during the '...' >> code, then the 'tab' field will be left set improperly. > > Please don't worry about this part. > This tab always be set to local value in stack. So even if it is > drop, it will not affect anything. > > For example: > char tab[256]; > const char *old_tab; > > old_tab = tcsp->tab; > snprintf (tab, 256, "%s\t", old_tab); > tcsp->tab = tab; > [...] > tcsp->tab = old_tab; > >> >> Hui> + case TYPE_CODE_PTR: >> Hui> + align_size = TYPE_LENGTH (type); >> Hui> + break; >> >> Surely the alignment rules are ABI dependent. >> I would guess that what you have will work in many cases, but definitely >> not all of them. > > All the type will be handle and record in function > ctf_save_type_check_and_write. > The size align will be handle in this function too. > >> >> 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")); >> >> These messages could be improved. >> >> Hui> + warning (_("\ >> Hui> +Not save \"%s\" of tracepoint %d to ctf file because get its value fail: %s"), >> Hui> + str, tps->tp->base.number, e.message); >> >> Likewise. > > Could you help me with this part? :) > >> >> Hui> + /* XXX: no sure about variable_length >> Hui> + and need set is_variable_length if need. */ >> Hui> + collect->align_size = align_size; >> Hui> + if (collect->align_size > tps->align_size) >> Hui> + tps->align_size = collect->align_size; >> >> No new FIXME comments. >> Can you find the answer to this question and either fix the code or drop >> the comment? > > Fixed. > >> >> Hui> + char name[strlen (print_name) + 1]; >> >> I think you need an explicit alloca here. >> Or xmalloc + xfree, which is probably better. > > Fixed. > >> >> Although, this approach just seems weird, since it seems like you >> already have the symbol and you want its value; constructing and parsing >> an expression to get this is very roundabout. >> >> I'm not sure I really understand the goal here; but the parsing approach >> is particularly fragile if you have shadowing. > > Function ctf_save_collect_get will parse the collect string and add > them to struct. > Each tracepoint will call this function just once. > > The code that try to get the value is in function ctf_save: > back_chain = make_cleanup (null_cleanup, NULL); > TRY_CATCH (e, RETURN_MASK_ERROR) > { > val = evaluate_expression (collect->expr); > content = value_contents (val); > } > Could you tell me some better way? > >> >> Hui> + iterate_over_block_local_vars (block, >> Hui> + tsv_save_do_loc_arg_collect, >> Hui> + &cb_data); >> >> Why just iterate over this block and not the enclosing ones as well? >> >> Hmm, a lot of this code looks like code from tracepoint.c. >> I think it would be better to share the code if that is possible. > > I tried to share code with function add_local_symbols. But it is not > a big function and use different way to get block. > >> >> Hui> +static char * >> Hui> +ctf_save_metadata_change_char (struct ctf_save_s *tcsp, char *ctf_str, >> Hui> + int i, const char *str) >> Hui> +{ >> Hui> + char *new; >> Hui> + >> Hui> + new = xmalloc (strlen (ctf_str) + (i ? 1 : 0) + strlen (str) + 1 - 1); >> Hui> + ctf_str[i] = '\0'; >> Hui> + sprintf (new, "%s%s%s_%s", ctf_str, (i ? "_" : ""), str, ctf_str + i + 1); >> >> Just use xstrprintf. > > Fixed. > >> >> Hui> +static void >> Hui> +ctf_save_do_nothing (void *p) >> Hui> +{ >> Hui> +} >> >> Use null_cleanup instead. > > Fixed. > >> >> Hui> + if (collect->expr) >> Hui> + free_current_contents (&collect->expr); >> >> Why free_current_contents here? >> That seems weird. > > If this collect is $_ret, it will not have collect->expr. > Or maybe this collect will be free because when setup this collect get error. > So check it before free it. > >> >> Hui> + char file_name[strlen (dirname) + 1 + strlen (CTF_DATASTREAM_NAME) + 1]; >> >> alloca or malloc. > > Fixed. > >> >> Hui> + sprintf (file_name, "%s/%s", dirname, CTF_METADATA_NAME); >> Hui> + tcs.metadata_fd = fopen (file_name, "w"); >> Hui> + if (!tcs.metadata_fd) >> Hui> + error (_("Unable to open file '%s' for saving trace data (%s)"), >> Hui> + file_name, safe_strerror (errno)); >> Hui> + ctf_save_metadata_header (&tcs); >> Hui> + >> Hui> + sprintf (file_name, "%s/%s", dirname, CTF_DATASTREAM_NAME); >> Hui> + tcs.datastream_fd = fopen (file_name, "w"); >> Hui> + if (!tcs.datastream_fd) >> Hui> + error (_("Unable to open file '%s' for saving trace data (%s)"), >> Hui> + file_name, safe_strerror (errno)); >> >> On error these files will be left partially written. >> Is that intentional? > > What my thought about this part is even if GDB get error when CTF > save, the data before this error is OK. So in clean up function > ctf_save_cleanup, it will not remove this file. And it will write the > data that don't have error, function ctf_save_metadata (tcsp). > >> >> Hui> +extern void ctf_save (char *dirname); >> >> Why not const? >> This applies in a few spots in the patch. > > Fixed. > >> >> Hui> @@ -2465,6 +2466,7 @@ void >> Hui> mi_cmd_trace_save (char *command, char **argv, int argc) >> [...] >> Hui> + if (strcmp (argv[0], "-ctf") == 0) >> Hui> + generate_ctf = 1; >> >> The 'usage' line needs an update. > > Fixed. > >> >> Hui> + if (generate_ctf) >> Hui> + ctf_save (filename); >> Hui> + else >> Hui> + trace_save (filename, target_saves); >> >> I don't understand why CTF isn't just an option to trace_save -- share >> the trace-dependent work, separate out the formatting. >> > > I separate read and write CTF support function because: > CTF format is a complex format. I tried to support all of it but I failed. > Then I changed to use libbabeltrace, it works very good with read CTF. > But it doesn't supply API for CTF write. And I discussion the > developer of libbabeltrace, they said they don't have plan for that. > So I add CTF write function inside GDB with my hand. And because CTF > is design for tracepoint support. So it is really fit with tsave > command. So I add it as an option. > >> Hui> trace_save_command (char *args, int from_tty) >> Hui> { >> Tom > > Thanks for your comments. I post a new version. > > Best, > Hui Hi Tom, I post a patch that updated according to the update of trunk. Thanks, Hui 2012-12-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. * 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 (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] |