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: "Metzger, Markus T" <markus dot t dot metzger at intel dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Wed, 10 Feb 2016 15:34:10 +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> <56B9D620 dot 2020104 at redhat dot com> <A78C989F6D9628469189715575E55B233325FC44 at IRSMSX104 dot ger dot corp dot intel dot com> <56BA61C6 dot 8060807 at redhat dot com> <A78C989F6D9628469189715575E55B233325FF30 at IRSMSX104 dot ger dot corp dot intel dot com> <56BB0A0D dot 80502 at redhat dot com> <A78C989F6D9628469189715575E55B233325FFC6 at IRSMSX104 dot ger dot corp dot intel dot com> <A78C989F6D9628469189715575E55B233326016F at IRSMSX104 dot ger dot corp dot intel dot com>
On 02/10/2016 03:02 PM, Metzger, Markus T wrote:
> No new fails there, as well (64-bit IA).
>
> I added a comment based on your statement that frame_unwind_caller_xxx
> callers should check frame_unwind_caller_id and assert that skip_artificial_frames
> does not return NULL.
>
> Info frame doesn't crash.
>
> (gdb) info frame
> Stack level 0, frame at 0x0:
> rip = 0x4005b0 in bar (tailcall-only.c:29); saved rip = 0x4005c2
> called by frame at 0x0
^^^^^^^^^^^^^^^
> source language c.
> Arglist at unknown address.
> Locals at unknown address,Registers are not available in btrace record history
>
> This is from a tailcall-only frame stack in replay mode using the tailcall-only test.
> The real caller has not been recorded.
Not sure how you got that, since "called by frame" seems to indicates that
the frame was not TAILCALL_FRAME:
else if (get_frame_type (fi) == TAILCALL_FRAME)
puts_filtered (" tail call frame");
else if (get_frame_type (fi) == INLINE_FRAME)
printf_filtered (" inlined into frame %d",
frame_relative_level (get_prev_frame (fi)));
else
{
printf_filtered (" called by frame at ");
fputs_filtered (paddress (gdbarch, get_frame_base (calling_frame_info)),
gdb_stdout);
}
>
> The output isn't very helpful for record btrace since we don't record register and
> memory changes.
So I'm mostly OK with the patch now, but I think you should dig a bit more
into the "info frame" output, since I think you _will_ internal error with a
TAILCALL_FRAME.
My remaining issue is now with the user-visible strings.
> @@ -985,6 +1007,10 @@ frame_pop (struct frame_info *this_frame)
> entering THISFRAME. */
> prev_frame = skip_tailcall_frames (prev_frame);
>
> + /* We cannot pop tailcall frames. */
> + if (prev_frame == NULL)
> + error (_("Cannot pop a tailcall frame."));
> +
I find this confusing, from a user perspective. AFAIK, you can pop a tailcall
frame; what you can't do is pop when you don't know anything about the frame
that started the tail calling. How about:
if (prev_frame == NULL)
error (_("Cannot return: tailcall caller frame not found."));
s/pop/return/, as "pop" is an internal implementation detail.
(I suggest also dropping the redundant comment.)
> + {
> + /* Ignore TAILCALL_FRAME type frames, they were executed already before
> + entering THISFRAME. */
> + frame = skip_tailcall_frames (frame);
> +
> + if (frame == NULL)
> + error (_("\"finish\" not meaningful for tailcall frames."));
> +
if (frame == NULL)
error (_("Cannot finish: tailcall caller frame not found."));
(I'd also be fine with dropping the "Cannot xxx:" part to make
the error messages the same in both cases.)
> + finish_forward (sm, frame);
> + }
> }
Thanks,
Pedro Alves