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

["Metzger, Markus T" <markus.t.metzger@intel.com>]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Tuesday, June 30, 2015 2:57 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 2/5] btrace: support Intel(R) Processor Trace
> 
> 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.

Thanks.


> > +  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.

This means that decode errors are already represented as gaps in the trace.
When the trace is printed, the error at a trace gap is printed.

This code is now handling a user interrupt, which is also represented
as a gap at the very end of the trace.

This reference to decode errors is maybe more confusing than helpful.
I'll remove it.


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

I'm just adding new elements and attributes.  I thought I'd bump the version
since there are new features.  Should I leave it at version 1.0?


regards,
markus.

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052