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: "Metzger, Markus T" <markus dot t dot metzger at intel dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Wed, 10 Feb 2016 10:29:26 +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>
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Wednesday, February 10, 2016 11:00 AM
> 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
> >>>>> 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.
> >>
> >> Yes, please.
> >>
> >> Though, I think all these frame_unwind_caller_XXX methods should be
> >> consistent in how they handle skip_artificial_frames (this_frame)
> >> returning NULL, because they're all called together, assuming they're
> >> referring to the same frame. If we throw error here, then I think we
> >> should throw in frame_unwind_caller_arch too, instead of having that
> >> one return the arch of the next frame.
> >
> > get_frame_arch and frame_unwind_arch don't seem to throw any error
> today.
>
> Because they assume there is always a prev frame.
>
> The case under discussion is different, it's about getting at the "real caller
> frame". Something has to change.
>
> > I'd rather not introduce new exceptions if not strictly necessary.
> > Its callers may not be prepared to handle them.
>
> Callers shouldn't be handed back potentially bogus results. If there's no
> return that makes sense, then throw and let the callers handle it (or don't
> handle it and let the user know what she tried to do doesn't make sense), or
> internal-error.
>
> >
> > In the absence of an arch unwinder, frame_unwind_arch uses the gdbarch
> > of the next frame. And DWARF tailcall frames use the gdbarch of the
> > bottom non-tailcall frame. This is consistent with the proposed changes.
>
> In that case, there _is_ a prev frame, and then we default to assuming the
> unwound arch is the same as the next frame's arch. But in this situation at
> hand, the difference is that we found that there's _no_ "real caller frame" at
> all.
>
> >
> > We may want to return frame_unwind_arch (next_frame) instead of
> > get_frame_arch (next_frame). OTOH gdb/dwarf2-frame-tailcall.c's
> > tailcall_frame_prev_arch returns get_frame_arch (cache-
> >next_bottom_frame).
> > I'm currently mimicking that behavior.
>
> See above. That situation looks similar on the surface, but the distinction is
> important. The "frame_unwind_CALLER" methods want to drill down past
> artificial frames, and return info on the "real caller", while the
> "frame_unwind" methods want to unwind one frame only, no matter
> artificial or not.
>
> All the frame_unwind_caller_XXX methods should retrieve information
> about the same frame that frame_unwind_caller_id returns. They should all
> really be guarded by a frame_unwind_caller_id check, like:
>
> if (frame_id_p (frame_unwind_caller_id (frame)))
> {
> scope_breakpoint
> = create_internal_breakpoint (frame_unwind_caller_arch (frame),
> frame_unwind_caller_pc (frame),
> bp_watchpoint_scope,
> &momentary_breakpoint_ops);
>
> and indeed most are. Those that aren't, either should be, or are not because
> they're called when we know the current frame is a non-artificial frame.
>
> Put another way, all the frame_unwind_caller_xxx methods should behave
> merely as convenience to writing the alternative form that has the caller skip
> artificial frames itself:
>
> frame = skip_artificial_frames (frame);
> if (frame_id_p (get_frame_id (frame)))
> {
> scope_breakpoint
> = create_internal_breakpoint (get_frame_arch (frame),
> get_frame_pc (frame),
> bp_watchpoint_scope,
> &momentary_breakpoint_ops);
>
>
> As such, if something calls frame_unwind_caller_pc|arch directly without
> checking whether frame_unwind_caller_id returns a valid frame, then the
> caller code should not continue as if there was indeed a valid caller frame.
> Throwing an error in that case is better than silently continuing with mocked
> info.
>
> So, what would break if we internal_error'ed in
> frame_unwind_caller_arch/pc instead or normal error, in the case that
> there's no non-artificial frame?
Likely nothing. The case of only-artificial-frames is not covered by the test
suite. I'm not sure it is even possible during live debug (ignoring the discussion
about get_prev_frame_always).
The changes were not motivated by fails but by my attempt to make the callers
of skip_artificial_frames handle the now-possible NULL return gracefully.
I ran the btrace suite including the new tests I added to cover the record btrace
tailcall-only-frames case and they pass. I'm running the full test suite now.
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