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

[Tom Tromey <tromey@redhat.com>]

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

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.

Hui> +    error (_("Try to use libbabeltrace got error"));

Please phrase differently.

Hui> +      error (_("Try to open \"%s\" got error"), dirname);

Likewise.

Hui> +      if (strcmp (bt_ctf_field_name(list_d[i]), name) == 0)

Missing a space before a paren.  This happens in a few spots.

Hui> +  /* XXX: some types are not support.  */

How about an error in this case?
No new FIXME comments anyway.

Hui> +extern void output_command (char *, int);

Time for this to go into a header file.

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.

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.

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.

Tom