Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Pedro Alves <palves@redhat.com>
To: Kevin Buettner <kevin@buettner.to>, gdb-patches@sourceware.org
Subject: Re: [PATCH v2 3/5] Change meaning of VALUE_FRAME_ID; rename to VALUE_NEXT_FRAME_ID
Date: Wed, 09 Nov 2016 14:48:00 -0000	[thread overview]
Message-ID: <cb05ca92-cd37-ddd2-18bb-081b2377bd3f@redhat.com> (raw)
In-Reply-To: <20161102151929.748d9cf0@pinnacle.lan>

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


  reply	other threads:[~2016-11-09 14:48 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-02 22:11 [PATCH v2 0/5] Prevent more recursion in python based unwinders Kevin Buettner
2016-11-02 22:14 ` [PATCH v2 1/5] Extend test gdb.python/py-recurse-unwind.exp Kevin Buettner
2016-11-09 13:59   ` Pedro Alves
2016-11-16 18:52     ` Kevin Buettner
2016-11-16 22:46       ` Sergio Durigan Junior
2016-11-17 15:27         ` Kevin Buettner
2016-11-02 22:16 ` [PATCH v2 3/5] Distinguish sentinel frame from null frame Kevin Buettner
2016-11-02 22:20   ` Kevin Buettner
2016-11-09 14:48   ` Pedro Alves
2016-11-16 18:54     ` Kevin Buettner
2016-11-02 22:19 ` [PATCH v2 3/5] Change meaning of VALUE_FRAME_ID; rename to VALUE_NEXT_FRAME_ID Kevin Buettner
2016-11-09 14:48   ` Pedro Alves [this message]
2016-11-16 19:08     ` Kevin Buettner
2016-11-02 22:23 ` [PATCH v2 4/5] Make gdb.PendingFrame.read_register handle "user" registers Kevin Buettner
2016-11-16 19:08   ` Kevin Buettner
2016-11-02 22:26 ` [PATCH v2 5/5] Stash frame id of current frame before stashing frame id for previous frame Kevin Buettner
2016-11-09 14:48   ` Pedro Alves
2016-11-16 19:07     ` Kevin Buettner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cb05ca92-cd37-ddd2-18bb-081b2377bd3f@redhat.com \
    --to=palves@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=kevin@buettner.to \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox