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
On Wed, 9 Nov 2016 14:48:35 +0000
Pedro Alves <palves@redhat.com> wrote:
> 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.
Fixed.
> > + 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".
Fixed.
> > + 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?
Agreed. I removed "any " from the comment.
> 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.
I made the next_frame and next_frame_id changes that you suggested.
Here's the change for the comment:
/* If we get another lazy lval_register value, it means the
- register is found by reading it from the next frame.
- get_frame_register_value should never return a value with
- the frame id pointing to FRAME. If it does, it means we
+ register is found by reading it from NEXT_FRAME's next frame.
+ frame_unwind_register_value should never return a value with
+ the frame id pointing to NEXT_FRAME. If it does, it means we
either have two consecutive frames with the same frame id
in the frame chain, or some code is trying to unwind
behind get_prev_frame's back (e.g., a frame unwind
> Otherwise LGTM.
Thanks again for the review.
Pushed.
Kevin