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

["Metzger, Markus T" <markus.t.metzger@intel.com>]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Tuesday, February 9, 2016 1:06 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 3/3] btrace, frame: fix crash in get_frame_type
> 
> On 02/05/2016 02:18 PM, Markus Metzger wrote:
> 
> > diff --git a/gdb/frame.c b/gdb/frame.c index 6ab8834..cea7003 100644
> > --- a/gdb/frame.c
> > +++ b/gdb/frame.c
> > @@ -420,7 +420,8 @@ fprint_frame (struct ui_file *file, struct
> > frame_info *fi)
> >
> >  /* 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.  */
> > +   Return FRAME if FRAME is a non-artificial frame.
> > +   Return NULL if FRAME is NULL or the start of an artificial-only
> > + chain. */
> 
> Missing double-space after period.  But, most importantly, I'm not sure I like
> that this accepts a NULL frame.

Fine with me.


> > @@ -511,11 +517,12 @@ frame_unwind_caller_id (struct frame_info
> *next_frame)
> >       requests the frame ID of "main()"s caller.  */
> >
> >    next_frame = skip_artificial_frames (next_frame);
> > -  this_frame = get_prev_frame_always (next_frame);
> > -  if (this_frame)
> > -    return get_frame_id (skip_artificial_frames (this_frame));
> > -  else
> > +  if (next_frame == NULL)
> >      return null_frame_id;
> > +
> > +  this_frame = get_prev_frame_always (next_frame);  this_frame =
> > + skip_artificial_frames (this_frame);  return get_frame_id
> > + (this_frame);
> >  }
> >
> >  const struct frame_id null_frame_id = { 0 }; /* All zeros.  */ @@
> > -795,6 +802,9 @@ frame_find_by_id (struct frame_id id)  static
> > CORE_ADDR  frame_unwind_pc (struct frame_info *this_frame)  {
> > +  if (this_frame == NULL)
> > +    throw_error (NOT_AVAILABLE_ERROR, _("PC not available"));
> 
> How can this happen?

One of its callers, frame_unwind_caller_pc, calls it with the result of
skip_artificial_frames like this:

CORE_ADDR
frame_unwind_caller_pc (struct frame_info *this_frame)
{
  return frame_unwind_pc (skip_artificial_frames (this_frame));
}

Rather than handling the skip_artificial_frames() NULL return here,
I made frame_unwind_pc handle a NULL frame argument.

I can move the check into frame_unwind_caller_pc if you prefer.


> > +  /* If the frame chain consists only of artificial frames, use
> NEXT_FRAME's.
> > +
> > +     For tailcall frames, we (i.e. the DWARF frame unwinder) assume that
> they
> > +     have the gdbarch of the bottom (callee) frame.  If NEXT_FRAME is an
> > +     artificial frame, as well, we should drill down to the bottom in
> > +     frame_unwind_arch either directly via the tailcall unwinder or via a
> chain
> > +     of get_frame_arch calls.  */
> > +  caller = skip_artificial_frames (next_frame);  if (caller == NULL)
> > +    get_frame_arch (next_frame);
> > +
> > +  return frame_unwind_arch (next_frame);
> 
> Hmm, this looks odd.  Is the get_frame_arch call there for side effects, or did
> you mean to make that "return get_frame_arch" ?

Ouch.  Yes, I meant to return that frame.


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