From: "Metzger, Markus T" <markus.t.metzger@intel.com>
To: Pedro Alves <palves@redhat.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: RE: [PATCH 2/5] btrace: support Intel(R) Processor Trace
Date: Wed, 01 Jul 2015 08:39:00 -0000 [thread overview]
Message-ID: <A78C989F6D9628469189715575E55B2333168453@IRSMSX104.ger.corp.intel.com> (raw)
In-Reply-To: <5592B100.803@redhat.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
prev parent reply other threads:[~2015-07-01 8:39 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-23 8:22 [PATCH 0/5] Support " Markus Metzger
2015-06-23 8:22 ` [PATCH 1/5] configure: check for libipt Markus Metzger
2015-06-30 12:56 ` Pedro Alves
2015-06-30 14:54 ` Metzger, Markus T
2015-06-30 15:01 ` Pedro Alves
2015-06-23 8:22 ` [PATCH 4/5] btrace: store raw btrace data Markus Metzger
2015-06-30 12:56 ` Pedro Alves
2015-06-23 8:22 ` [PATCH 5/5] btrace: maintenance commands Markus Metzger
2015-06-23 15:28 ` Eli Zaretskii
2015-06-24 7:05 ` Metzger, Markus T
2015-06-24 14:38 ` Eli Zaretskii
2015-06-30 12:57 ` Pedro Alves
2015-06-23 8:22 ` [PATCH 3/5] btrace, linux: use data_size and data_offset Markus Metzger
2015-06-30 12:56 ` Pedro Alves
2015-06-23 8:23 ` [PATCH 2/5] btrace: support Intel(R) Processor Trace Markus Metzger
2015-06-23 15:32 ` Eli Zaretskii
2015-06-30 12:56 ` Pedro Alves
2015-06-30 14:54 ` Metzger, Markus T
2015-06-30 15:08 ` Pedro Alves
2015-07-01 8:39 ` Metzger, Markus T [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=A78C989F6D9628469189715575E55B2333168453@IRSMSX104.ger.corp.intel.com \
--to=markus.t.metzger@intel.com \
--cc=gdb-patches@sourceware.org \
--cc=palves@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).