This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 3/5] Change meaning of VALUE_FRAME_ID; rename to VALUE_NEXT_FRAME_ID
- From: Pedro Alves <palves at redhat dot com>
- To: Kevin Buettner <kevin at buettner dot to>, gdb-patches at sourceware dot org
- Date: Wed, 9 Nov 2016 14:48:35 +0000
- Subject: Re: [PATCH v2 3/5] Change meaning of VALUE_FRAME_ID; rename to VALUE_NEXT_FRAME_ID
- Authentication-results: sourceware.org; auth=none
- References: <20161102151111.2462c806@pinnacle.lan> <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