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

Tom Tromey
2013-03-01T15:51:03Z

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