This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 02/10/2016 07:40 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: Pedro Alves [mailto:palves@redhat.com]
>> Sent: Tuesday, February 9, 2016 11:02 PM
>> 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
>>
>> On 02/09/2016 02:42 PM, Metzger, Markus T wrote:
>>
>>>>> 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?

I'm thinking that maybe only "info frame" (frame_info) would trip on it,
but then that one should probably check frame_unwind_caller_id itself
too.

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]