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

[Hui Zhu <teawater at gmail dot com>]

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.

Attachment: ctf-target.txt
Description: Text document