Re: [PATCH] Add CTF support to GDB [3/4] ctf target

[Hui Zhu <teawater at gmail dot 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