This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 3/3] btrace, frame: fix crash in get_frame_type
- From: Pedro Alves <palves at redhat dot com>
- To: Markus Metzger <markus dot t dot metzger at intel dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 09 Feb 2016 12:05:52 +0000
- Subject: Re: [PATCH v2 3/3] btrace, frame: fix crash in get_frame_type
- Authentication-results: sourceware.org; auth=none
- References: <1454681922-2228-1-git-send-email-markus dot t dot metzger at intel dot com> <1454681922-2228-3-git-send-email-markus dot t dot metzger at intel dot 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