From patchwork Wed Jun 14 16:11:50 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 21007 Received: (qmail 55329 invoked by alias); 14 Jun 2017 16:12:03 -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 55123 invoked by uid 89); 14 Jun 2017 16:11:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 14 Jun 2017 16:11:48 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4C4B2C0587CF; Wed, 14 Jun 2017 16:11:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4C4B2C0587CF Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 4C4B2C0587CF Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 338BB8E50D; Wed, 14 Jun 2017 16:11:51 +0000 (UTC) Subject: Re: [RFC] LONGEST and ULONGEST function template instantiation To: Yao Qi , Alan Hayward References: <1497353362-14441-1-git-send-email-yao.qi@linaro.org> From: Pedro Alves Cc: gdb-patches@sourceware.org Message-ID: <3038a7b4-d090-7614-1eb0-54c427f95169@redhat.com> Date: Wed, 14 Jun 2017 17:11:50 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1497353362-14441-1-git-send-email-yao.qi@linaro.org> On 06/13/2017 12:29 PM, Yao Qi wrote: > This patch converts functions extract_{unsigned,signed}_integer > to a function template extract_integer, which has two instantiations. It > also does the similar changes to store__{unsigned,signed}_integer, > regcache::raw_read_{unsigned,signed}, regcache::raw_write_{unsigned,signed}, > regcache::cooked_read_{unsigned,signed}, > regcache::cooked_write_{unsigned,signed}. It is an RFC, because I > am not sure people like it or hate it :) I like it, but I'd like it more with a few tweaks to the implementation. :-) See patch below. > > This patch was posted here > https://sourceware.org/ml/gdb-patches/2017-05/msg00492.html but the > problem was fixed in a different way. However, I think the patch is still > useful to shorten the code. > > The patch increase the binary code size (measured on x86_64-linux), from > > $ size ./gdb > text data bss dec hex filename > 23856767 2395936 364616 26617319 19625e7 ./gdb > > to > > $ size ./gdb > text data bss dec hex filename > 23858667 2395936 364616 26619219 1962d53 ./gdb The version below has no impact on code size: $ size gdb.before gdb.after text data bss dec hex filename 7535236 125008 181184 7841428 77a694 gdb.before 7535236 125008 181184 7841428 77a694 gdb.after Since the templates will ever only be instantiated with T=LONGEST and T=ULONGEST, then there's no actual need to move the templates' definitions to the header file. Even if we wanted to move the functions to the header so that their bodies can be inlined for performance (no idea whether it really helps anything), I'd still think this is a useful intermediate step, since it avoids both changing and moving the function bodies at the same time. > + > +#define extract_unsigned_integer extract_integer > + > +#define extract_signed_integer extract_integer These can be static inline functions, instead of macros. On 06/14/2017 02:54 PM, Alan Hayward wrote: > My only objection to this would be that “enable_if” is fairly confusing. > However, having said that, I think the end result is quite nice, and > calling the functions is very simple, so I withdraw my objection. > > It would be nice if the header files could use “LongType” instead of > the longer “enable_if” block, however I remember when I tried this > myself and couldn’t find a way around it. > In C++11 we have default template params, so we can use those for the SFINAE magic instead of having to use a function's parameter of the function's return type. Also, I think that using the "gdb::Requires" helper makes the parameter's intention clearer. From bce7ebe33950b5a2cd5343d9a678ad23f02ec625 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 14 Jun 2017 16:24:07 +0100 Subject: [PATCH] extract/store integer templates --- gdb/defs.h | 47 +++++++++++++++++++----- gdb/findvar.c | 105 ++++++++++++++++++------------------------------------ gdb/regcache.c | 108 +++++++++++++------------------------------------------- gdb/regcache.h | 20 +++++------ gdb/sh64-tdep.c | 12 +++---- 5 files changed, 113 insertions(+), 179 deletions(-) diff --git a/gdb/defs.h b/gdb/defs.h index a1a97bb..55d16d1 100644 --- a/gdb/defs.h +++ b/gdb/defs.h @@ -82,6 +82,11 @@ enum compile_i_scope_types COMPILE_I_PRINT_VALUE_SCOPE, }; + +template +using RequireLongest = gdb::Requires, + std::is_same>>; + /* Just in case they're not defined in stdio.h. */ #ifndef SEEK_SET @@ -637,11 +642,22 @@ enum { MAX_REGISTER_SIZE = 64 }; /* In findvar.c. */ -extern LONGEST extract_signed_integer (const gdb_byte *, int, - enum bfd_endian); +template> +T extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order); -extern ULONGEST extract_unsigned_integer (const gdb_byte *, int, - enum bfd_endian); +static inline LONGEST +extract_signed_integer (const gdb_byte *addr, int len, + enum bfd_endian byte_order) +{ + return extract_integer (addr, len, byte_order); +} + +static inline ULONGEST +extract_unsigned_integer (const gdb_byte *addr, int len, + enum bfd_endian byte_order) +{ + return extract_integer (addr, len, byte_order); +} extern int extract_long_unsigned_integer (const gdb_byte *, int, enum bfd_endian, LONGEST *); @@ -649,11 +665,26 @@ extern int extract_long_unsigned_integer (const gdb_byte *, int, extern CORE_ADDR extract_typed_address (const gdb_byte *buf, struct type *type); -extern void store_signed_integer (gdb_byte *, int, - enum bfd_endian, LONGEST); +/* All 'store' functions accept a host-format integer and store a + target-format integer at ADDR which is LEN bytes long. */ -extern void store_unsigned_integer (gdb_byte *, int, - enum bfd_endian, ULONGEST); +template> +extern void store_integer (gdb_byte *addr, int len, enum bfd_endian byte_order, + T val); + +static inline void +store_signed_integer (gdb_byte *addr, int len, + enum bfd_endian byte_order, LONGEST val) +{ + return store_integer (addr, len, byte_order, val); +} + +static inline void +store_unsigned_integer (gdb_byte *addr, int len, + enum bfd_endian byte_order, ULONGEST val) +{ + return store_integer (addr, len, byte_order, val); +} extern void store_typed_address (gdb_byte *buf, struct type *type, CORE_ADDR addr); diff --git a/gdb/findvar.c b/gdb/findvar.c index 6c18e25..beb127e 100644 --- a/gdb/findvar.c +++ b/gdb/findvar.c @@ -47,70 +47,54 @@ you lose #endif -LONGEST -extract_signed_integer (const gdb_byte *addr, int len, - enum bfd_endian byte_order) +template +T +extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order) { - LONGEST retval; + T retval = 0; const unsigned char *p; const unsigned char *startaddr = addr; const unsigned char *endaddr = startaddr + len; - if (len > (int) sizeof (LONGEST)) + if (len > (int) sizeof (T)) error (_("\ That operation is not available on integers of more than %d bytes."), - (int) sizeof (LONGEST)); + (int) sizeof (T)); /* Start at the most significant end of the integer, and work towards the least significant. */ if (byte_order == BFD_ENDIAN_BIG) { p = startaddr; - /* Do the sign extension once at the start. */ - retval = ((LONGEST) * p ^ 0x80) - 0x80; - for (++p; p < endaddr; ++p) + if (std::is_signed::value) + { + /* Do the sign extension once at the start. */ + retval = ((LONGEST) * p ^ 0x80) - 0x80; + ++p; + } + for (; p < endaddr; ++p) retval = (retval << 8) | *p; } else { p = endaddr - 1; - /* Do the sign extension once at the start. */ - retval = ((LONGEST) * p ^ 0x80) - 0x80; - for (--p; p >= startaddr; --p) + if (std::is_signed::value) + { + /* Do the sign extension once at the start. */ + retval = ((LONGEST) * p ^ 0x80) - 0x80; + --p; + } + for (; p >= startaddr; --p) retval = (retval << 8) | *p; } return retval; } -ULONGEST -extract_unsigned_integer (const gdb_byte *addr, int len, - enum bfd_endian byte_order) -{ - ULONGEST retval; - const unsigned char *p; - const unsigned char *startaddr = addr; - const unsigned char *endaddr = startaddr + len; - - if (len > (int) sizeof (ULONGEST)) - error (_("\ -That operation is not available on integers of more than %d bytes."), - (int) sizeof (ULONGEST)); - - /* Start at the most significant end of the integer, and work towards - the least significant. */ - retval = 0; - if (byte_order == BFD_ENDIAN_BIG) - { - for (p = startaddr; p < endaddr; ++p) - retval = (retval << 8) | *p; - } - else - { - for (p = endaddr - 1; p >= startaddr; --p) - retval = (retval << 8) | *p; - } - return retval; -} +/* Explicit instantiations. */ +template LONGEST extract_integer (const gdb_byte *addr, int len, + enum bfd_endian byte_order); +template ULONGEST extract_integer (const gdb_byte *addr, int len, + enum bfd_endian byte_order); /* Sometimes a long long unsigned integer can be extracted as a LONGEST value. This is done so that we can print these values @@ -180,10 +164,10 @@ extract_typed_address (const gdb_byte *buf, struct type *type) /* All 'store' functions accept a host-format integer and store a target-format integer at ADDR which is LEN bytes long. */ - +template void -store_signed_integer (gdb_byte *addr, int len, - enum bfd_endian byte_order, LONGEST val) +store_integer (gdb_byte *addr, int len, enum bfd_endian byte_order, + T val) { gdb_byte *p; gdb_byte *startaddr = addr; @@ -209,33 +193,14 @@ store_signed_integer (gdb_byte *addr, int len, } } -void -store_unsigned_integer (gdb_byte *addr, int len, - enum bfd_endian byte_order, ULONGEST val) -{ - unsigned char *p; - unsigned char *startaddr = (unsigned char *) addr; - unsigned char *endaddr = startaddr + len; +/* Explicit instantiations. */ +template void store_integer (gdb_byte *addr, int len, + enum bfd_endian byte_order, + LONGEST val); - /* Start at the least significant end of the integer, and work towards - the most significant. */ - if (byte_order == BFD_ENDIAN_BIG) - { - for (p = endaddr - 1; p >= startaddr; --p) - { - *p = val & 0xff; - val >>= 8; - } - } - else - { - for (p = startaddr; p < endaddr; ++p) - { - *p = val & 0xff; - val >>= 8; - } - } -} +template void store_integer (gdb_byte *addr, int len, + enum bfd_endian byte_order, + ULONGEST val); /* Store the address ADDR as a pointer of type TYPE at BUF, in target form. */ diff --git a/gdb/regcache.c b/gdb/regcache.c index 46f4641..608717f 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -662,11 +662,12 @@ enum register_status regcache_raw_read_signed (struct regcache *regcache, int regnum, LONGEST *val) { gdb_assert (regcache != NULL); - return regcache->raw_read_signed (regnum, val); + return regcache->raw_read (regnum, val); } +template enum register_status -regcache::raw_read_signed (int regnum, LONGEST *val) +regcache::raw_read (int regnum, T *val) { gdb_byte *buf; enum register_status status; @@ -675,9 +676,9 @@ regcache::raw_read_signed (int regnum, LONGEST *val) buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]); status = raw_read (regnum, buf); if (status == REG_VALID) - *val = extract_signed_integer - (buf, m_descr->sizeof_register[regnum], - gdbarch_byte_order (m_descr->gdbarch)); + *val = extract_integer (buf, + m_descr->sizeof_register[regnum], + gdbarch_byte_order (m_descr->gdbarch)); else *val = 0; return status; @@ -688,44 +689,26 @@ regcache_raw_read_unsigned (struct regcache *regcache, int regnum, ULONGEST *val) { gdb_assert (regcache != NULL); - return regcache->raw_read_unsigned (regnum, val); -} - - -enum register_status -regcache::raw_read_unsigned (int regnum, ULONGEST *val) -{ - gdb_byte *buf; - enum register_status status; - - gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers); - buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]); - status = raw_read (regnum, buf); - if (status == REG_VALID) - *val = extract_unsigned_integer - (buf, m_descr->sizeof_register[regnum], - gdbarch_byte_order (m_descr->gdbarch)); - else - *val = 0; - return status; + return regcache->raw_read (regnum, val); } void regcache_raw_write_signed (struct regcache *regcache, int regnum, LONGEST val) { gdb_assert (regcache != NULL); - regcache->raw_write_signed (regnum, val); + regcache->raw_write (regnum, val); } +template void -regcache::raw_write_signed (int regnum, LONGEST val) +regcache::raw_write (int regnum, T val) { gdb_byte *buf; gdb_assert (regnum >=0 && regnum < m_descr->nr_raw_registers); buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]); - store_signed_integer (buf, m_descr->sizeof_register[regnum], - gdbarch_byte_order (m_descr->gdbarch), val); + store_integer (buf, m_descr->sizeof_register[regnum], + gdbarch_byte_order (m_descr->gdbarch), val); raw_write (regnum, buf); } @@ -734,19 +717,7 @@ regcache_raw_write_unsigned (struct regcache *regcache, int regnum, ULONGEST val) { gdb_assert (regcache != NULL); - regcache->raw_write_unsigned (regnum, val); -} - -void -regcache::raw_write_unsigned (int regnum, ULONGEST val) -{ - gdb_byte *buf; - - gdb_assert (regnum >=0 && regnum < m_descr->nr_raw_registers); - buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]); - store_unsigned_integer (buf, m_descr->sizeof_register[regnum], - gdbarch_byte_order (m_descr->gdbarch), val); - raw_write (regnum, buf); + regcache->raw_write (regnum, val); } LONGEST @@ -857,11 +828,12 @@ regcache_cooked_read_signed (struct regcache *regcache, int regnum, LONGEST *val) { gdb_assert (regcache != NULL); - return regcache->cooked_read_signed (regnum, val); + return regcache->cooked_read (regnum, val); } +template enum register_status -regcache::cooked_read_signed (int regnum, LONGEST *val) +regcache::cooked_read (int regnum, T *val) { enum register_status status; gdb_byte *buf; @@ -870,9 +842,8 @@ regcache::cooked_read_signed (int regnum, LONGEST *val) buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]); status = cooked_read (regnum, buf); if (status == REG_VALID) - *val = extract_signed_integer - (buf, m_descr->sizeof_register[regnum], - gdbarch_byte_order (m_descr->gdbarch)); + *val = extract_integer (buf, m_descr->sizeof_register[regnum], + gdbarch_byte_order (m_descr->gdbarch)); else *val = 0; return status; @@ -883,25 +854,7 @@ regcache_cooked_read_unsigned (struct regcache *regcache, int regnum, ULONGEST *val) { gdb_assert (regcache != NULL); - return regcache->cooked_read_unsigned (regnum, val); -} - -enum register_status -regcache::cooked_read_unsigned (int regnum, ULONGEST *val) -{ - enum register_status status; - gdb_byte *buf; - - gdb_assert (regnum >= 0 && regnum < m_descr->nr_cooked_registers); - buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]); - status = cooked_read (regnum, buf); - if (status == REG_VALID) - *val = extract_unsigned_integer - (buf, m_descr->sizeof_register[regnum], - gdbarch_byte_order (m_descr->gdbarch)); - else - *val = 0; - return status; + return regcache->cooked_read (regnum, val); } void @@ -909,18 +862,19 @@ regcache_cooked_write_signed (struct regcache *regcache, int regnum, LONGEST val) { gdb_assert (regcache != NULL); - regcache->cooked_write_signed (regnum, val); + regcache->cooked_write (regnum, val); } +template void -regcache::cooked_write_signed (int regnum, LONGEST val) +regcache::cooked_write (int regnum, T val) { gdb_byte *buf; gdb_assert (regnum >=0 && regnum < m_descr->nr_cooked_registers); buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]); - store_signed_integer (buf, m_descr->sizeof_register[regnum], - gdbarch_byte_order (m_descr->gdbarch), val); + store_integer (buf, m_descr->sizeof_register[regnum], + gdbarch_byte_order (m_descr->gdbarch), val); cooked_write (regnum, buf); } @@ -929,19 +883,7 @@ regcache_cooked_write_unsigned (struct regcache *regcache, int regnum, ULONGEST val) { gdb_assert (regcache != NULL); - regcache->cooked_write_unsigned (regnum, val); -} - -void -regcache::cooked_write_unsigned (int regnum, ULONGEST val) -{ - gdb_byte *buf; - - gdb_assert (regnum >=0 && regnum < m_descr->nr_cooked_registers); - buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]); - store_unsigned_integer (buf, m_descr->sizeof_register[regnum], - gdbarch_byte_order (m_descr->gdbarch), val); - cooked_write (regnum, buf); + regcache->cooked_write (regnum, val); } /* See regcache.h. */ diff --git a/gdb/regcache.h b/gdb/regcache.h index 4cf27a0..3f3f823 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -282,23 +282,19 @@ public: #endif void raw_write (int regnum, const gdb_byte *buf); - enum register_status raw_read_signed (int regnum, LONGEST *val); + template> + enum register_status raw_read (int regnum, T *val); - void raw_write_signed (int regnum, LONGEST val); - - enum register_status raw_read_unsigned (int regnum, ULONGEST *val); - - void raw_write_unsigned (int regnum, ULONGEST val); + template> + void raw_write (int regnum, T val); struct value *cooked_read_value (int regnum); - enum register_status cooked_read_signed (int regnum, LONGEST *val); - - void cooked_write_signed (int regnum, LONGEST val); - - enum register_status cooked_read_unsigned (int regnum, ULONGEST *val); + template> + enum register_status cooked_read (int regnum, T *val); - void cooked_write_unsigned (int regnum, ULONGEST val); + template> + void cooked_write (int regnum, T val); void raw_update (int regnum); diff --git a/gdb/sh64-tdep.c b/gdb/sh64-tdep.c index 4eb8947..3273197 100644 --- a/gdb/sh64-tdep.c +++ b/gdb/sh64-tdep.c @@ -1665,11 +1665,11 @@ sh64_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache, */ /* *INDENT-ON* */ /* Get FPSCR as an int. */ - status = regcache->raw_read_unsigned (fpscr_base_regnum, &fpscr_value); + status = regcache->raw_read (fpscr_base_regnum, &fpscr_value); if (status != REG_VALID) return status; /* Get SR as an int. */ - status = regcache->raw_read_unsigned (sr_base_regnum, &sr_value); + status = regcache->raw_read (sr_base_regnum, &sr_value); if (status != REG_VALID) return status; /* Build the new value. */ @@ -1847,15 +1847,15 @@ sh64_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache, fpscr_value = fpscr_c_value & fpscr_mask; sr_value = (fpscr_value & sr_mask) >> 6; - regcache->raw_read_unsigned (fpscr_base_regnum, &old_fpscr_value); + regcache->raw_read (fpscr_base_regnum, &old_fpscr_value); old_fpscr_value &= 0xfffc0002; fpscr_value |= old_fpscr_value; - regcache->raw_write_unsigned (fpscr_base_regnum, fpscr_value); + regcache->raw_write (fpscr_base_regnum, fpscr_value); - regcache->raw_read_unsigned (sr_base_regnum, &old_sr_value); + regcache->raw_read (sr_base_regnum, &old_sr_value); old_sr_value &= 0xffff8fff; sr_value |= old_sr_value; - regcache->raw_write_unsigned (sr_base_regnum, sr_value); + regcache->raw_write (sr_base_regnum, sr_value); } else if (reg_nr == FPUL_C_REGNUM)