This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

>+    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"

>+/* 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 information. 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] 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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]