From patchwork Fri Jun 8 15:16:31 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Alan Hayward X-Patchwork-Id: 27718 Received: (qmail 45226 invoked by alias); 8 Jun 2018 15:16:40 -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 44935 invoked by uid 89); 8 Jun 2018 15:16:40 -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= X-HELO: EUR01-DB5-obe.outbound.protection.outlook.com Received: from mail-db5eur01on0077.outbound.protection.outlook.com (HELO EUR01-DB5-obe.outbound.protection.outlook.com) (104.47.2.77) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 08 Jun 2018 15:16:37 +0000 Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com (10.172.226.148) by DB6PR0802MB2424.eurprd08.prod.outlook.com (10.172.251.8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.841.15; Fri, 8 Jun 2018 15:16:31 +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 15:16:31 +0000 From: Alan Hayward To: Simon Marchi CC: "gdb-patches@sourceware.org" , nd Subject: Re: [PATCH v2 04/10] Add regcache raw_compare method Date: Fri, 8 Jun 2018 15:16:31 +0000 Message-ID: References: <20180606151629.36602-1-alan.hayward@arm.com> <20180606151629.36602-5-alan.hayward@arm.com> In-Reply-To: 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: DB6PR0802MB2424: nodisclaimer: True x-exchange-antispam-report-test: UriScan:(37575265505322)(180628864354917); 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: <6090291ADC1EFA4EA6FA4DF1EE93F618@eurprd08.prod.outlook.com> MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: 90307b1b-c19f-4c73-81c5-08d5cd52d0db X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: 90307b1b-c19f-4c73-81c5-08d5cd52d0db X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Jun 2018 15:16:31.7081 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0802MB2424 X-IsSubscribed: yes > On 7 Jun 2018, at 21:55, Simon Marchi wrote: > > On 2018-06-06 11:16 AM, Alan Hayward wrote: >> raw_compare returns the same as a memcmp (0 for success, >> the diff otherwise). Kept with tristate return as this >> feels potentally more useful than a simple true/false return. >> (Happy to change if not). > > I would err on the bool side, but I don't really mind, the important is > that it's documented properly. We still use many ints as bool in GDB, so > someone seeing an int return value could easily think it's 0 -> not equal, > 1 -> equal. Ok, updated. > > Speaking of doc, I would suggest (as in the previous patch) to centralize > the doc in the class/struct at the root, so we don't duplicate it. Done. > >> 2018-06-06 Alan Hayward >> >> gdb/ >> * common/common-regcache.h (raw_compare): New function. >> * regcache.c (regcache::raw_compare): Likewise. >> * regcache.h (regcache::raw_compare): New declaration. >> >> gdbserver/ >> * regcache.c (regcache::raw_compare): New function. >> * regcache.h (regcache::raw_compare): New declaration. >> --- >> gdb/common/common-regcache.h | 1 + >> gdb/gdbserver/regcache.c | 10 ++++++++++ >> gdb/gdbserver/regcache.h | 5 +++++ >> gdb/regcache.c | 15 +++++++++++++++ >> gdb/regcache.h | 11 +++++++++++ >> 5 files changed, 42 insertions(+) >> >> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h >> index 487da0a7fb..e91439bec5 100644 >> --- a/gdb/common/common-regcache.h >> +++ b/gdb/common/common-regcache.h >> @@ -67,6 +67,7 @@ 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 int raw_compare (int regnum, const void *buf, int offset) const = 0; >> virtual register_status get_register_status (int regnum) const = 0; >> }; >> >> diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c >> index 857721ee3d..9648428349 100644 >> --- a/gdb/gdbserver/regcache.c >> +++ b/gdb/gdbserver/regcache.c >> @@ -501,3 +501,13 @@ regcache::get_register_status (int regnum) const >> return REG_VALID; >> #endif >> } >> + >> +/* See gdbserver/regcache.h. */ >> + >> +int >> +regcache::raw_compare (int regnum, const void *buf, int offset) const >> +{ >> + gdb_assert (register_size (tdesc, regnum) > offset); > > Should we check ">= offset"? I think it could be useful for some edge cases > where we could avoid an "if (offset < size)" in the caller, if we know offset > could be equal to size. memcmp would return 0 (equal), which is fine. > Given that memcmp can cope with this, then ok. (I need to make sure I update the patch that uses this function too!) > Maybe assign "register_size (tdesc, regnum)" to a variable (e.g. reg_size) > and use it in both places? Done. > >> + return memcmp (buf, register_data (this, regnum, 1) + offset, >> + register_size (tdesc, regnum) - offset); >> +} >> diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h >> index 1842f1d9cf..b26f39a8ad 100644 >> --- a/gdb/gdbserver/regcache.h >> +++ b/gdb/gdbserver/regcache.h >> @@ -50,6 +50,11 @@ struct regcache : public reg_buffer_common >> >> void raw_collect (int regnum, void *buf) const override; >> >> + /* Compare the contents of the register stored in the regcache (ignoring the >> + first OFFSET bytes) to the contents of BUF (without any offset). Returns >> + the same result as memcmp. */ >> + int raw_compare (int regnum, const void *buf, int offset) const override; >> + >> enum register_status get_register_status (int regnum) const override; >> }; >> >> diff --git a/gdb/regcache.c b/gdb/regcache.c >> index a914b548ca..383e355e9f 100644 >> --- a/gdb/regcache.c >> +++ b/gdb/regcache.c >> @@ -1082,6 +1082,21 @@ regcache::collect_regset (const struct regset *regset, >> transfer_regset (regset, NULL, regnum, NULL, buf, size); >> } >> >> +/* See regcache.h. */ >> + >> +int >> +regcache::raw_compare (int regnum, const void *buf, int offset) const >> +{ >> + const char *regbuf; >> + size_t size; >> + >> + gdb_assert (buf != NULL); >> + assert_regnum (regnum); > > Should we assert that offset is < or <= size here too? > Not sure why I didn’t add this. Patch update with changes above. Are you ok with this version? diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h index 29d9a81182ad1a5797b080e136b682fe59ecef37..fe3ece7ac52d60d07d718ad617f30be2f7133b5f 100644 --- a/gdb/common/common-regcache.h +++ b/gdb/common/common-regcache.h @@ -75,6 +75,11 @@ struct reg_buffer_common /* Collect register REGNUM from REGCACHE and store its contents in BUF. */ virtual void raw_collect (int regnum, void *buf) const = 0; + + /* Compare the contents of the register stored in the regcache (ignoring the + first OFFSET bytes) to the contents of BUF (without any offset). Returns + True if the same. */ + virtual bool raw_compare (int regnum, const void *buf, int offset) const = 0; }; #endif /* COMMON_REGCACHE_H */ diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h index 352c1df3f9eeacd622a3d5d668ff4ea98aea9c6f..b4c4c20ebd368f56f14af758301ff7d55fd16dae 100644 --- a/gdb/gdbserver/regcache.h +++ b/gdb/gdbserver/regcache.h @@ -54,6 +54,9 @@ struct regcache : public reg_buffer_common /* See common/common-regcache.h. */ void raw_collect (int regnum, void *buf) const override; + + /* See common/common-regcache.h. */ + bool raw_compare (int regnum, const void *buf, int offset) const override; }; struct regcache *init_register_cache (struct regcache *regcache, diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c index 83837525a18c1dc96b6e9d24397064e247ced459..735ce7bccfc7a058f8604620928f3894d4655887 100644 --- a/gdb/gdbserver/regcache.c +++ b/gdb/gdbserver/regcache.c @@ -502,3 +502,17 @@ regcache::get_register_status (int regnum) const return REG_VALID; #endif } + +/* See common/common-regcache.h. */ + +bool +regcache::raw_compare (int regnum, const void *buf, int offset) const +{ + gdb_assert (buf != NULL); + + const unsigned char *regbuf = register_data (this, regnum, 1); + int size = register_size (tdesc, regnum); + gdb_assert (size >= offset); + + return (memcmp (buf, regbuf + offset, size - offset) == 0); +} diff --git a/gdb/regcache.h b/gdb/regcache.h index 4e5659b25bc530a4ced32520825136534be2e642..511864858d24a7fa53047fdc247846a551b121af 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -191,6 +191,10 @@ public: xfree (m_registers); xfree (m_register_status); } + + /* See common/common-regcache.h. */ + bool raw_compare (int regnum, const void *buf, int offset) const override; + protected: /* Assert on the range of REGNUM. */ void assert_regnum (int regnum) const; diff --git a/gdb/regcache.c b/gdb/regcache.c index 032fef0d34ac635c96dd6c39426f5c6d04f28095..fd0fd7318c8cca0083c897d2bd6ba1361dd627c0 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -1078,6 +1078,20 @@ regcache::collect_regset (const struct regset *regset, transfer_regset (regset, NULL, regnum, NULL, buf, size); } +/* See common/common-regcache.h. */ + +bool +reg_buffer::raw_compare (int regnum, const void *buf, int offset) const +{ + gdb_assert (buf != NULL); + assert_regnum (regnum); + + const char *regbuf = (const char *) register_buffer (regnum); + size_t size = m_descr->sizeof_register[regnum]; + gdb_assert (size >= offset); + + return (memcmp (buf, regbuf + offset, size - offset) == 0); +} /* Special handling for register PC. */