From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id BC92B3858C1F for ; Tue, 22 Nov 2022 19:56:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BC92B3858C1F Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.0.64] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 1E1C01E0CB; Tue, 22 Nov 2022 14:56:09 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1669146969; bh=Yn+MMVzRXkGws6vXMk8ZK4NYjb0x7AksNzYj8gWO6H4=; h=Date:Subject:To:References:From:In-Reply-To:From; b=alRfK+ElI2LeLhsQwwEAlYJHHT8es3tTpkIObOlgxI9css0z2mLZS05WCrUqaugqd hDMznRTMZcZjkLOAx3vzHXtLSSDpOMZeIWz1bay9nmai+VK2jQS/Xz/nI1fy7tQqSh CrJbWI+iHQUDAAB7RCo9tBxIkPq5nfw4GOpKaEZE= Message-ID: Date: Tue, 22 Nov 2022 14:56:08 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 Subject: Re: [PATCH 1/5] regcache: Add collect/supply_regset variants that accept a register base. Content-Language: fr To: John Baldwin , gdb-patches@sourceware.org References: <20220708005816.9408-1-jhb@FreeBSD.org> <20220708005816.9408-2-jhb@FreeBSD.org> From: Simon Marchi In-Reply-To: <20220708005816.9408-2-jhb@FreeBSD.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 7/7/22 20:58, John Baldwin wrote: > Some register sets described by an array of regcache_map_entry > structures do not have fixed register numbers in their associated > architecture but do describe a block of registers whose numbers are at > fixed offsets relative to some base register value. An example of > this are the TLS register sets for the ARM and AArch64 architectures. > > Currently OS-specific architectures create register maps and register > sets dynamically using the register base number. However, this > requires duplicating the code to create the register map and register > set. To reduce duplication, add variants of the collect_regset and > supply_regset regcache methods which accept a base register number. > For valid register map entries (i.e. not REGCACHE_MAP_SKIP), add this > base register number to the value from the map entry to determine the > final register number. The patch LGTM. I have some small comments, once addressed you can put my: Approved-By: Simon Marchi > --- > gdb/regcache.c | 28 +++++++++++++++++++++++++--- > gdb/regcache.h | 24 ++++++++++++++++++++++-- > 2 files changed, 47 insertions(+), 5 deletions(-) > > diff --git a/gdb/regcache.c b/gdb/regcache.c > index 037659ef8fa..1db3d972ef8 100644 > --- a/gdb/regcache.c > +++ b/gdb/regcache.c > @@ -1180,7 +1180,7 @@ regcache::transfer_regset_register (struct regcache *out_regcache, int regnum, > /* See regcache.h. */ > > void > -regcache::transfer_regset (const struct regset *regset, > +regcache::transfer_regset (const struct regset *regset, int regbase, > struct regcache *out_regcache, > int regnum, const gdb_byte *in_buf, > gdb_byte *out_buf, size_t size) const > @@ -1195,6 +1195,9 @@ regcache::transfer_regset (const struct regset *regset, > int regno = map->regno; > int slot_size = map->size; > > + if (regno != REGCACHE_MAP_SKIP) > + regno += regbase; > + > if (slot_size == 0 && regno != REGCACHE_MAP_SKIP) > slot_size = m_descr->sizeof_register[regno]; > > @@ -1242,7 +1245,18 @@ void > regcache::supply_regset (const struct regset *regset, > int regnum, const void *buf, size_t size) > { > - transfer_regset (regset, this, regnum, (const gdb_byte *) buf, nullptr, size); > + transfer_regset (regset, 0, this, regnum, (const gdb_byte *) buf, nullptr, > + size); > +} Can the old regcache::{supply,collect}_regset (without regbase) become trivial wrappers around the new ones (with regbase)? I would put the implementation directly in the .h if doing that. > + > +/* See regcache.h. */ > + > +void > +regcache::supply_regset (const struct regset *regset, int regbase, > + int regnum, const void *buf, size_t size) > +{ > + transfer_regset (regset, regbase, this, regnum, (const gdb_byte *) buf, > + nullptr, size); > } > > /* Collect register REGNUM from REGCACHE to BUF, using the register > @@ -1261,11 +1275,19 @@ void > regcache::collect_regset (const struct regset *regset, > int regnum, void *buf, size_t size) const > { > - transfer_regset (regset, nullptr, regnum, nullptr, (gdb_byte *) buf, size); > + transfer_regset (regset, 0, nullptr, regnum, nullptr, (gdb_byte *) buf, size); > } > > /* See regcache.h */ > > +void > +regcache::collect_regset (const struct regset *regset, int regbase, > + int regnum, void *buf, size_t size) const > +{ > + transfer_regset (regset, regbase, nullptr, regnum, nullptr, (gdb_byte *) buf, > + size); > +} > + > bool > regcache_map_supplies (const struct regcache_map_entry *map, int regnum, > struct gdbarch *gdbarch, size_t size) > diff --git a/gdb/regcache.h b/gdb/regcache.h > index 1dbba5ce9af..f01127b20fb 100644 > --- a/gdb/regcache.h > +++ b/gdb/regcache.h > @@ -150,6 +150,14 @@ extern void regcache_collect_regset (const struct regset *regset, > int regnum, void *buf, size_t size); > > > +extern void regcache_supply_regset (const struct regset *regset, int regbase, > + struct regcache *regcache, > + int regnum, const void *buf, > + size_t size); > +extern void regcache_collect_regset (const struct regset *regset, int regbase, > + const struct regcache *regcache, > + int regnum, void *buf, size_t size); These don't have a definition, I guess they are not needed. (Could we make a patch that removes regcache_supply_regset and regcache_collect_regset, since they just forward to the regcache methods?) > + belonging to the regset, otherwise just the register numbered > + REGNUM. The REGSET's 'regmap' field must point to an array of > + 'struct regcache_map_entry'. The valid register numbers in each > + entry in 'struct regcache_map_entry' are offset by REGBASE. */ > + > + void supply_regset (const struct regset *regset, int regbase, > + int regnum, const void *buf, size_t size); > + > + void collect_regset (const struct regset *regset, int regbase, int regnum, > + void *buf, size_t size) const; > + > void supply_regset (const struct regset *regset, > int regnum, const void *buf, size_t size); > > - > void collect_regset (const struct regset *regset, int regnum, > void *buf, size_t size) const; Can you document the last two, just by saying something like "Same as the above, but with REGBASE == 0"? Simon