Re: [PATCH 3/5] Read CTF by the ctf target

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> +# Where is libbabeltrace? This will be empty if lzma was not available.
Yao> +LIBBABELTRACE = @btlibs@
Yao> +LIBBABELTRACE_CFLAGS = @btinc@

This mentions "lzma", but I didn't see a link... maybe just a
cut-and-pasto.

It is clearer to use the same variable names in configure.ac and
Makefile.in.  I'd suggest @LIBBABELTRACE@ and @LIBBABELTRACE_CFLAGS@ and
then change configure.ac.

Yao> +AC_ARG_WITH(babeltrace, [  --with-babeltrace=PATH       Specify prefix directory for the installed BABELTRACE package
Yao> +                          Equivalent to --with-babeltrace-include=PATH/include
Yao> +                          plus --with-babeltrace-lib=PATH/lib])

It's better to use AS_HELP_STRING.

Yao> +AC_ARG_WITH(bt_include, [  --with-babeltrace-include=PATH Specify directory for installed babeltrace include files])

Lines are too long.  AS_HELP_STRING will help with that :)

Yao> +AC_ARG_WITH(bt_lib, [  --with-babeltrace-lib=PATH   Specify the directory for the installed babeltrace library])
Yao> +
Yao> +case $with_babeltrace in
Yao> +  no)
Yao> +    btlibs=
Yao> +    btinc=
Yao> +    ;;
Yao> +  "" | yes)
Yao> +    btlibs=" -lbabeltrace -lbabeltrace-ctf "
Yao> +    btinc=""
Yao> +    ;;
Yao> +  *)
Yao> +    btlibs="-L$with_babeltrace/lib -lbabeltrace -lbabeltrace-ctf"
Yao> +    btinc="-I$with_babeltrace/include "
Yao> +    ;;
Yao> +esac

Earlier code initialized the output variables, but it turns out there
was no need to do that.

Yao> +  [AC_MSG_RESULT([yes]); AC_DEFINE(HAVE_LIBBABELTRACE, 1, [Define if libbabeltrace is available])],

Needs some line breaks.

Yao> +extern int trace_regblock_size;

Maybe in tracepoint.h instead?
I see other instances of this (in tracepoint.c and remote.c).
In most cases I think extern declarations belong in headers.
(There can be exceptions, IMO, but this doesn't seem like one.)

Yao> +	  const struct bt_definition *scope
Yao> +	    = bt_ctf_get_top_level_scope(event,
Yao> +					 BT_EVENT_FIELDS);
Yao> +	  const struct bt_definition *array
Yao> +	    = bt_ctf_get_field(event, scope, "contents");
Yao> +
Yao> +	  trace_regblock_size
Yao> +	    = bt_ctf_get_array_len (bt_ctf_get_decl_from_def (array));

Some spaces missing before "(".

Yao> +  /* Restore the position.  */
Yao> +  bt_iter_set_pos(bt_ctf_get_iter (ctf_iter), pos);

Likewise.

There are more too, I didn't mark them all.

Yao> +      /* xfree (regs); */

It seems like this should be deleted.
I don't really know, though.

Yao> +	  unsigned short mlen;

I'm curious about the rationale for the type here.
And a bit later:

Yao> +	  mlen = (uint16_t) bt_ctf_get_uint64 (def);

Why use uint16_t here and not mlen's real type?
But I don't understand why this has a shorter type anyhow.

Yao> +  /* It's unduly pedantic to refuse to look at the executable for
Yao> +     read-only pieces; so do the equivalent of readonly regions aka
Yao> +     QTro packet.  */
Yao> +  /* FIXME account for relocation at some point.  */

There's a rule against adding new FIXME comments.  Sometimes I think
maybe we're too strict about this; though in practice it does seem to me
that these are rarely fixed for their own sake, but instead just as a
side effect of something else.

I suppose you could rephrase it to be a note.
Or is it fixable?  It isn't clear to me in what situation this code is
invoked.

I was curious why this looks directly at the exec_bfd rather than using
something like ALL_OBJFILE_OSECTIONS.  It seemed like this might help
with the relocation problem.

Yao> +  if (exec_bfd)

A while ago we agreed to use the more explicit `exec_bfd != NULL' form.

Yao> +  /* Didn't find anything.  */
Yao> +  return found;

I think this can be reached in the "did find something" case as well, so
I suggest just dropping the comment.

Yao> +/* Return what address the current frame was collected at.  */

I think "Return the address at which the current frame was collected".

Yao> +      /* FIXME this is a poor heuristic if multiple locations.  */
Yao> +      if (tp && tp->base.loc)
Yao> +	addr = tp->base.loc->address;

The FIXME rule again.
Elsewhere the code just rejects multiple locations; but I'm not sure if
that is appropriate here.

Tom