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

["Metzger, Markus T" <markus dot t dot metzger at intel 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