This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Add CTF support to GDB [3/4] ctf target
- From: Hui Zhu <teawater at gmail dot com>
- To: "Abid, Hafiz" <Hafiz_Abid at mentor dot com>
- Cc: Tom Tromey <tromey at redhat dot com>, "Qi, Yao" <Yao_Qi at mentor dot com>, "Zhu, Hui" <Hui_Zhu at mentor dot com>, gdb-patches ml <gdb-patches at sourceware dot org>
- Date: Mon, 11 Feb 2013 20:55:07 +0800
- Subject: Re: [PATCH] Add CTF support to GDB [3/4] ctf target
- References: <50AC323F.1070907@mentor.com> <50AC3E15.1020308@codesourcery.com> <CANFwon2hXtyFDXH8Zy-03Ao6rW1kRrNPuhOqdj=fF0UXgXiQkw@mail.gmail.com> <CANFwon3nLhuDMWkc0AuZBC2h6DK5T_Ggdb2OZx7e+GV5t6k-2g@mail.gmail.com> <87zk203zw3.fsf@fleche.redhat.com> <CANFwon0k48X5dAo_K=dR2novtD+qo+U3HftgvqsSZ1ZJno1MHQ@mail.gmail.com> <EB3B29AD43CA924DA27099BC8519237696CE8401@EU-MBX-03.mgc.mentorg.com> <CANFwon1YXrqbf3Pff5c_d2x+Ew05rD4cobyb-8tz2N4=Fpoozw@mail.gmail.com>
On Wed, Jan 23, 2013 at 1:53 PM, Hui Zhu <teawater@gmail.com> wrote:
> On Wed, Jan 16, 2013 at 11:12 PM, Abid, Hafiz <Hafiz_Abid@mentor.com> wrote:
>> 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 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.
>
> 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 something 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 information. 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 <hui_zhu@mentor.com>
>
> * 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 <tromey@redhat.com> wrote:
>>>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> 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?
>>>
>>
>> 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 = 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.
>>
>> 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) == 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* 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.
>>
>> 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 == 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.
>>
>> 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 == tfind_number && num == -1)
>> && (has_stack_frames () || traceframe_number >= 0))
>> old_frame_id = get_frame_id (get_current_frame ());
>>
>> target ctf cannot support "old_frame_id = get_frame_id
>> (get_current_frame ());". But traceframe_number >= 0 will let GDB
>> call the line that target ctf don't support.
>> And I don't think traceframe_number >= 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 >= 0".
>>
>>>
>>> Tom
>>
>> Thanks for your help.
>>
>> According to your comments. I post a new patch.
>>
>> Merry Christmas!
>>
>> Best,
>> Hui
>>
>> 2012-12-20 Hui Zhu <hui_zhu@mentor.com>
>>
>> * 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.
Hi,
Ping.
Thanks,
Hui