Re: [PATCH v2 3/5] Change meaning of VALUE_FRAME_ID; rename to VALUE_NEXT_FRAME_ID

[Pedro Alves <palves at redhat dot com>]

On 11/02/2016 10:19 PM, Kevin Buettner wrote:

> @@ -2295,7 +2305,10 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
>    if (ctx.num_pieces > 0)
>      {
>        struct piece_closure *c;
> -      struct frame_id frame_id = get_frame_id (frame);
> +      struct frame_id frame_id
> +        = ((frame == NULL) 

Redundant parens.

> +	   ? null_frame_id
> +	   : get_frame_id (get_next_frame_sentinel_okay (frame)));
>        ULONGEST bit_size = 0;
>        int i;
>  


> diff --git a/gdb/valops.c b/gdb/valops.c
> index 40392e8..f96016f 100644
> --- a/gdb/valops.c
> +++ b/gdb/valops.c
> @@ -1112,8 +1112,15 @@ value_assign (struct value *toval, struct value *fromval)
>  	struct gdbarch *gdbarch;
>  	int value_reg;
>  
> -	/* Figure out which frame this is in currently.  */
> +	/* Figure out which frame this is in currently.
> +	
> +	   We use VALUE_FRAME_ID for obtaining the value's frame id instead of
> +	   VALUE_NEXT_FRAME_ID due to requiring a frame which may be passed  to

Spurious space in "passed  to".

> +	   put_frame_register_bytes() below.  That function will (eventually)
> +	   perform the any necessary unwind operation by first obtaining the next
> +	   frame.  */

"the any necessary" looks like a typo?

>  	frame = frame_find_by_id (VALUE_FRAME_ID (toval));
> +
>  	value_reg = VALUE_REGNUM (toval);
>  
>  	if (!frame)



> @@ -3989,7 +3991,7 @@ value_fetch_lazy (struct value *val)
>  
>        while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
>  	{
> -	  struct frame_id frame_id = VALUE_FRAME_ID (new_val);
> +	  struct frame_id frame_id = VALUE_NEXT_FRAME_ID (new_val);
>  
>  	  frame = frame_find_by_id (frame_id);
>  	  regnum = VALUE_REGNUM (new_val);
> @@ -4004,7 +4006,13 @@ value_fetch_lazy (struct value *val)
>  	  gdb_assert (!gdbarch_convert_register_p (get_frame_arch (frame),
>  						   regnum, type));
>  
> -	  new_val = get_frame_register_value (frame, regnum);
> +	  /* FRAME was obtained, above, via VALUE_NEXT_FRAME_ID. 
> +	     Since a "->next" operation was performed when setting
> +	     this field, we do not need to perform a "next" operation
> +	     again when unwinding the register.  That's why
> +	     frame_unwind_register_value() is called here instead of
> +	     get_frame_register_value().  */
> +	  new_val = frame_unwind_register_value (frame, regnum);

The comment just below needs updating: it's still phrased in terms
of get_frame_register_value.  Also, I suspect that renaming the "frame" and
"frame_id" locals to "next_frame" and "next_frame_id" would allow simplifying
the new comment.

>  
>  	  /* If we get another lazy lval_register value, it means the
>  	     register is found by reading it from the next frame.
> @@ -4018,7 +4026,7 @@ value_fetch_lazy (struct value *val)
>  	     in this situation.  */
>  	  if (VALUE_LVAL (new_val) == lval_register
>  	      && value_lazy (new_val)
> -	      && frame_id_eq (VALUE_FRAME_ID (new_val), frame_id))
> +	      && frame_id_eq (VALUE_NEXT_FRAME_ID (new_val), frame_id))
>  	    internal_error (__FILE__, __LINE__,
>  			    _("infinite loop while fetching a register"));
>  	}


Otherwise LGTM.

Thanks,
Pedro Alves