Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

Yao Qi
2017-05-22T12:07:34Z

Alan Hayward <Alan.Hayward@arm.com> writes:

> -extern ULONGEST extract_unsigned_integer (const gdb_byte *, int,
> - enum bfd_endian);
> +inline ULONGEST extract_unsigned_integer (const gdb_byte *addr, int len,
> + enum bfd_endian byte_order)
> +{
> + return extract_integer<LONGEST> (addr, len, byte_order);

s/LONGEST/ULONGEST/

> +}
>

[My C++ knowledge is still poor, so forgive me if I ask something stupid]

Is there any reason you do not put function template (extract_integer and
store_integer) in header? Any benefit of doing this?

> extern int extract_long_unsigned_integer (const gdb_byte *, int,
> enum bfd_endian, LONGEST *);
> @@ -649,11 +664,21 @@ 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);
> +template<typename T> void store_integer (gdb_byte *addr, int len,
> + enum bfd_endian byte_order,
> + LongType<T> val);

We can define store_integer slightly differently, so that we can use
implicit instantiation,

template<typename T>
typename std::enable_if<(std::is_same<T, LONGEST>::value
|| std::is_same<T, ULONGEST>::value),
void>::type
store_integer (gdb_byte *addr, int len, enum bfd_endian byte_order, T val)
{} so that ...

>
> -extern void store_unsigned_integer (gdb_byte *, int,
> - enum bfd_endian, ULONGEST);
> +inline void store_signed_integer (gdb_byte *addr, int len,
> + enum bfd_endian byte_order, LONGEST val)
> +{
> + store_integer<LONGEST> (addr, len, byte_order, val);

... the template argument can be deduced,

store_integer (addr, len, byte_order, val);

should be OK.

> diff --git a/gdb/findvar.c b/gdb/findvar.c
> index ed4d5c1266c9de069981b306bc8229ae5fb02350..3bcc98ce3421cb800fc8222535b16ca94ee043da 100644
> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -46,70 +46,54 @@
> you lose
> #endif
>
> -LONGEST
> -extract_signed_integer (const gdb_byte *addr, int len,
> - enum bfd_endian byte_order)
> +template<typename T>
> +LongType<T>
> +extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order)
> {
> - LONGEST retval;
> + T retval;
> 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<T>::value)
> + {
> + /* Do the sign extension once at the start. */
> + retval = ((LONGEST) * p ^ 0x80) - 0x80;
> + ++p;
> + }

Need to set retval to zero if (!std::is_signed<T>::value), otherwise,
retval might be not initialized.

> diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
> index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..1e391bfd16864d9d45f3b6e0de425aa5543515df 100644
> --- a/gdb/mips-fbsd-tdep.c
> +++ b/gdb/mips-fbsd-tdep.c
> @@ -48,9 +48,9 @@
> #define MIPS_FBSD_NUM_FPREGS 34
>
> /* Supply a single register. If the source register size matches the
> - size the regcache expects, this can use regcache_raw_supply(). If
> + size the regcache expects, this can use regcache->raw_supply (). If
> they are different, this copies the source register into a buffer
> - that can be passed to regcache_raw_supply(). */
> + that can be passed to regcache->raw_supply (). */
>
> static void
> mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
> @@ -63,20 +63,17 @@ mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
> else
> {
> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> - gdb_byte buf[MAX_REGISTER_SIZE];
> LONGEST val;
>
> - val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
> - store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
> - val);
> - regcache_raw_supply (regcache, regnum, buf);
> + val = extract_integer<LONGEST> ((const gdb_byte *) addr, len, byte_order);
> + regcache->raw_supply<LONGEST> (regnum, val);
> }
> }
>
> /* Collect a single register. If the destination register size
> matches the size the regcache expects, this can use
> - regcache_raw_supply(). If they are different, this fetches the
> - register via regcache_raw_supply() into a buffer and then copies it
> + regcache->raw_supply (). If they are different, this fetches the
> + register via regcache->raw_supply () into a buffer and then copies it
> into the final destination. */
>
> static void
> @@ -90,13 +87,8 @@ mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
> else
> {
> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> - gdb_byte buf[MAX_REGISTER_SIZE];
> - LONGEST val;
> -
> - regcache_raw_collect (regcache, regnum, buf);
> - val = extract_signed_integer (buf, register_size (gdbarch, regnum),
> - byte_order);
> - store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
> + LONGEST val = regcache->raw_collect<LONGEST> (regnum);
> + store_integer<LONGEST> ((gdb_byte *) addr, len, byte_order, val);
> }
> }
>
> diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
> index 48a582a16c934abe6e8f87c46a6009649c606d49..bac106ed5e9e5d89285420beb52b33d6ce36265a 100644
> --- a/gdb/mips-linux-tdep.c
> +++ b/gdb/mips-linux-tdep.c
> @@ -118,11 +118,10 @@ supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
> {
> struct gdbarch *gdbarch = get_regcache_arch (regcache);
> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> - gdb_byte buf[MAX_REGISTER_SIZE];
> - store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
> - extract_signed_integer ((const gdb_byte *) addr, 4,
> - byte_order));
> - regcache_raw_supply (regcache, regnum, buf);
> + LONGEST val;
> +
> + val = extract_integer<LONGEST> ((const gdb_byte *) addr, 4, byte_order);
> + regcache->raw_supply<LONGEST> (regnum, val);

