From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32488 invoked by alias); 16 Jan 2013 15:12:43 -0000 Received: (qmail 32475 invoked by uid 22791); 16 Jan 2013 15:12:42 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,URIBL_BLACK X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 16 Jan 2013 15:12:36 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1TvUfL-0006Nx-Lt from Hafiz_Abid@mentor.com ; Wed, 16 Jan 2013 07:12:35 -0800 Received: from SVR-IES-FEM-03.mgc.mentorg.com ([137.202.0.108]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Wed, 16 Jan 2013 07:12:35 -0800 Received: from EU-MBX-03.mgc.mentorg.com ([169.254.2.46]) by SVR-IES-FEM-03.mgc.mentorg.com ([137.202.0.108]) with mapi id 14.01.0289.001; Wed, 16 Jan 2013 15:12:33 +0000 From: "Abid, Hafiz" To: Hui Zhu , Tom Tromey CC: "Qi, Yao" , "Zhu, Hui" , gdb-patches ml Subject: RE: [PATCH] Add CTF support to GDB [3/4] ctf target Date: Wed, 16 Jan 2013 15:12:00 -0000 Message-ID: References: <50AC323F.1070907@mentor.com> <50AC3E15.1020308@codesourcery.com> <87zk203zw3.fsf@fleche.redhat.com>, In-Reply-To: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: 2013-01/txt/msg00336.txt.bz2 Hi Hui, Some minor issues that I noted. Many functions in your patch starts with bt_ctf_ like the function from lib= babeltrace. So it is difficult to see which function comes from the library= and which function is actually in GDB. I am not sure about the convention = but it looks to me that it would make the code more readable if you renamed= your functions. Bring them into GDB namespace. >+ error (_("Initialize libbabeltrace fail")); >+ error (_("Use libbabeltrace open \"%s\" fail"), dirname); >+ warning (_("get tracepoint id fail.")); As Tom already pointed out, these messages can be improved. How about somet= hing like "Unable to get the tracepoint id" >+/* Add each variable of crrent traceframe to GDB as internalvar. */ current >+ warning (_("$%s is not support."), name); supported >+ if (bt_ctf_event_to_internalvar ()) >+ warning (_("add internal var of this frame fail.")); This warning message does not tell much. Perhaps it can be moved inside bt_= ctf_event_to_internalvar where we have more context to provide more informa= tion. Also it needs to be re-phrased. May be it is just me but indentation looks off at many places. Regards, Abid ________________________________________ From: gdb-patches-owner@sourceware.org [gdb-patches-owner@sourceware.org] o= n behalf of Hui Zhu [teawater@gmail.com] Sent: Friday, December 21, 2012 8:22 AM To: Tom Tromey Cc: Qi, Yao; Zhu, Hui; gdb-patches ml Subject: Re: [PATCH] Add CTF support to GDB [3/4] ctf target On Fri, Nov 30, 2012 at 4:41 AM, Tom Tromey wrote: >>>>>> "Hui" =3D=3D Hui Zhu writes: > > Hui> --- a/configure.ac > Hui> +++ b/configure.ac > Hui> @@ -2306,6 +2306,134 @@ if test "$enable_gdbserver" =3D "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 op= tion) ;; > Hui> +esac],[enable_ctf_target=3Dauto]) > Hui> + > Hui> +if test "$enable_ctf_target" =3D "yes" || test "$enable_ctf_target"= =3D "auto"; then > Hui> + pkg_config_args=3Dglib-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? > The developer of babeltrace added a patch to "provides a basic pkg-config file for libbabeltrace" after I sent request about that. So its trunk support pkg-config now. New patch support it now. > Hui> +static struct bt_context *ctx =3D NULL; > Hui> +static struct bt_ctf_iter *iter =3D 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. I added comments to each function to this patch. > > Hui> + error (_("Try to use libbabeltrace got error")); > > Please phrase differently. I change it to "Initialize libbabeltrace fail". > > Hui> + error (_("Try to open \"%s\" got error"), dirname); > > Likewise. I change it to: error (_("Use libbabeltrace open \"%s\" fail"), dirname); > > Hui> + if (strcmp (bt_ctf_field_name(list_d[i]), name) =3D=3D 0) > > Missing a space before a paren. This happens in a few spots. Fixed. > > Hui> + /* XXX: some types are not support. */ > > How about an error in this case? > No new FIXME comments anyway. Removed. When this type is not support, GDB will output a warning for that. > > Hui> +extern void output_command (char *, int); > > Time for this to go into a header file. Fixed. > > 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* stopn= otes); > > 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. I change it to: /* Return name of current traceframe's tracepoint. Return NULL if the target doesn't support it. */ const char *(*to_get_current_tracepoint_name) (void); /* Dump all the value of current traceframe. Return fail if the target doesn't support it. Then GDB will dump all the value of current traceframe with itself. */ int (*to_trace_dump) (int from_tty); > > 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. Fixed. > > 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 t= he > Hui> trace frame. */ > Hui> if (!(type =3D=3D tfind_number && num =3D=3D -1) > Hui> - && (has_stack_frames () || traceframe_number >=3D 0)) > Hui> + && has_stack_frames ()) > > I'm curious about the rationale and impact of this change. I agree with this change looks odd. But it is indispensable for this patch. According to my prev mail in this thread, maybe you had gotten that CTF format is not base on memory mode like tfild. So it don't have frame or something like it. All this part of code is: if (!(type =3D=3D tfind_number && num =3D=3D -1) && (has_stack_frames () || traceframe_number >=3D 0)) old_frame_id =3D get_frame_id (get_current_frame ()); target ctf cannot support "old_frame_id =3D get_frame_id (get_current_frame ());". But traceframe_number >=3D 0 will let GDB call the line that target ctf don't support. And I don't think traceframe_number >=3D 0 is very import for "target remote" or "target tfile" that support tfind because both of them "has_stack_frames ()". So I remove "traceframe_number >=3D 0". > > Tom Thanks for your help. According to your comments. I post a new patch. Merry Christmas! Best, Hui 2012-12-20 Hui Zhu * aclocal.m4: Add PKG_PROG_PKG_CONFIG. * c-exp.y (lookup_enum): Rename to lookup_enum_gdb. * config.in (HAVE_LIBBABELTRACE): new macro. * configure: New option "--enable-ctf-target". * configure.ac: New option "--enable-ctf-target". * ctf.c (babeltrace/babeltrace.h, babeltrace/types.h, babeltrace/ctf/events.h, babeltrace/ctf/iterator.h): New includes. (ctx, iter, current_tp, ctf_event, ctf_ops): New variables. (bt_ctf_close, bt_ctf_open, bt_ctf_find_field, bt_ctf_event_id, bt_ctf_def_to_val, bt_ctf_event_to_internalvar, bt_ctf_goto_begin, bt_ctf_find_num, bt_ctf_find_tp, ctf_open, ctf_close, ctf_trace_find, ctf_get_current_tracepoint_name, ctf_trace_dump, ctf_has_all_memory, ctf_has_memory, ctf_has_stack, ctf_has_registers, ctf_thread_alive, init_ctf_ops, _initialize_ctf): New functions. * gdbtypes.c (lookup_enum): Rename to lookup_enum_gdb. * python/py-type.c (typy_lookup_typename): Rename lookup_enum to lookup_enum_gdb. * symtab.h (lookup_enum): Rename to lookup_enum_gdb. * target.c (update_current_target): Add to_get_current_tracepoint_name and to_trace_dump. (update_current_target): Ditto. * target.h (target_ops): Add to_get_current_tracepoint_name and to_trace_dump. (target_get_current_tracepoint_name, target_trace_dump): New macro. * tracepoint.c (tfind_1): Add checks for has_stack_frames (). Call target_get_current_tracepoint_name to show the name of tracepoint. (trace_dump_command): Call target_trace_dump.