From patchwork Fri Jun 8 14:14:06 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Alan Hayward X-Patchwork-Id: 27717 Received: (qmail 87560 invoked by alias); 8 Jun 2018 14:14:14 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 87549 invoked by uid 89); 8 Jun 2018 14:14:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LOTSOFHASH, MIME_BASE64_BLANKS, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 spammy=Transfer X-HELO: EUR01-HE1-obe.outbound.protection.outlook.com Received: from mail-he1eur01on0065.outbound.protection.outlook.com (HELO EUR01-HE1-obe.outbound.protection.outlook.com) (104.47.0.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 08 Jun 2018 14:14:10 +0000 Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com (10.172.226.148) by DB6PR0802MB2247.eurprd08.prod.outlook.com (10.172.227.144) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.797.11; Fri, 8 Jun 2018 14:14:06 +0000 Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com ([fe80::d984:bdee:1856:c64]) by DB6PR0802MB2133.eurprd08.prod.outlook.com ([fe80::d984:bdee:1856:c64%7]) with mapi id 15.20.0841.011; Fri, 8 Jun 2018 14:14:06 +0000 From: Alan Hayward To: Simon Marchi CC: "gdb-patches@sourceware.org" , nd Subject: Re: [PATCH v2 03/10] Add reg_buffer_common Date: Fri, 8 Jun 2018 14:14:06 +0000 Message-ID: References: <20180606151629.36602-1-alan.hayward@arm.com> <20180606151629.36602-4-alan.hayward@arm.com> <17367496-22c7-d61c-6800-cbdfd856f308@ericsson.com> In-Reply-To: <17367496-22c7-d61c-6800-cbdfd856f308@ericsson.com> authentication-results: spf=none (sender IP is ) smtp.mailfrom=Alan.Hayward@arm.com; x-ms-publictraffictype: Email x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-ht: Tenant x-ms-traffictypediagnostic: DB6PR0802MB2247: nodisclaimer: True x-exchange-antispam-report-test: UriScan:(37575265505322); x-ms-exchange-senderadcheck: 1 x-forefront-prvs: 06973FFAD3 received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-ID: <24B980DFECA16349B07C8899514A9DBF@eurprd08.prod.outlook.com> MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: fe223327-c598-4510-cf48-08d5cd4a1859 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: fe223327-c598-4510-cf48-08d5cd4a1859 X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Jun 2018 14:14:06.1114 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0802MB2247 X-IsSubscribed: yes > On 7 Jun 2018, at 21:18, Simon Marchi wrote: > > Hi Alan, > > Just some quick comments. > >> --- >> gdb/common/common-regcache.h | 8 ++++++++ >> gdb/gdbserver/regcache.c | 47 ++++++++++++++++++++++++++++++++------------ >> gdb/gdbserver/regcache.h | 18 +++++++++++------ >> gdb/regcache.h | 19 ++++++++++++++---- >> 4 files changed, 69 insertions(+), 23 deletions(-) >> >> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h >> index 696ba00955..487da0a7fb 100644 >> --- a/gdb/common/common-regcache.h >> +++ b/gdb/common/common-regcache.h >> @@ -62,4 +62,12 @@ extern enum register_status regcache_raw_read_unsigned >> >> ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum); >> >> +struct reg_buffer_common >> +{ >> + virtual ~reg_buffer_common () = default; >> + virtual void raw_supply (int regnum, const void *buf) = 0; >> + virtual void raw_collect (int regnum, void *buf) const = 0; >> + virtual register_status get_register_status (int regnum) const = 0; >> +}; > > Ideally, we would gather the documentation for these methods here. Where they > are implemented/overriden, we can maybe add a reference such as > > /* See struct reg_buffer_common. */ > > ? Agreed. Updated all the comments for all the moved functions. I noticed the comment for transfer_regset had become detached from the function, so I move it back to the right place. > >> diff --git a/gdb/regcache.h b/gdb/regcache.h >> index 3edddf47e1..b559a10752 100644 >> --- a/gdb/regcache.h >> +++ b/gdb/regcache.h >> @@ -139,7 +139,7 @@ typedef struct cached_reg >> >> /* Buffer of registers. */ >> >> -class reg_buffer >> +class reg_buffer : public reg_buffer_common >> { >> public: >> reg_buffer (gdbarch *gdbarch, bool has_pseudo); >> @@ -151,13 +151,24 @@ public: >> >> /* Get the availability status of the value of register REGNUM in this >> buffer. */ >> - enum register_status get_register_status (int regnum) const; >> + enum register_status get_register_status (int regnum) const override; >> >> virtual ~reg_buffer () >> { >> xfree (m_registers); >> xfree (m_register_status); >> } >> + >> + virtual void raw_supply (int regnum, const void *buf) override >> + { >> + gdb_assert (false); >> + } >> + >> + virtual void raw_collect (int regnum, void *buf) const override >> + { >> + gdb_assert (false); >> + } > > Hmm, I understand why you need to do this right now. But what do you think of the > idea of moving the supply and collect implementations up to reg_buffer? I think > that the supply/collect operations are a good fit to go in reg_buffer. Essentially > they just peek/poke in the buffer. The regcache layer's responsibility is then to > use that register buffer to implement a cache in front of the target registers, > and offer the API to properly read/write registers (including pseudo ones). > > For reference here's the patch in the regcache-for-alan branch that did this: > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=0e04bb35765d2717818dddd88328cb975f417b2c;hp=ca9f66e37913be55abaea44813a768b40673a39a > I’m happy with that. I hadn’t that there was no methods for copying in and out of reg_buffer. Your change improves that. Ok, so updated with changes as above. Are you ok with this version? diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h index 696ba00955bcdec85ab2a1abd7e030cd10418524..29d9a81182ad1a5797b080e136b682fe59ecef37 100644 --- a/gdb/common/common-regcache.h +++ b/gdb/common/common-regcache.h @@ -62,4 +62,19 @@ extern enum register_status regcache_raw_read_unsigned ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum); +struct reg_buffer_common +{ + virtual ~reg_buffer_common () = default; + + /* Get the availability status of the value of register REGNUM in this + buffer. */ + virtual register_status get_register_status (int regnum) const = 0; + + /* Supply register REGNUM, whose contents are stored in BUF, to REGCACHE. */ + virtual void raw_supply (int regnum, const void *buf) = 0; + + /* Collect register REGNUM from REGCACHE and store its contents in BUF. */ + virtual void raw_collect (int regnum, void *buf) const = 0; +}; + #endif /* COMMON_REGCACHE_H */ diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h index 2c0df648f6d439517b56844d00b1bf0a1c9eafd1..352c1df3f9eeacd622a3d5d668ff4ea98aea9c6f 100644 --- a/gdb/gdbserver/regcache.h +++ b/gdb/gdbserver/regcache.h @@ -28,23 +28,32 @@ struct target_desc; inferior; this is primarily for simplicity, as the performance benefit is minimal. */ -struct regcache +struct regcache : public reg_buffer_common { /* The regcache's target description. */ - const struct target_desc *tdesc; + const struct target_desc *tdesc = nullptr; /* Whether the REGISTERS buffer's contents are valid. If false, we haven't fetched the registers from the target yet. Not that this register cache is _not_ pass-through, unlike GDB's. Note that "valid" here is unrelated to whether the registers are available in a traceframe. For that, check REGISTER_STATUS below. */ - int registers_valid; - int registers_owned; - unsigned char *registers; + int registers_valid = 0; + int registers_owned = 0; + unsigned char *registers = nullptr; #ifndef IN_PROCESS_AGENT /* One of REG_UNAVAILBLE or REG_VALID. */ - unsigned char *register_status; + unsigned char *register_status = nullptr; #endif + + /* See common/common-regcache.h. */ + enum register_status get_register_status (int regnum) const override; + + /* See common/common-regcache.h. */ + void raw_supply (int regnum, const void *buf) override; + + /* See common/common-regcache.h. */ + void raw_collect (int regnum, void *buf) const override; }; struct regcache *init_register_cache (struct regcache *regcache, diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c index 0718b9f9a04d58577f71ab14887e18dc05fee018..83837525a18c1dc96b6e9d24397064e247ced459 100644 --- a/gdb/gdbserver/regcache.c +++ b/gdb/gdbserver/regcache.c @@ -159,7 +159,7 @@ init_register_cache (struct regcache *regcache, struct regcache * new_register_cache (const struct target_desc *tdesc) { - struct regcache *regcache = XCNEW (struct regcache); + struct regcache *regcache = new struct regcache; gdb_assert (tdesc->registers_size != 0); @@ -174,7 +174,7 @@ free_register_cache (struct regcache *regcache) if (regcache->registers_owned) free (regcache->registers); free (regcache->register_status); - free (regcache); + delete regcache; } } @@ -300,35 +300,37 @@ regcache_register_size (const struct regcache *regcache, int n) } static unsigned char * -register_data (struct regcache *regcache, int n, int fetch) +register_data (const struct regcache *regcache, int n, int fetch) { return (regcache->registers + find_register_by_number (regcache->tdesc, n).offset / 8); } -/* Supply register N, whose contents are stored in BUF, to REGCACHE. - If BUF is NULL, the register's value is recorded as - unavailable. */ - void supply_register (struct regcache *regcache, int n, const void *buf) +{ + return regcache->raw_supply (n, buf); +} + +/* See common/common-regcache.h. */ + +void +regcache::raw_supply (int n, const void *buf) { if (buf) { - memcpy (register_data (regcache, n, 0), buf, - register_size (regcache->tdesc, n)); + memcpy (register_data (this, n, 0), buf, register_size (tdesc, n)); #ifndef IN_PROCESS_AGENT - if (regcache->register_status != NULL) - regcache->register_status[n] = REG_VALID; + if (register_status != NULL) + register_status[n] = REG_VALID; #endif } else { - memset (register_data (regcache, n, 0), 0, - register_size (regcache->tdesc, n)); + memset (register_data (this, n, 0), 0, register_size (tdesc, n)); #ifndef IN_PROCESS_AGENT - if (regcache->register_status != NULL) - regcache->register_status[n] = REG_UNAVAILABLE; + if (register_status != NULL) + register_status[n] = REG_UNAVAILABLE; #endif } } @@ -410,8 +412,15 @@ supply_register_by_name (struct regcache *regcache, void collect_register (struct regcache *regcache, int n, void *buf) { - memcpy (buf, register_data (regcache, n, 1), - register_size (regcache->tdesc, n)); + regcache->raw_collect (n, buf); +} + +/* See common/common-regcache.h. */ + +void +regcache::raw_collect (int n, void *buf) const +{ + memcpy (buf, register_data (this, n, 1), register_size (tdesc, n)); } enum register_status @@ -480,3 +489,16 @@ regcache_write_pc (struct regcache *regcache, CORE_ADDR pc) } #endif + +/* See common/common-regcache.h. */ + +enum register_status +regcache::get_register_status (int regnum) const +{ +#ifndef IN_PROCESS_AGENT + gdb_assert (regnum >= 0 && regnum < tdesc->reg_defs.size ()); + return (enum register_status) (register_status[regnum]); +#else + return REG_VALID; +#endif +} diff --git a/gdb/regcache.h b/gdb/regcache.h index 3edddf47e12f8e9ecb6528bdac061d4135df2d5b..4e5659b25bc530a4ced32520825136534be2e642 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -139,7 +139,7 @@ typedef struct cached_reg /* Buffer of registers. */ -class reg_buffer +class reg_buffer : public reg_buffer_common { public: reg_buffer (gdbarch *gdbarch, bool has_pseudo); @@ -149,9 +149,42 @@ public: /* Return regcache's architecture. */ gdbarch *arch () const; - /* Get the availability status of the value of register REGNUM in this - buffer. */ - enum register_status get_register_status (int regnum) const; + /* See common/common-regcache.h. */ + enum register_status get_register_status (int regnum) const override; + + /* See common/common-regcache.h. */ + void raw_collect (int regnum, void *buf) const override; + + /* Collect register REGNUM from REGCACHE. Store collected value as an integer + at address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED. + If ADDR_LEN is greater than the register size, then the integer will be + sign or zero extended. If ADDR_LEN is smaller than the register size, then + the most significant bytes of the integer will be truncated. */ + void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len, + bool is_signed) const; + + /* See common/common-regcache.h. */ + void raw_supply (int regnum, const void *buf) override; + + void raw_supply (int regnum, const reg_buffer &src) + { + raw_supply (regnum, src.register_buffer (regnum)); + } + + /* Supply register REGNUM to REGCACHE. Value to supply is an integer stored + at address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED. + If the register size is greater than ADDR_LEN, then the integer will be + sign or zero extended. If the register size is smaller than the integer, + then the most significant bytes of the integer will be truncated. */ + void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len, + bool is_signed); + + /* Supply register REGNUM with zeroed value to REGCACHE. This is not the same + as calling raw_supply with NULL (which will set the state to + unavailable). */ + void raw_supply_zeroed (int regnum); + + void invalidate (int regnum); virtual ~reg_buffer () { @@ -234,24 +267,9 @@ public: : readable_regcache (gdbarch, has_pseudo) {} - /* Supply register REGNUM, whose contents are stored in BUF, to REGCACHE. */ - void raw_supply (int regnum, const void *buf); - - void raw_supply (int regnum, const reg_buffer &src) - { - raw_supply (regnum, src.register_buffer (regnum)); - } - void raw_update (int regnum) override {} - void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len, - bool is_signed); - - void raw_supply_zeroed (int regnum); - - void invalidate (int regnum); - DISABLE_COPY_AND_ASSIGN (detached_regcache); }; @@ -292,12 +310,6 @@ public: void raw_update (int regnum) override; - /* Collect register REGNUM from REGCACHE and store its contents in BUF. */ - void raw_collect (int regnum, void *buf) const; - - void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len, - bool is_signed) const; - /* Partial transfer of raw registers. Perform read, modify, write style operations. */ void raw_write_part (int regnum, int offset, int len, const gdb_byte *buf); diff --git a/gdb/regcache.c b/gdb/regcache.c index a914b548cadce1d375f5d72b0936b2258b90e08f..032fef0d34ac635c96dd6c39426f5c6d04f28095 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -320,6 +320,8 @@ regcache::restore (readonly_detached_regcache *src) } } +/* See common/common-regcache.h. */ + enum register_status reg_buffer::get_register_status (int regnum) const { @@ -329,7 +331,7 @@ reg_buffer::get_register_status (int regnum) const } void -detached_regcache::invalidate (int regnum) +reg_buffer::invalidate (int regnum) { assert_regnum (regnum); m_register_status[regnum] = REG_UNKNOWN; @@ -879,8 +881,10 @@ regcache::cooked_write_part (int regnum, int offset, int len, write_part (regnum, offset, len, buf, false); } +/* See common/common-regcache.h. */ + void -detached_regcache::raw_supply (int regnum, const void *buf) +reg_buffer::raw_supply (int regnum, const void *buf) { void *regbuf; size_t size; @@ -905,15 +909,11 @@ detached_regcache::raw_supply (int regnum, const void *buf) } } -/* Supply register REGNUM to REGCACHE. Value to supply is an integer stored at - address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED. If - the register size is greater than ADDR_LEN, then the integer will be sign or - zero extended. If the register size is smaller than the integer, then the - most significant bytes of the integer will be truncated. */ +/* See regcache.h. */ void -detached_regcache::raw_supply_integer (int regnum, const gdb_byte *addr, - int addr_len, bool is_signed) +reg_buffer::raw_supply_integer (int regnum, const gdb_byte *addr, + int addr_len, bool is_signed) { enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch); gdb_byte *regbuf; @@ -929,12 +929,10 @@ detached_regcache::raw_supply_integer (int regnum, const gdb_byte *addr, m_register_status[regnum] = REG_VALID; } -/* Supply register REGNUM with zeroed value to REGCACHE. This is not the same - as calling raw_supply with NULL (which will set the state to - unavailable). */ +/* See regcache.h. */ void -detached_regcache::raw_supply_zeroed (int regnum) +reg_buffer::raw_supply_zeroed (int regnum) { void *regbuf; size_t size; @@ -948,8 +946,10 @@ detached_regcache::raw_supply_zeroed (int regnum) m_register_status[regnum] = REG_VALID; } +/* See common/common-regcache.h. */ + void -regcache::raw_collect (int regnum, void *buf) const +reg_buffer::raw_collect (int regnum, void *buf) const { const void *regbuf; size_t size; @@ -962,19 +962,11 @@ regcache::raw_collect (int regnum, void *buf) const memcpy (buf, regbuf, size); } -/* Transfer a single or all registers belonging to a certain register - set to or from a buffer. This is the main worker function for - regcache_supply_regset and regcache_collect_regset. */ - -/* Collect register REGNUM from REGCACHE. Store collected value as an integer - at address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED. - If ADDR_LEN is greater than the register size, then the integer will be sign - or zero extended. If ADDR_LEN is smaller than the register size, then the - most significant bytes of the integer will be truncated. */ +/* See regcache.h. */ void -regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len, - bool is_signed) const +reg_buffer::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len, + bool is_signed) const { enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch); const gdb_byte *regbuf; @@ -989,6 +981,10 @@ regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len, byte_order); } +/* Transfer a single or all registers belonging to a certain register + set to or from a buffer. This is the main worker function for + regcache_supply_regset and regcache_collect_regset. */ + void regcache::transfer_regset (const struct regset *regset, struct regcache *out_regcache,