Re: [PATCH 2/4] Tweak meaning of VALUE_FRAME_ID

[Pedro Alves <palves@redhat.com>]

On 09/28/2016 09:52 AM, Kevin Buettner wrote:
> Tweak meaning of VALUE_FRAME_ID.
> 
> The VALUE_FRAME_ID macro provides access to a member in struct value
> that's used to hold the frame id that's used when determining a
> register's value or when assigning to a register.  The underlying
> member has a long and obscure name.  I won't refer to it here, but
> will simply refer to VALUE_FRAME_ID as if it's the struct value member
> instead of being a convenient macro.
> 
> At the moment, without this patch in place, VALUE_FRAME_ID is set in
> value_of_register_lazy() and several other locations to hold the frame
> id of the frame passed to those functions.
> 
> VALUE_FRAME_ID is used in the lval_register case of
> value_fetch_lazy().  To fetch the register's value, it calls
> get_frame_register_value() which, in turn, calls
> frame_unwind_register_value() with frame->next.
> 
> A python based unwinder may wish to determine the value of a register
> or evaluate an expression containing a register.  When it does this,
> value_fetch_lazy() will be called under some circumstances.  It will
> attempt to determine the frame id associated with the frame passed to
> it.  In so doing, it will end up back in the frame sniffer of the very
> same python unwinder that's attempting to learn the value of a
> register as part of the sniffing operation.  This recursion is not
> desirable.
> 
> As noted above, when value_fetch_lazy() wants to fetch a register's
> value, it does so (indirectly) by unwinding from frame->next.
> 
> With this in mind, a solution suggests itself:  Change VALUE_FRAME_ID
> to hold the frame id associated with the next frame.  Then, when it
> comes time to obtain the value associated with the register, we can
> simply unwind from the frame corresponding to the frame id stored in
> VALUE_FRAME_ID.  This neatly avoids the python unwinder recursion
> problem by changing when the "next" operation occurs.  Instead of the
> "next" operation occuring when the register value is fetched, it
> occurs earlier on when assigning a frame id to VALUE_FRAME_ID.
> (Thanks to Pedro for this suggestion.)
> 

At the very least, The VALUE_FRAME_ID macro's documentation and
the value->frame_id's documentation must be updated to reflect
the new meaning.  Ideally, we'd rename these to VALUE_NEXT_FRAME_ID
and value->frame_id.  But that'd probably be best done as a
separate follow up patch.

> diff --git a/gdb/sentinel-frame.c b/gdb/sentinel-frame.c
> index 6cd1bc3..515650c 100644
> --- a/gdb/sentinel-frame.c
> +++ b/gdb/sentinel-frame.c
> @@ -51,7 +51,7 @@ sentinel_frame_prev_register (struct frame_info *this_frame,
>    struct value *value;
>  
>    value = regcache_cooked_read_value (cache->regcache, regnum);
> -  VALUE_FRAME_ID (value) = get_frame_id (this_frame);
> +  VALUE_FRAME_ID (value) = get_frame_id (get_next_frame_sentinel_okay (this_frame));

This looks a bit odd.  I think it only works because getting the
next of the sentinel frame returns back the sentinel frame again.

How about we write directly:

  VALUE_FRAME_ID (value) = sentinel_frame_id;

With that, maybe we could make it an internal error to
call get_next_frame_sentinel_okay on the sentinel frame?

Otherwise this look OK to me, but I'd like to understand
better the need for get_frame_id call in the other patch.
Maybe there's something else that we should be doing.

Thanks,
Pedro Alves