From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 4PnPBVU6sV+7QwAAWB0awg (envelope-from ) for ; Sun, 15 Nov 2020 09:25:25 -0500 Received: by simark.ca (Postfix, from userid 112) id 152B41F08B; Sun, 15 Nov 2020 09:25:25 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 668431E552 for ; Sun, 15 Nov 2020 09:25:24 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D0EEE385782C; Sun, 15 Nov 2020 14:25:23 +0000 (GMT) Received: from rock.gnat.com (rock.gnat.com [205.232.38.15]) by sourceware.org (Postfix) with ESMTP id 1D705385782C for ; Sun, 15 Nov 2020 14:25:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1D705385782C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=brobecker@adacore.com Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id ABEE1117ED0; Sun, 15 Nov 2020 09:25:03 -0500 (EST) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id e-2pjWweL0-n; Sun, 15 Nov 2020 09:25:03 -0500 (EST) Received: from float.home (localhost.localdomain [127.0.0.1]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id 4ED29117C99; Sun, 15 Nov 2020 09:25:03 -0500 (EST) Received: by float.home (Postfix, from userid 1000) id 74D8FA1870; Sun, 15 Nov 2020 18:24:58 +0400 (+04) Date: Sun, 15 Nov 2020 18:24:58 +0400 From: Joel Brobecker To: Andrew Burgess Subject: Re: [PATCH] gdb: user variables with components of dynamic type Message-ID: <20201115142458.GA509764@adacore.com> References: <20201022153238.1947197-1-andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201022153238.1947197-1-andrew.burgess@embecosm.com> X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" Hi Andrew, > - /* If type has a dynamic resolved location property > - update it's value address. */ > + /* If either the WHOLE value, or the COMPONENT value has a dynamic > + resolved location property then update the address of the COMPONENT. > + > + If the COMPONENT itself has a dynamic location, and was an > + lval_internalvar_component, then we change this to lval_memory. > + Usually a component of an internalvar is created non-lazy, and has its > + content immediately copied from the parent internalvar. However, > + for components with a dynamic location, the content of the component > + is not contained within the parent, but is instead accessed > + indirectly. Further, the component will be created as a lazy value. > + > + By changing the type of the component to lval_memory we ensure that > + value_fetch_lazy can successfully load the component. */ > type = value_type (whole); > if (NULL != TYPE_DATA_LOCATION (type) > && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST) > set_value_address (component, TYPE_DATA_LOCATION_ADDR (type)); > + > + type = value_type (component); > + if (NULL != TYPE_DATA_LOCATION (type) > + && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST) > + { > + if (VALUE_LVAL (component) == lval_internalvar_component) > + { > + gdb_assert (value_lazy (component)); > + VALUE_LVAL (component) = lval_memory; > + } > + else > + gdb_assert (VALUE_LVAL (component) == lval_memory); > + set_value_address (component, TYPE_DATA_LOCATION_ADDR (type)); > + } I have a suggestion, but I am not sure it might be right for everyone, as perhaps other people's brains might be thinking differently. In your patch you architected it with one large comment at beginning followed by a number of conditinal branches, with the comment explaining the various scenarios that we're about to handle. If your brain works like mine, I would find the following approach to make it easier for me to understand the code: type = value_type (component); if (NULL != TYPE_DATA_LOCATION (type) && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST) { /* COMPONENT's type has a dynamic location, so we need to update our component's address to reflect the actual location after resolution. */ if (VALUE_LVAL (component) == lval_internalvar_component) { /* This happens when [...]. We have to change the lval to lval_memory because ... */ gdb_assert (value_lazy (component)); VALUE_LVAL (component) = lval_memory; } /* At this point, we assume that COMPONENT is now an lval_memory, and we can now set it address. */ gdb_assert (VALUE_LVAL (component) == lval_memory); set_value_address (component, TYPE_DATA_LOCATION_ADDR (type)); } As I mentioned, maybe you don't think the read code the same way as I do, and so it would be absolutely fine with me if you don't agree with the suggestion ;-). -- Joel