This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH] btrace, frame: fix crash in get_frame_type
- 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: Fri, 5 Feb 2016 08:23:10 +0000
- Subject: RE: [PATCH] btrace, frame: fix crash in get_frame_type
- Authentication-results: sourceware.org; auth=none
- References: <1453828132-2319-1-git-send-email-markus dot t dot metzger at intel dot com> <56B375EE dot 7020407 at redhat dot com>
> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Thursday, February 4, 2016 5:02 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH] btrace, frame: fix crash in get_frame_type
Hi Pedro,
Thanks for your review.
> > The comment on skip_artificial_frames says
> >
> > /* Given FRAME, return the enclosing frame as found in real frames read-in
> from
> > inferior memory. Skip any previous frames which were made up by GDB.
> > Return the original frame if no immediate previous frames exist.
> > */
> >
> > That last part, "return the original frame if no immediate previous
> > frames exist", is missing. I added that.
>
> Not sure about this. Why does it make sense to return the original frame?
> It sounds arbitrary -- could just as well be the outermost? What does the
> caller in question do with it, and why is it correct?
Looks like I misinterpreted the comment. I first thought (without checking,
my bad) that someone had accidentally removed that part without updating
the comment.
I now think that the comment should rather be read as "If the argument frame
is not an artificial frame, return that". The function (originally called
skip_inlined_frames) was never able to handle frame chains that didn't end
with a normal frame.
Let me check the various callers. I'm inclined to return NULL in this case.
> > /* Ignore TAILCALL_FRAME type frames, they were executed already
> before
> > entering THISFRAME. */
> > - while (get_frame_type (prev_frame) == TAILCALL_FRAME)
> > + while (prev_frame != NULL && get_frame_type (prev_frame) ==
> > + TAILCALL_FRAME)
> > prev_frame = get_prev_frame (prev_frame);
> >
> > + /* We cannot pop tailcall frames. */ if (prev_frame == NULL)
> > + error (_("Cannot pop tailcall frame(s)."));
> > +
>
> How about factoring that out to a skip_tailcall_frames function, similar to
> skip_artificial_frames, and then do:
>
> prev_frame = skip_tailcall_frames (prev_frame);
> if (prev_frame == NULL)
> error (_("Cannot pop tailcall frame(s)."));
>
> here and similarly in the other case.
>
> And I wonder whether we should be using get_prev_frame_always for this
> too, like skip_artificial_frames uses.
I can try that. I'll split the patch as those changes are unrelated.
Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928