From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29828 invoked by alias); 5 Dec 2012 01:47:37 -0000 Received: (qmail 29819 invoked by uid 22791); 5 Dec 2012 01:47:36 -0000 X-SWARE-Spam-Status: No, hits=-4.7 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-bk0-f41.google.com (HELO mail-bk0-f41.google.com) (209.85.214.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 05 Dec 2012 01:47:29 +0000 Received: by mail-bk0-f41.google.com with SMTP id jg9so2056390bkc.0 for ; Tue, 04 Dec 2012 17:47:28 -0800 (PST) Received: by 10.204.12.220 with SMTP id y28mr4938493bky.112.1354672048145; Tue, 04 Dec 2012 17:47:28 -0800 (PST) MIME-Version: 1.0 Received: by 10.205.32.12 with HTTP; Tue, 4 Dec 2012 17:46:47 -0800 (PST) In-Reply-To: <878v9k5g46.fsf@fleche.redhat.com> References: <50AC3217.6040608@mentor.com> <878v9k5g46.fsf@fleche.redhat.com> From: Hui Zhu Date: Wed, 05 Dec 2012 01:47:00 -0000 Message-ID: Subject: Re: [PATCH] Add CTF support to GDB [1/4] Add "-ctf" to tsave command To: Tom Tromey Cc: Hui Zhu , gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes 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: 2012-12/txt/msg00068.txt.bz2 On Fri, Nov 30, 2012 at 4:06 AM, Tom Tromey wrote: >>>>>> "Hui" == Hui Zhu 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. > > 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 have 2 questions about use ui_file: 1. I can add new interface about fseek to ui_file? 2. I found that write functions of stdio_file don't check write fail. Is that right, or I miss something? For example: /* Calling error crashes when we are called from the exception framework. */ if (fwrite (buf, length_buf, 1, stdio->file)) { /* Nothing. */ } Thanks, Hui > > 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. > > Hui> + ctf_save_type_name_write (tcsp, TYPE_FIELD_TYPE (type, biggest_id)); > > Here too. > > Hui> + ctf_save_fwrite_format (tcsp->metadata_fd, "%s", TYPE_NAME (type)); > > What if TYPE_NAME is NULL? > > 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 > > 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. > > 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. > > 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. > > 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. > > 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? > > Hui> + char name[strlen (print_name) + 1]; > > I think you need an explicit alloca here. > Or xmalloc + xfree, which is probably better. > > 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. > > 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. > > 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. > > Hui> +static void > Hui> +ctf_save_do_nothing (void *p) > Hui> +{ > Hui> +} > > Use null_cleanup instead. > > Hui> + if (collect->expr) > Hui> + free_current_contents (&collect->expr); > > Why free_current_contents here? > That seems weird. > > Hui> + char file_name[strlen (dirname) + 1 + strlen (CTF_DATASTREAM_NAME) + 1]; > > alloca or malloc. > > 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? > > Hui> +extern void ctf_save (char *dirname); > > Why not const? > This applies in a few spots in the patch. > > 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. > > 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. > > Hui> trace_save_command (char *args, int from_tty) > Hui> { > Tom