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 Tue, Dec 18, 2012 at 10:26 PM, Hui Zhu <teawater@gmail.com> wrote: > 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. Hi Tom, When I tried to fixed the format issue of target ctf patch, I found that this patch has some format issue too. So I post a new version that fixed these issues. Thanks, Hui 2012-12-20 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] |