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
next prev parent 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