[2/4] Tweak meaning of VALUE_FRAME_ID

  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.)

This patch implements this idea.

It builds on the patch "Distinguish sentinel frame from null frame".
Without that work in place, it's necessary to check for null_id at
several places and then obtain the sentinel frame.

In the course of developing this patch, I found that the lval_register
case in value_assign() required some special care.  It was passing the
frame associated with VALUE_FRAME_ID (for the value being assigned) to
put_frame_register_bytes().  But put_frame_register_bytes() calls
put_frame_register(), which calls frame_register, which does
frame_register_unwind() on frame->next.  I briefly considered adjusting
frame_register_unwind to work on the frame passed to it instead of
frame->next, but this would have required more extensive changes
throughout more of GDB.  Instead, I opted for changing value_assign()
so that frame->prev is passed to put_frame_register_bytes().

gdb/ChangeLog:

	    * findvar.c (value_of_register_lazy): VALUE_FRAME_ID for
	    register value now refers to the next frame.
	    (default_value_from_register): Likewise.
	    (value_from_register): Likewise.
	    * frame_unwind.c (frame_unwind_got_optimized): Likewise.
	    * sentinel-frame.c (sentinel_frame_prev_register): Likewise.
	    * valops.c (value_assign): Obtain `prev' frame for passing
	    to put_frame_register_bytes().
	    * value.c (value_fetch_lazy): Call frame_unwind_register_value()
	    instead of get_frame_register_value().
---
 gdb/findvar.c        | 22 ++++++++++++++++++----
 gdb/frame-unwind.c   |  2 +-
 gdb/sentinel-frame.c |  2 +-
 gdb/valops.c         | 11 +++++++++++
 gdb/value.c          |  2 +-
 5 files changed, 32 insertions(+), 7 deletions(-)
  

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