RE: [PATCH] btrace, frame: fix crash in get_frame_type

["Metzger, Markus T" <markus.t.metzger@intel.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