From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29444 invoked by alias); 9 Nov 2016 14:48:42 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 25579 invoked by uid 89); 9 Nov 2016 14:48:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.8 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 09 Nov 2016 14:48:38 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0BB6D8FD02; Wed, 9 Nov 2016 14:48:37 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uA9EmZrC027905; Wed, 9 Nov 2016 09:48:36 -0500 Subject: Re: [PATCH v2 3/5] Change meaning of VALUE_FRAME_ID; rename to VALUE_NEXT_FRAME_ID To: Kevin Buettner , gdb-patches@sourceware.org References: <20161102151111.2462c806@pinnacle.lan> <20161102151929.748d9cf0@pinnacle.lan> From: Pedro Alves Message-ID: Date: Wed, 09 Nov 2016 14:48:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20161102151929.748d9cf0@pinnacle.lan> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-11/txt/msg00216.txt.bz2 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