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

[Pedro Alves <palves@redhat.com>]

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.

>  
>  static struct frame_info *
>  skip_artificial_frames (struct frame_info *frame)
> @@ -428,11 +429,13 @@ skip_artificial_frames (struct frame_info *frame)
>    /* Note we use get_prev_frame_always, and not get_prev_frame.  The
>       latter will truncate the frame chain, leading to this function
>       unintentionally returning a null_frame_id (e.g., when the user
> -     sets a backtrace limit).  This is safe, because as these frames
> -     are made up by GDB, there must be a real frame in the chain
> -     below.  */
> -  while (get_frame_type (frame) == INLINE_FRAME
> -	 || get_frame_type (frame) == TAILCALL_FRAME)
> +     sets a backtrace limit).
> +
> +     Note that for record targets we may get a frame chain that consists
> +     of artificial frames only.  */
> +  while (frame != NULL &&
> +	 (get_frame_type (frame) == INLINE_FRAME
> +	  || get_frame_type (frame) == TAILCALL_FRAME))
>      frame = get_prev_frame_always (frame);

&& goes on the next line, but can we make these look like:

static struct frame_info *
skip_artificial_frames (struct frame_info *frame)
{
...
  while (get_frame_type (frame) == INLINE_FRAME
	 || get_frame_type (frame) == TAILCALL_FRAME)
    {
      frame = get_prev_frame_always (frame);
      if (frame == NULL)
        break;
    }

  return frame;
}

> @@ -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?

> +
>    if (this_frame->prev_pc.status == CC_UNKNOWN)
>      {
>        if (gdbarch_unwind_pc_p (frame_unwind_arch (this_frame)))



> +
>    /* Make a copy of all the register values unwound from this frame.
>       Save them in a scratch buffer so that there isn't a race between
>       trying to extract the old values from the current regcache while
> @@ -2575,7 +2589,20 @@ frame_unwind_arch (struct frame_info *next_frame)
>  struct gdbarch *
>  frame_unwind_caller_arch (struct frame_info *next_frame)
>  {
> -  return frame_unwind_arch (skip_artificial_frames (next_frame));
> +  struct frame_info *caller;
> +
> +  /* 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" ?

> +# We can step
> +gdb_test "record goto begin" ".*foo.*"
> +gdb_test "stepi" ".*foo_1.*"
> +gdb_test "step" ".*bar.*"
> +gdb_test "stepi" ".*bar_1.*"

Please watch out for duplicate test messages:
https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

Thanks,
Pedro Alves