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

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Tuesday, June 30, 2015 5:09 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 2/5] btrace: support Intel(R) Processor Trace
> 
> On 06/30/2015 03:54 PM, Metzger, Markus T wrote:
> 
> >>> +  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.
> 
> I still don't get why throw the errors in the TRY branch:
> 
>       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)));
> 
> ... if they're just dropped on the catch brock.  Shouldn't those
> be rethrown?  The CATCH block you had does not do that.  And it
> neither rethrows the ctrl-c that generates the RETURN_QUIT:
> 
> +  CATCH (error, RETURN_MASK_ALL)
> +    {
> +      /* Indicate a gap in the trace if we quit trace processing.  Errors were
> +	 already logged before.  */
> +      if (error.reason == RETURN_QUIT && btinfo->end != NULL)
> +	{
> +	  btinfo->end = ftrace_new_gap (btinfo->end, BDE_PT_USER_QUIT);
> +	  btinfo->ngaps++;
> +	}
> +    }
> 
> So shouldn't that be:
> 
>   CATCH (error, RETURN_MASK_ALL)
>     {
>       /* Indicate a gap in the trace if we quit trace processing.  Errors were
> 	 already logged before.  */
>       if (error.reason == RETURN_QUIT && btinfo->end != NULL)
> 	{
> 	  btinfo->end = ftrace_new_gap (btinfo->end, BDE_PT_USER_QUIT);
> 	  btinfo->ngaps++;
> 	}
> 
> +     throw_exception (error);
>     }
> 
> ?

You're right.  We should rethrow.  Thanks.

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