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: 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: Thu, 04 Feb 2016 16:01:50 +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>
On 01/26/2016 05:08 PM, Markus Metzger wrote:
> In skip_artificial_frames we repeatedly call get_prev_frame_always until we get
> a non-inline and non-tailcall frame assuming that there must be such a frame
> eventually.
>
> For record targets, however, we may have a frame chain that consists only of
> inline or tailcall frames.
>
> This leads to a crash in get_frame_type when we dereference a NULL frame
> pointer.
>
> 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?
> I found two other places where get_frame_type is called in a similar setup to
> skip tailcall frames:
>
> - in infcmd.c to implement the "finish" command
> - in frame.c's pop_frame which is used by the "return" command
>
> In both cases I added a NULL pointer check for the frame and throw an error in
> case we don't find a non-tailcall frame.
> /* 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.
>
> In infcmd I further moved the tailcall-frame-chasing loop to the
> forward-stepping case since we don't need a frame for reverse execution and we
> don't want to fail because of that. Reverse-finish does make sense for a
> tailcall frame.
Thanks,
Pedro Alves