This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH 2/5] btrace: support Intel(R) Processor Trace
- From: "Metzger, Markus T" <markus dot t dot metzger at intel dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Wed, 1 Jul 2015 08:39:04 +0000
- Subject: RE: [PATCH 2/5] btrace: support Intel(R) Processor Trace
- Authentication-results: sourceware.org; auth=none
- References: <1435047418-21611-1-git-send-email-markus dot t dot metzger at intel dot com> <1435047418-21611-3-git-send-email-markus dot t dot metzger at intel dot com> <55929205 dot 6080907 at redhat dot com> <A78C989F6D9628469189715575E55B233316814E at IRSMSX104 dot ger dot corp dot intel dot com> <5592B100 dot 803 at redhat 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