From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38017 invoked by alias); 17 Jun 2018 02:52:56 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 38006 invoked by uid 89); 17 Jun 2018 02:52:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS,UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy=relatively, greater, supplying, confusing X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 17 Jun 2018 02:52:54 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 3CB311E02D; Sat, 16 Jun 2018 22:52:52 -0400 (EDT) Subject: Re: [PATCH] Support large registers in regcache transfer_regset To: Alan Hayward , gdb-patches@sourceware.org Cc: nd@arm.com References: <20180612080356.33157-1-alan.hayward@arm.com> From: Simon Marchi Message-ID: <6bba6aa4-c350-e5fb-3913-823eccae60ef@simark.ca> Date: Sun, 17 Jun 2018 02:52:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180612080356.33157-1-alan.hayward@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-06/txt/msg00430.txt.bz2 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. > 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". Also, can they be "gdb_byte *" instead of "void *"? > +{ > + 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. > + /* 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. > + /* 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. Simon