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

[Yao Qi <qiyaoltc@gmail.com>]

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 (齐尧)