Re: [PATCH v3 03/15] Read CTF by the ctf target

Doug Evans
2013-04-09T15:53:47Z

Yao Qi writes:
> On 04/09/2013 12:58 AM, Doug Evans wrote:
> >> + if (ret < 0)
> > > + {
> > > + ctf_destroy ();
> > > + error (_("Unable to use libbabeltrace open \"%s\""), dirname);
> >
> > The wording of this error message (here and below) is awkward.
> > I don't know libbabeltrace enough to know if this is accurate, but
> > it reads better.
> >
> > error (_("Unable to use libbabeltrace on \"%s\""), dirname);
> >
> > or
> >
> > error (_("Unable to use libbabeltrace on directory \"%s\""), dirname);
>
> I changed the message to
>
> error (_("Unable to use libbabeltrace on directory \"%s\""),
> dirname);
>
> >
> > Also, IWBN to add a more specific reason for the failure,
> > but I'm not sure how easy that would be.
> > Is there a bt_foo function akin to strerror?
> >
>
> Unfortunately, there isn't such function.
>
>
> > > +
> > > + if (bt_iter_next (bt_ctf_get_iter (ctf_iter)) < 0)
> > > + break;
> > > + }
> > > +
> > > + /* Restore the position. */
> > > + bt_iter_set_pos (bt_ctf_get_iter (ctf_iter), pos);
> > > +
> >
> > Remove blank line.
> >
> > > +}
>
> Removed.
>
>
> > > +
> > > + scope = bt_ctf_get_top_level_scope (event,
> > > + BT_EVENT_FIELDS);
> > > +
> > > + def = bt_ctf_get_field (event, scope, "address");
> > > + maddr = bt_ctf_get_uint64 (def);
> > > + def = bt_ctf_get_field (event, scope, "length");
> > > + mlen = (uint16_t) bt_ctf_get_uint64 (def);
> >
> > I don't know libbabeltrace, I'm assuming length can be at most 16 bits.
> >
>
> Right. "length" is defined as uint16_t in metadata, on the other hand,
> "length" is got from the remote target, which is 16-bit as well.
>
> > > +
> > > +/* This is the implementation of target_ops method to_traceframe_info.
> > > + Iterate the events whose name is "memory", in current
> > > + frame, extract memory range information, and return them in
> > > + traceframe_info. */
> > > +
> > > +static struct traceframe_info *
> > > +ctf_traceframe_info (void)
> > > +{
> > > + struct traceframe_info *info = XCNEW (struct traceframe_info);
> > > + const char *name;
> > > + struct bt_iter_pos *pos;
> > > +
> > > + gdb_assert (ctf_iter != NULL);
> > > + /* Save the current position. */
> > > + pos = bt_iter_get_pos (bt_ctf_get_iter (ctf_iter));
> > > + gdb_assert (pos->type == BT_SEEK_RESTORE);
> > > +
> > > + do
> > > + {
> > > + struct bt_ctf_event *event
> > > + = bt_ctf_iter_read_event (ctf_iter);
> > > +
> > > + name = bt_ctf_event_name (event);
> > > +
> > > + if (name == NULL || strcmp (name, "register") == 0
> > > + || strcmp (name, "frame") == 0)
> > > + ;
> > > + else if (strcmp (name, "memory") == 0)
> > > + {
> > > + const struct bt_definition *scope
> > > + = bt_ctf_get_top_level_scope (event,
> > > + BT_EVENT_FIELDS);
> > > + const struct bt_definition *def;
> > > + struct mem_range *r;
> > > +
> > > + r = VEC_safe_push (mem_range_s, info->memory, NULL);
> > > + def = bt_ctf_get_field (event, scope, "address");
> > > + r->start = bt_ctf_get_uint64 (def);
> > > +
> > > + def = bt_ctf_get_field (event, scope, "length");
> > > + r->length = (uint16_t) bt_ctf_get_uint64 (def);
> > > + }
> > > + else
> > > + warning (_("Unhandled trace block type (%s) "
> > > + "while building trace frame info."),
> > > + name);
> >
> > These days we prefer multi-line single statements for if/else/etc.
> > to be wrapped in braces. i.e.,
> >
> > {
> > warning (_("Unhandled trace block type (%s) "
> > "while building trace frame info."),
> > name);
> > }
> >
>
> Fixed. I knew that single statement with comments should be warped by
> braces, like:
>
> if (foo)
> /* Comments. */
> bar ();
>
> after reading GDB internals, I realize that "warning message in
> multiple lines" falls into this category as well. You are right.
>
> > > +
> > > +/* module initialization */
> >
> > blank line
> >
> > > +void
> > > +_initialize_ctf (void)
>
> Fixed.
>
> --
> Yao ( )
>
> gdb:
>
> 2013-04-09 Hui Zhu <hui@codesourcery.com>
> Yao Qi <yao@codesourcery.com>
>
> * configure.ac: Check libbabeltrace is installed.
> * config.in: Regenerate.
> * configure: Regenerate.
> * Makefile.in (LIBBABELTRACE): New.
> (CLIBS): Add LIBBABELTRACE.
> * ctf.c (ctx, ctf_iter, trace_dirname): New.
> (ctf_destroy, ctf_open_dir, ctf_open): New.
> (ctf_close, ctf_files_info): New.
> (ctf_fetch_registers, ctf_xfer_partial): New.
> (ctf_get_trace_state_variable_value): New.
> (ctf_get_tpnum_from_frame_event): New.
> (ctf_get_traceframe_address): New.
> (ctf_trace_find, ctf_has_stack): New.
> (ctf_has_registers, ctf_traceframe_info, init_ctf_ops): New.
> (_initialize_ctf): New.
> * tracepoint.c (get_tracepoint_number): New
> (struct traceframe_info, trace_regblock_size): Move it to ...
> * tracepoint.h: ... here.
> (get_tracepoint_number): Declare it.

Thanks.
The patch is ok.