From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22732 invoked by alias); 11 Feb 2013 12:55:55 -0000 Received: (qmail 22724 invoked by uid 22791); 11 Feb 2013 12:55:55 -0000 X-SWARE-Spam-Status: No, hits=-4.8 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-oa0-f41.google.com (HELO mail-oa0-f41.google.com) (209.85.219.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 11 Feb 2013 12:55:48 +0000 Received: by mail-oa0-f41.google.com with SMTP id i10so6261237oag.0 for ; Mon, 11 Feb 2013 04:55:47 -0800 (PST) X-Received: by 10.60.32.147 with SMTP id j19mr10603676oei.68.1360587347542; Mon, 11 Feb 2013 04:55:47 -0800 (PST) MIME-Version: 1.0 Received: by 10.60.46.133 with HTTP; Mon, 11 Feb 2013 04:55:07 -0800 (PST) In-Reply-To: References: <50AC323F.1070907@mentor.com> <50AC3E15.1020308@codesourcery.com> <87zk203zw3.fsf@fleche.redhat.com> From: Hui Zhu Date: Mon, 11 Feb 2013 12:55:00 -0000 Message-ID: Subject: Re: [PATCH] Add CTF support to GDB [3/4] ctf target To: "Abid, Hafiz" Cc: Tom Tromey , "Qi, Yao" , "Zhu, Hui" , gdb-patches ml Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: 2013-02/txt/msg00248.txt.bz2 On Wed, Jan 23, 2013 at 1:53 PM, Hui Zhu wrote: > On Wed, Jan 16, 2013 at 11:12 PM, Abid, Hafiz wro= te: >> Hi Hui, >> Some minor issues that I noted. >> >> Many functions in your patch starts with bt_ctf_ like the function from = libbabeltrace. So it is difficult to see which function comes from the libr= ary and which function is actually in GDB. I am not sure about the conventi= on but it looks to me that it would make the code more readable if you rena= med your functions. Bring them into GDB namespace. > > I agree with you. And it increase the Increased the possibility of > same name issue with babeltrace in the future. > So I change it to "ctf_". > >> >>>+ 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 so= mething like "Unable to get the tracepoint id" > > Fixed. > >> >>>+/* Add each variable of crrent traceframe to GDB as internalvar. */ >> current > > Fixed. > >> >>>+ warning (_("$%s is not support."), name); >> supported > > Fixed. > >> >>>+ 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 info= rmation. Also it needs to be re-phrased. > > Sorry. I didn't have good words for these 2 fail. So I just change > warning to Unable to xxx... > >> >> May be it is just me but indentation looks off at many places. >> >> Regards, >> Abid > > Thanks, > Hui > > 2013-01-23 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. > (ctf_close_dir, ctf_open_dir, ctf_find_field, ctf_event_id, > ctf_def_to_val, ctf_event_to_internalvar, ctf_goto_begin, > ctf_find_num, 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. > >> >> ________________________________________ >> From: gdb-patches-owner@sourceware.org [gdb-patches-owner@sourceware.org= ] on 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 thi= s 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=3Dauto]) >>> Hui> + >>> Hui> +if test "$enable_ctf_target" =3D "yes" || test "$enable_ctf_targe= t" =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* sto= pnotes); >>> >>> 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= the >>> 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 pa= tch. >> 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 include= s. >> (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_begi= n, >> 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. Hi, Ping. Thanks, Hui