Re: [PATCH 2/5] btrace: support Intel(R) Processor Trace

[Pedro Alves <palves@redhat.com>]

On 06/23/2015 09:16 AM, Markus Metzger wrote:
> Adds a new command "record btrace pt" to configure the kernel to use
> Intel(R) Processor Trace instead of Branch Trace Strore.

This looks very good to me.  A few minor details to sort out,
and this is good to go.

> 
> The "record btrace" command chooses the tracing format automatically.
> 
> Intel Processor Trace support requires kernel 4.1 and libipt.

s/kernel/Linux/.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 3ec5851..9ab64bf 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -112,6 +112,10 @@ show mpx bound
>  set mpx bound on i386 and amd64
>     Support for bound table investigation on Intel(R) MPX enabled applications.
>  
> +record btrace pt
> +record pt
> +  Start branch trace recording using Intel(R) Processor Trace format.
> +
>  * New options
>  

The patch also includes new RSP packets.  Those should be mentioned
in NEWS as well.  I think the new configure switch should be mentioned
in a:

* New configure options

section too.

> +  decoder = pt_insn_alloc_decoder (&config);
> +  if (decoder == NULL)
> +    error (_("Failed to allocate the Intel(R) Processor Trace decoder."));
> +
> +  TRY
> +    {
> +      struct pt_image *image;
> +
> +      image = pt_insn_get_image(decoder);
> +      if (image == NULL)
> +	error (_("Failed to configure the Intel(R) Processor Trace decoder."));
> +
> +      errcode = pt_image_set_callback(image, btrace_pt_readmem_callback, NULL);
> +      if (errcode < 0)
> +	error (_("Failed to configure the Intel(R) Processor Trace decoder: "
> +		 "%s."), pt_errstr (pt_errcode (errcode)));
> +
> +      ftrace_add_pt (decoder, &btinfo->begin, &btinfo->end, &level,
> +		     &btinfo->ngaps);
> +    }
> +  CATCH (error, RETURN_MASK_ALL)
> +    {
> +      /* Indicate a gap in the trace if we quit trace processing.  Errors were
> +	 already logged before.  */

What does this "already logged before" mean?  AFAICS, the errors thrown
in the TRY branch are just swallowed here.  Did you mean to rethrow them?
Otherwise I'm not seeing the point in throwing them in the first place.

> +      if (error.reason == RETURN_QUIT && btinfo->end != NULL)
> +	{
> +	  btinfo->end = ftrace_new_gap (btinfo->end, BDE_PT_USER_QUIT);
> +	  btinfo->ngaps++;
> +	}
> +    }
> +  END_CATCH
> +



>    internal_error (__FILE__, __LINE__, _("Unkown branch trace format."));
> @@ -1056,7 +1312,7 @@ check_xml_btrace_version (struct gdb_xml_parser *parser,
>  {
>    const char *version = xml_find_attribute (attributes, "version")->value;
>  
> -  if (strcmp (version, "1.0") != 0)
> +  if (strcmp (version, "1.0") != 0 && strcmp (version, "2.0") != 0)

What fails if you send the new xml to an old/unpatched gdb, while keeping the
version at 1.0?  Is the new file really incompatible, or are you just
adding new elements/attributes?  There's usually no need to bump the
version in the latter case.  Old gdb will just ignore the
elements/attributes it doesn't recognize, and thus not support the feature.

> +/* The "set record btrace pt" command.  */
> +
> +static void
> +cmd_set_record_btrace_pt (char *args, int from_tty)
> +{
> +  printf_unfiltered (_("\"set record btrace pt\" must be followed "
> +		       "by an apporpriate subcommand.\n"));

typo: "apporpriate"

> +  help_list (set_record_btrace_pt_cmdlist, "set record btrace pt ",
> +	     all_commands, gdb_stdout);
> +}


> +  add_setshow_uinteger_cmd ("buffer-size", no_class,
> +			    &record_btrace_conf.pt.size,
> +			    _("Set the record/replay pt buffer size."),
> +			    _("Show the record/replay pt buffer size."), _("\
> +Bigger buffers allow longer recording but also take more time to process \
> +the recorded execution.\n\
> +The actual buffer size may differ from the requested size.  Use \"info record\" \
> +to see the actual buffer size."), NULL, NULL,
                                           ^^^^
Please add a "showfunc" hook for i18n.

Thanks,
Pedro Alves