Re: [patch] Convert frame_stash to a hash table

[Pedro Alves <palves@redhat.com>]

On 05/16/2013 02:50 PM, Phil Muldoon wrote:
> On 16/05/13 14:42, Pedro Alves wrote:
>> On 05/16/2013 02:09 PM, Phil Muldoon wrote:
>>
>> When doing a backtrace, you'll end up linearly walking the frame
>> chain, and normally you don't go back to newer frames -- unwind a
>> frame (frame.prev()), print info about it, unwind the next, print it,
>> on and on.  As such, a single frame stashed in the frame stash should be
>> sufficient.  But it's not.  frapy_older does:
> 
> When using frame filters, in the case of eliding frames this may not
> be the case.  In fact we cannot predict how frame filters will
> navigate the stack.

For sure.  However, I think in your backtrace example, the frame
filter actually did nothing, correct?

> 
>  
>>   TRY_CATCH (except, RETURN_MASK_ALL)
>>     {
>>
>>       /* Try to get the previous frame, to determine if this is the last frame
>> 	 in a corrupt stack.  If so, we need to store the frame_id of the next
>> 	 frame and not of this one (which is possibly invalid).  */
>>       if (get_prev_frame (frame) == NULL
>> 	  && get_frame_unwind_stop_reason (frame) != UNWIND_NO_REASON
>> 	  && get_next_frame (frame) != NULL)
>> 	{
>> 	  frame_obj->frame_id = get_frame_id (get_next_frame (frame));
>> 	  frame_obj->frame_id_is_next = 1;
>> 	}
> 
> 
> Yes, this is bogus.  But even if you remove this, the performance hits
> still register as significant.

I'd expected that a simple filter (like I imagine yours was)
you'd not see any performance hit.

> 
>> and given the present frame stash can only hold one frame,
>> these get_prev_frame/get_next_frame calls constantly invalidate it. 
>> Now, I don't get this "detect corrupt stack" code at all.
> 
> Me either, it should be removed.  Hiding the corrupt stack from a
> Python consumer seems all kinds of wrong.  I am going to fix this
> next.  I decided not to include it in this patch, as I wanted the
> focus to be on frame_stash issues where Python scripts can randomly
> access frame from all over the stack.

OK.  Again, I'm not questioning the merit of the patch, but the
example/rationale.  :-)  Personally, I'd rather that was fixed first,
and then the new frame hash stash justified/explained with
with an example where gdb's inefficiencies are exposed even when
gdb's python code is sane.  :-)

> Take this example
> 
> f = gdb.newest_frame()
> 
> do some other inferior operations happen, stop.
> 
> g = gdb.newest_frame()
> 
> Now is I access f, say f.type(), that will not be in the frame_stash,
> it was from awhile ago.  These kinds of patterns do crop up in frame
> filters, because we are filtering, eliding frames.

I'm confused.  :-)  If you do other inferior operations
that resume the inferior, then the new hash stash won't help either.
Resuming the inferior always invalidates all frames, along with the
stash.

-- 
Pedro Alves