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

[Doug Evans <dje@google.com>]

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.