From patchwork Tue Jun 19 11:27:43 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Alan Hayward X-Patchwork-Id: 27922 Received: (qmail 41877 invoked by alias); 19 Jun 2018 11:28:21 -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 37057 invoked by uid 89); 19 Jun 2018 11:27:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.2 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, MIME_BASE64_BLANKS, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS, UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy=filling, slot, relatively, reality X-HELO: EUR04-HE1-obe.outbound.protection.outlook.com Received: from mail-eopbgr70059.outbound.protection.outlook.com (HELO EUR04-HE1-obe.outbound.protection.outlook.com) (40.107.7.59) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 19 Jun 2018 11:27:48 +0000 Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com (10.172.226.148) by DB6PR0802MB2390.eurprd08.prod.outlook.com (10.172.250.143) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.863.19; Tue, 19 Jun 2018 11:27:43 +0000 Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com ([fe80::a152:f8f6:6608:7624]) by DB6PR0802MB2133.eurprd08.prod.outlook.com ([fe80::a152:f8f6:6608:7624%5]) with mapi id 15.20.0863.016; Tue, 19 Jun 2018 11:27:43 +0000 From: Alan Hayward To: Simon Marchi CC: GDB Patches , nd Subject: Re: [PATCH] Support large registers in regcache transfer_regset Date: Tue, 19 Jun 2018 11:27:43 +0000 Message-ID: <85CCB6E0-63F0-4298-B328-D43D3207A91E@arm.com> References: <20180612080356.33157-1-alan.hayward@arm.com> <6bba6aa4-c350-e5fb-3913-823eccae60ef@simark.ca> In-Reply-To: <6bba6aa4-c350-e5fb-3913-823eccae60ef@simark.ca> 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-correlation-id: 08687ac2-8ae6-4120-81f9-08d5d5d7ac92 x-ms-office365-filtering-ht: Tenant x-ms-traffictypediagnostic: DB6PR0802MB2390: nodisclaimer: True x-exchange-antispam-report-test: UriScan:(180628864354917); x-ms-exchange-senderadcheck: 1 x-forefront-prvs: 07083FF734 received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-ID: <4197743B1301204790953A6AE9C41F85@eurprd08.prod.outlook.com> MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: 08687ac2-8ae6-4120-81f9-08d5d5d7ac92 X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Jun 2018 11:27:43.1823 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0802MB2390 X-IsSubscribed: yes > On 17 Jun 2018, at 03:52, Simon Marchi wrote: > > On 2018-06-12 04:03 AM, Alan Hayward wrote: >> Regcache transfer_regset is used to copy registers to/from a regset. >> If the size of a regcache register is greater than it's slot in the regset >> then the writes will overflow into the next slot(s), causing general >> overflow corruption with the latter slots. > > Hi Alan, > > Can you please remind me when this happens? When reading cores, we don't > write to regsets, if I understand correctly, so it would not be a problem > tben. Type “generate-core-file” on the command line. Not sure if there are other ways of triggering it. > >> Add raw_collect_part and raw_supply_part, which are simplified versions of >> raw_read_part and raw_write_part. Use these in accordingly in transfer_regset. >> >> This is required to support the .reg2 core section on Aarch64 SVE, which >> only has enough space to store the first 128bits of each vector register. >> >> This patch replaces [PATCH v2 10/10] Remove reg2 section from Aarch64 SVE cores >> >> Tested by generating cores on aarch64 and aarch64 sve, both via gdb and the >> kernel, then ensuring the cores load back on the both systems. >> Checked cores on x86 still look ok. >> Ran make check on x86 and aarch64. >> >> 2018-06-12 Alan Hayward >> >> * regcache.c (reg_buffer::raw_collect_part): New function. >> (reg_buffer::raw_supply_part): Likewise. >> (regcache::transfer_regset): Call new functions. >> * regcache.h (reg_buffer::raw_collect_part): New declaration. >> (reg_buffer::raw_supply_part): Likewise. >> --- >> gdb/regcache.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++------ >> gdb/regcache.h | 8 +++++ >> 2 files changed, 94 insertions(+), 10 deletions(-) >> >> diff --git a/gdb/regcache.c b/gdb/regcache.c >> index 750ea2ad30..babc0e1d43 100644 >> --- a/gdb/regcache.c >> +++ b/gdb/regcache.c >> @@ -812,6 +812,27 @@ readable_regcache::read_part (int regnum, int offset, int len, void *in, >> return REG_VALID; >> } >> >> +/* See regcache.h. */ >> + >> +void >> +reg_buffer::raw_collect_part (int regnum, int offset, int len, void *in) const > > I know this comes from raw_read_part/raw_write_part, but I find the naming of the > in/out parameters confusing. I think it would make more sense if this one was > called "out" and the one in raw_supply_part "in”. > Agreed. I’ll also update the original code too. > Also, can they be "gdb_byte *" instead of "void *”? Done. > >> +{ >> + struct gdbarch *gdbarch = arch (); >> + gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum)); >> + >> + gdb_assert (in != NULL); >> + gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]); >> + gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]); > > The "&& offset <= m_descr->sizeof_register[regnum]" is redundant, given the > following line. Other than mimicking raw_read_part, is there a reason why > these are signed integers? Having them unsigned would avoid having to assert > they are >= 0. Looking at regcache, int is used for regnum throughout. I’d rather not have a mismatch, and wouldn’t want to update everything else either (at least not in this patch). In addition, if this code is going to now call down to raw_collect/raw_supply, they should match. I’ve updated the asserts in both here and the original code too. > >> + /* Something to do? */ >> + if (offset + len == 0) >> + return; >> + /* Read (when needed) ... */ >> + raw_collect (regnum, reg); >> + >> + /* ... modify ... */ >> + memcpy (in, reg + offset, len); >> +} >> + >> enum register_status >> regcache::write_part (int regnum, int offset, int len, >> const void *out, bool is_raw) >> @@ -849,6 +870,33 @@ regcache::write_part (int regnum, int offset, int len, >> return REG_VALID; >> } >> >> +/* See regcache.h. */ >> + >> +void >> +reg_buffer::raw_supply_part (int regnum, int offset, int len, >> + const gdb_byte *out) >> +{ >> + struct gdbarch *gdbarch = arch (); >> + gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum)); >> + >> + gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]); >> + gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]); > > "&& offset <= m_descr->sizeof_register[regnum]" is redundant here too. Done. > >> + /* Something to do? */ >> + if (offset + len == 0) >> + return; >> + /* Read (when needed) ... */ >> + if (offset > 0 >> + || offset + len < m_descr->sizeof_register[regnum]) >> + raw_collect (regnum, reg); >> + >> + if (out == nullptr) >> + memset (reg + offset, 0, len); >> + else >> + memcpy (reg + offset, out, len); >> + /* ... write (when needed). */ >> + raw_supply (regnum, reg); >> +} >> + >> enum register_status >> readable_regcache::raw_read_part (int regnum, int offset, int len, gdb_byte *buf) >> { >> @@ -1016,12 +1064,26 @@ regcache::transfer_regset (const struct regset *regset, >> if (offs + slot_size > size) >> break; >> >> - if (out_buf) >> - raw_collect (regno, (gdb_byte *) out_buf + offs); >> + gdb_byte *out_loc = (gdb_byte *) out_buf + offs; >> + const gdb_byte *in_loc = in_buf ? (const gdb_byte *) in_buf + offs >> + : NULL; >> + >> + if (slot_size < m_descr->sizeof_register[regno]) >> + { >> + /* Register is bigger than the size of the slot. Prevent >> + possible overflow. */ >> + if (out_buf) >> + raw_collect_part (regno, 0, slot_size, out_loc); >> + else >> + out_regcache->raw_supply_part (regno, 0, slot_size, in_loc); >> + } >> else >> - out_regcache->raw_supply (regno, in_buf >> - ? (const gdb_byte *) in_buf + offs >> - : NULL); >> + { >> + if (out_buf) >> + raw_collect (regno, out_loc); >> + else >> + out_regcache->raw_supply (regno, in_loc); >> + } > > I think the code here could stay relatively simple if we always went > through raw_collect_part/raw_supply_part. To avoid the unnecessary copy > that would happen in raw_collect_part/raw_supply_part in the typical case > where the full register is transferred, there could be a shortcut path > that calls raw_collect/raw_supply when collecting/supplying the full > reguster. > Ok, added this too. However, this uncovered an issue. If the slot size is larger than the size of the register then this causes raw_collect_part/raw_supply_part to assert. This is triggered on aarch64 with CPSR, because the register is 32bits but the core file reserves 64bits. In the current code there is a bug here as the existing code doesn’t ensure this extra space in the slot size is cleared when writing out cores. (In reality, probably not much of a problem as we ignore that space when reading the core back in. But it could be an issue if another program was Reading gdb’s core files). To fix this I allowed raw_collect_part/raw_supply_part to handle cases where the length overflows the register, either filling with zeros or truncating accordingly. Also, because the existing transfer_register code would result in register being set to invalid when writing null to the regcache, I also ensured that was kept. I’ve also made a few more changes to write_part/read_part to make them consistent with the changes to the new functions. There is no functionality change to them. Is this ok? 2018-06-19 Alan Hayward * regcache.c (readable_regcache::read_part): Add full read shortcut. (reg_buffer::raw_collect_part): New function. (regcache::write_part): Add full write shortcut. (reg_buffer::raw_supply_part): New function. (regcache::transfer_regset): Use new functions. * regcache.h (reg_buffer::raw_collect_part): New declaration. (reg_buffer::raw_supply_part): Likewise. diff --git a/gdb/regcache.h b/gdb/regcache.h index 41465fb20d0dcdddaf39e47c1fc79f1e8eadc260..297f04b5b44690562602a2b5ce15b9dd1a92b18b 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -163,6 +163,11 @@ public: void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len, bool is_signed) const; + /* Collect register REGNUM from REGCACHE, starting at offset in REGCACHE, + reading only LEN. If this runs off the end of the register, then fill the + additional space with zeros. */ + void raw_collect_part (int regnum, int offset, int len, gdb_byte *out) const; + /* See common/common-regcache.h. */ void raw_supply (int regnum, const void *buf) override; @@ -184,6 +189,11 @@ public: unavailable). */ void raw_supply_zeroed (int regnum); + /* Supply register REGNUM to REGCACHE, starting at offset in REGCACHE, writing + only LEN, without editing the rest of the register. If the length of the + supplied value would overflow the register, then truncate. */ + void raw_supply_part (int regnum, int offset, int len, const gdb_byte *in); + void invalidate (int regnum); virtual ~reg_buffer () = default; @@ -254,8 +264,11 @@ public: struct value *cooked_read_value (int regnum); protected: - enum register_status read_part (int regnum, int offset, int len, void *in, - bool is_raw); + + /* Perform a partial register transfer using a read, modify, write + operation. */ + enum register_status read_part (int regnum, int offset, int len, + gdb_byte *out, bool is_raw); }; /* Buffer of registers, can be read and written. */ @@ -356,8 +369,10 @@ private: int regnum, const void *in_buf, void *out_buf, size_t size) const; + /* Perform a partial register transfer using a read, modify, write + operation. */ enum register_status write_part (int regnum, int offset, int len, - const void *out, bool is_raw); + const gdb_byte *out, bool is_raw); /* The address space of this register cache (for registers where it diff --git a/gdb/regcache.c b/gdb/regcache.c index 750ea2ad30f60b03dd76fc30cb72f87d5a531406..5c672e6928101710904c2e70fc829036214f64b6 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -780,75 +780,138 @@ regcache::cooked_write (int regnum, const gdb_byte *buf) regnum, buf); } -/* Perform a partial register transfer using a read, modify, write - operation. */ +/* See regcache.h. */ enum register_status -readable_regcache::read_part (int regnum, int offset, int len, void *in, +readable_regcache::read_part (int regnum, int offset, int len, gdb_byte *out, bool is_raw) { - struct gdbarch *gdbarch = arch (); - gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum)); + int reg_size = register_size (arch (), regnum); - gdb_assert (in != NULL); - gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]); - gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]); - /* Something to do? */ - if (offset + len == 0) + gdb_assert (out != NULL); + gdb_assert (offset >= 0 && len >= 0 && offset + len <= reg_size); + + if (offset == 0 && len == 0) return REG_VALID; - /* Read (when needed) ... */ + + if (offset == 0 && len == reg_size) + return (is_raw) ? raw_read (regnum, out) : cooked_read (regnum, out); + + /* Read to buffer, then write out. */ enum register_status status; + gdb_byte *reg = (gdb_byte *) alloca (reg_size); - if (is_raw) - status = raw_read (regnum, reg); - else - status = cooked_read (regnum, reg); + status = (is_raw) ? raw_read (regnum, reg) : cooked_read (regnum, reg); if (status != REG_VALID) return status; - /* ... modify ... */ - memcpy (in, reg + offset, len); - + memcpy (out, reg + offset, len); return REG_VALID; } +/* See regcache.h. */ + +void +reg_buffer::raw_collect_part (int regnum, int offset, int len, + gdb_byte *out) const +{ + int reg_size = register_size (arch (), regnum); + + gdb_assert (out != nullptr); + gdb_assert (offset >= 0 && len >= 0 && offset <= reg_size); + + if (offset == 0 && len == 0) + return; + + if (offset == 0 && len == reg_size) + return raw_collect (regnum, out); + + /* Read to buffer, then write out. */ + gdb_byte *reg = (gdb_byte *) alloca (reg_size); + raw_collect (regnum, reg); + + if (offset + len <= reg_size) + memcpy (out, reg + offset, len); + else + { + /* Requested region runs off the end of the register. Clear the + additional space. */ + memcpy (out, reg + offset, reg_size - offset); + memset (out + reg_size, 0, offset + len - reg_size); + } +} + +/* See regcache.h. */ + enum register_status -regcache::write_part (int regnum, int offset, int len, - const void *out, bool is_raw) +regcache::write_part (int regnum, int offset, int len, const gdb_byte *in, + bool is_raw) { - struct gdbarch *gdbarch = arch (); - gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum)); + int reg_size = register_size (arch (), regnum); - gdb_assert (out != NULL); - gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]); - gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]); - /* Something to do? */ - if (offset + len == 0) + gdb_assert (in != NULL); + gdb_assert (offset >= 0 && len >= 0 && offset + len <= reg_size); + + if (offset == 0 && len == 0) return REG_VALID; - /* Read (when needed) ... */ - if (offset > 0 - || offset + len < m_descr->sizeof_register[regnum]) + + if (offset == 0 && len == reg_size) { - enum register_status status; + (is_raw) ? raw_write (regnum, in) : cooked_write (regnum, in); + return REG_VALID; + } - if (is_raw) - status = raw_read (regnum, reg); - else - status = cooked_read (regnum, reg); + gdb_byte *reg = (gdb_byte *) alloca (reg_size); + + /* Read when needed. */ + if (offset > 0 || offset + len < reg_size) + { + enum register_status status; + status = (is_raw) ? raw_read (regnum, reg) : cooked_read (regnum, reg); if (status != REG_VALID) return status; } - memcpy (reg + offset, out, len); - /* ... write (when needed). */ - if (is_raw) - raw_write (regnum, reg); - else - cooked_write (regnum, reg); - + /* Write to buffer, then write out. */ + memcpy (reg + offset, in, len); + is_raw ? raw_write (regnum, reg) : cooked_write (regnum, reg); return REG_VALID; } +/* See regcache.h. */ + +void +reg_buffer::raw_supply_part (int regnum, int offset, int len, + const gdb_byte *in) +{ + int reg_size = register_size (arch (), regnum); + + gdb_assert (in != nullptr); + gdb_assert (offset >= 0 && len >= 0 && offset <= reg_size); + + if (offset == 0 && len == 0) + return; + + if (offset + len > reg_size) + { + /* Truncate length to fit the size of the regcache register. */ + len = reg_size - offset; + } + + if (offset == 0 && len == reg_size) + return raw_supply (regnum, in); + + gdb_byte *reg = (gdb_byte *) alloca (reg_size); + + /* Read when needed. */ + if (offset > 0 || offset + len < reg_size) + raw_collect (regnum, reg); + + /* Write to buffer, then write out. */ + memcpy (reg + offset, in, len); + raw_supply (regnum, reg); +} + enum register_status readable_regcache::raw_read_part (int regnum, int offset, int len, gdb_byte *buf) { @@ -994,6 +1057,7 @@ regcache::transfer_regset (const struct regset *regset, { const struct regcache_map_entry *map; int offs = 0, count; + struct gdbarch *gdbarch = arch (); for (map = (const struct regcache_map_entry *) regset->regmap; (count = map->count) != 0; @@ -1016,12 +1080,18 @@ regcache::transfer_regset (const struct regset *regset, if (offs + slot_size > size) break; + /* Use part versions to prevent possible overflow. */ if (out_buf) - raw_collect (regno, (gdb_byte *) out_buf + offs); + raw_collect_part (regno, 0, slot_size, + (gdb_byte *) out_buf + offs); + else if (in_buf) + out_regcache->raw_supply_part (regno, 0, slot_size, + (const gdb_byte *) in_buf + offs); else - out_regcache->raw_supply (regno, in_buf - ? (const gdb_byte *) in_buf + offs - : NULL); + { + /* Invalidate the register. */ + out_regcache->raw_supply (regno, nullptr); + } } else { @@ -1030,12 +1100,19 @@ regcache::transfer_regset (const struct regset *regset, if (offs + slot_size > size) return; + /* Use part versions to prevent possible overflow. */ if (out_buf) - raw_collect (regnum, (gdb_byte *) out_buf + offs); + raw_collect_part (regnum, 0, slot_size, + (gdb_byte *) out_buf + offs); + else if (in_buf) + out_regcache->raw_supply_part (regnum, 0, slot_size, + (const gdb_byte *) in_buf + offs); else - out_regcache->raw_supply (regnum, in_buf - ? (const gdb_byte *) in_buf + offs - : NULL); + { + /* Invalidate the register. */ + out_regcache->raw_supply (regnum, nullptr); + } + return; } }