If we declare raw_supply a little bit differently (see below), we can do

regcache->raw_supply (regnum, val);

> }
>
> /* Unpack an elf_gregset_t into GDB's register cache. */
> @@ -460,14 +459,9 @@ mips64_fill_gregset (const struct regcache *regcache,
>
> if (regaddr != -1)
> {
> - gdb_byte buf[MAX_REGISTER_SIZE];
> - LONGEST val;
> -
> - regcache_raw_collect (regcache, regno, buf);
> - val = extract_signed_integer (buf, register_size (gdbarch, regno),
> - byte_order);
> + LONGEST val = regcache->raw_collect<LONGEST> (regno);
> dst = regp + regaddr;
> - store_signed_integer ((gdb_byte *) dst, 8, byte_order, val);
> + store_integer<LONGEST> ((gdb_byte *) dst, 8, byte_order, val);

Likewise,

store_integer ((gdb_byte *) dst, 8, byte_order, val);

> }
> }
>

> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index 4dcfccbac70f0f962bf5e5596d035fda42322795..1ec8b98aa79c58c413786351a715ee46a0f5d5c2 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -21,6 +21,7 @@
> #define REGCACHE_H
>
> #include "common-regcache.h"
> +#include "defs.h"
>
> struct regcache;
> struct regset;
> @@ -294,8 +295,12 @@ public:
>
> void raw_collect (int regnum, void *buf) const;
>
> + template<typename T> LongType<T> raw_collect (int regnum) const;
> +
> void raw_supply (int regnum, const void *buf);
>
> + template<typename T> void raw_supply (int regnum, LongType<T> val);
> +

Similarly, we can do

template<typename T>
typename std::enable_if<(std::is_same<T, LONGEST>::value
|| std::is_same<T, ULONGEST>::value),
void>::type
raw_supply (int regnum, T val);

so that we don't need to explicit instantiate it.

> void raw_supply_zeroed (int regnum);
>
> void raw_copy (int regnum, struct regcache *src_regcache);
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 957b265c28d929376c3b7d8c100ea355d8292b94..4facc6b55e2bd08972cce1081b27e669103b6717 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1208,6 +1208,31 @@ regcache::raw_supply (int regnum, const void *buf)
> }
> }
>
> +/* Supply register REGNUM, whose contents are stored in VAL, to
> + REGCACHE. */
> +
> +template<typename T>
> +void
> +regcache::raw_supply (int regnum, LongType<T> val)
> +{
> + enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
> + gdb_byte *regbuf;
> + size_t size;
> +
> + gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
> + gdb_assert (!m_readonly_p);
> +
> + regbuf = register_buffer (regnum);
> + size = m_descr->sizeof_register[regnum];
> +
> + store_integer<T> (regbuf, size, byte_order, val);
> + m_register_status[regnum] = REG_VALID;
> +}

Again, why don't you put it in regcache.h?

--
Yao (齐尧)