From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31981 invoked by alias); 29 Nov 2012 20:06:15 -0000 Received: (qmail 31969 invoked by uid 22791); 29 Nov 2012 20:06:14 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 29 Nov 2012 20:06:06 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qATK636X020850 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 29 Nov 2012 15:06:04 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qATK61tO000932 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 29 Nov 2012 15:06:02 -0500 From: Tom Tromey To: Hui Zhu Cc: Subject: Re: [PATCH] Add CTF support to GDB [1/4] Add "-ctf" to tsave command References: <50AC3217.6040608@mentor.com> Date: Thu, 29 Nov 2012 20:06:00 -0000 In-Reply-To: <50AC3217.6040608@mentor.com> (Hui Zhu's message of "Wed, 21 Nov 2012 09:44:55 +0800") Message-ID: <878v9k5g46.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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-11/txt/msg00894.txt.bz2 >>>>> "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. 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