From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5442 invoked by alias); 29 Nov 2012 20:42:04 -0000 Received: (qmail 5233 invoked by uid 22791); 29 Nov 2012 20:42:00 -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:41:54 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qATKfoQ7030501 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 29 Nov 2012 15:41:50 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qATKfmcX017006 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 29 Nov 2012 15:41:48 -0500 From: Tom Tromey To: Hui Zhu Cc: Yao Qi , Hui Zhu , gdb-patches ml Subject: Re: [PATCH] Add CTF support to GDB [3/4] ctf target References: <50AC323F.1070907@mentor.com> <50AC3E15.1020308@codesourcery.com> Date: Thu, 29 Nov 2012 20:42:00 -0000 In-Reply-To: (Hui Zhu's message of "Tue, 27 Nov 2012 18:54:27 +0800") Message-ID: <87zk203zw3.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/msg00897.txt.bz2 >>>>> "Hui" == Hui Zhu writes: Hui> --- a/configure.ac Hui> +++ b/configure.ac Hui> @@ -2306,6 +2306,134 @@ if test "$enable_gdbserver" = "yes" -a " Hui> AC_MSG_ERROR(Automatic gdbserver build is not supported for this configuration) Hui> fi Hui> +AC_ARG_ENABLE(ctf-target, Hui> +AS_HELP_STRING([--enable-ctf-target], Hui> + [enable ctf target (yes/no/auto, default is auto)]), Hui> +[case "${enableval}" in Hui> + yes| no|auto) ;; Hui> + *) AC_MSG_ERROR(bad value ${enableval} for --enable-ctf-target option) ;; Hui> +esac],[enable_ctf_target=auto]) Hui> + Hui> +if test "$enable_ctf_target" = "yes" || test "$enable_ctf_target" = "auto"; then Hui> + pkg_config_args=glib-2.0 Hui> + for module in . gmodule [...] This seems like an awful lot of code just to check for one library. Does libbabeltrace not come with its own pkg-config file? Or not link against its dependencies? Hui> +static struct bt_context *ctx = NULL; Hui> +static struct bt_ctf_iter *iter = NULL; Hui> +static int current_tp; Hui> +static struct bt_ctf_event *ctf_event; Comments. Most of this patch was impenetrable to me due to the general lack of comments. Hui> + error (_("Try to use libbabeltrace got error")); Please phrase differently. Hui> + error (_("Try to open \"%s\" got error"), dirname); Likewise. Hui> + if (strcmp (bt_ctf_field_name(list_d[i]), name) == 0) Missing a space before a paren. This happens in a few spots. Hui> + /* XXX: some types are not support. */ How about an error in this case? No new FIXME comments anyway. Hui> +extern void output_command (char *, int); Time for this to go into a header file. Hui> --- a/target.h Hui> +++ b/target.h Hui> @@ -811,6 +811,10 @@ struct target_ops Hui> successful, 0 otherwise. */ Hui> int (*to_set_trace_notes) (char *user, char *notes, char* stopnotes); Hui> + const char *(*to_get_current_tracepoint_name) (void); Hui> + Hui> + int (*to_trace_dump) (int from_tty); These sorts of additions particularly need documentation. Hui> +#define target_get_current_tracepoint_name() \ Hui> +(*current_target.to_get_current_tracepoint_name) () Hui> + Hui> +#define target_trace_dump(from_tty) \ Hui> +(*current_target.to_trace_dump) (from_tty) Formatting. Hui> --- a/tracepoint.c Hui> +++ b/tracepoint.c Hui> @@ -2243,7 +2243,7 @@ tfind_1 (enum trace_find_type type, int Hui> below (correctly) decide to print out the source location of the Hui> trace frame. */ Hui> if (!(type == tfind_number && num == -1) Hui> - && (has_stack_frames () || traceframe_number >= 0)) Hui> + && has_stack_frames ()) I'm curious about the rationale and impact of this change. Tom