Re: [RFA][PATCH v4 1/5] S/390 regmap rework

[Luis Machado <lgustavo@codesourcery.com>]

Hi,

A few nits and only nits.

On 07/03/2013 02:00 PM, Andreas Arnez wrote:
> S/390 regmap rework: Represent register maps in a less redundant and
> more readable way.  Also remove some code repetition.
>
>   /* GNU/Linux target descriptions.  */
>   extern struct target_desc *tdesc_s390_linux32;
> Index: gdb/gdb/s390-nat.c
> ===================================================================
> --- gdb.orig/gdb/s390-nat.c
> +++ gdb/gdb/s390-nat.c
> @@ -63,139 +63,129 @@ static int have_regset_system_call = 0;
>
>   #define regmap_fpregset s390_regmap_fpregset
>
> -/* When debugging a 32-bit executable running under a 64-bit kernel,
> -   we have to fix up the 64-bit registers we get from the kernel
> -   to make them look like 32-bit registers.  */
> +static void
> +s390_native_supply (struct regcache *regcache, const short *map,
> +		    const gdb_byte *regp)
> +{
> +  for (; map[0] >= 0; map += 2)
> +    regcache_raw_supply (regcache, map[1], regp + map[0]);
> +}
>
>   static void
> -s390_native_supply (struct regcache *regcache, int regno,
> -		    const gdb_byte *regp, int *regmap)
> +s390_native_collect (const struct regcache *regcache, const short *map,
> +		     int regno, gdb_byte *regp)
>   {
> -  int offset = regmap[regno];
> +  for (; map[0] >= 0; map += 2)
> +    if (regno == -1 || regno == map[1])
> +      regcache_raw_collect (regcache, map[1], regp + map[0]);
> +}

Maybe add a comment to these? Observing that...

> +
> +/* Fill GDB's register array with the general-purpose register values
> +   in *REGP.
>
> +   When debugging a 32-bit executable running under a 64-bit kernel,
> +   we have to fix up the 64-bit registers we get from the kernel to
> +   make them look like 32-bit registers.  */
> +void
> +supply_gregset (struct regcache *regcache, const gregset_t *regp)
> +{

... you need a empty line between the comment of a function and the 
declaration of that function.

>   #ifdef __s390x__
>     struct gdbarch *gdbarch = get_regcache_arch (regcache);
> -  if (offset != -1 && gdbarch_ptr_bit (gdbarch) == 32)
> +  if (gdbarch_ptr_bit (gdbarch) == 32)
>       {
>         enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +      ULONGEST pswm = 0, pswa = 0;
> +      gdb_byte buf[4];
> +      const short *map;
>
> -      if (regno == S390_PSWM_REGNUM)
> -	{
> -	  ULONGEST pswm;
> -	  gdb_byte buf[4];
> -
> -	  pswm = extract_unsigned_integer (regp + regmap[S390_PSWM_REGNUM],
> -					   8, byte_order);
> -
> -	  store_unsigned_integer (buf, 4, byte_order, (pswm >> 32) | 0x80000);
> -	  regcache_raw_supply (regcache, regno, buf);
> -	  return;
> -	}
> -
> -      if (regno == S390_PSWA_REGNUM)
> +      for (map = regmap_gregset; map[0] >= 0; map += 2)
>   	{
> -	  ULONGEST pswm, pswa;
> -	  gdb_byte buf[4];
> +	  const gdb_byte *p = (const gdb_byte *) regp + map[0];
> +	  int regno = map[1];
>
> -	  pswa = extract_unsigned_integer (regp + regmap[S390_PSWA_REGNUM],
> -					   8, byte_order);
> -	  pswm = extract_unsigned_integer (regp + regmap[S390_PSWM_REGNUM],
> -					   8, byte_order);
> -
> -	  store_unsigned_integer (buf, 4, byte_order,
> -				  (pswa & 0x7fffffff) | (pswm & 0x80000000));
> -	  regcache_raw_supply (regcache, regno, buf);
> -	  return;
> +	  if (regno == S390_PSWM_REGNUM)
> +	    pswm = extract_unsigned_integer (p, 8, byte_order);
> +	  else if (regno == S390_PSWA_REGNUM)
> +	    pswa = extract_unsigned_integer (p, 8, byte_order);
> +	  else
> +	    {
> +	      if ((regno >= S390_R0_REGNUM && regno <= S390_R15_REGNUM)
> +		  || regno == S390_ORIG_R2_REGNUM)
> +		p += 4;
> +	      regcache_raw_supply (regcache, regno, p);
> +	    }
>   	}
>
> -      if ((regno >= S390_R0_REGNUM && regno <= S390_R15_REGNUM)
> -	  || regno == S390_ORIG_R2_REGNUM)
> -	offset += 4;
> +      store_unsigned_integer (buf, 4, byte_order, (pswm >> 32) | 0x80000);
> +      regcache_raw_supply (regcache, S390_PSWM_REGNUM, buf);
> +      store_unsigned_integer (buf, 4, byte_order,
> +			      (pswa & 0x7fffffff) | (pswm & 0x80000000));
> +      regcache_raw_supply (regcache, S390_PSWA_REGNUM, buf);
> +      return;
>       }
>   #endif
>
> -  if (offset != -1)
> -    regcache_raw_supply (regcache, regno, regp + offset);
> +  s390_native_supply (regcache, regmap_gregset, (const gdb_byte *) regp);
>   }
>
> -static void
> -s390_native_collect (const struct regcache *regcache, int regno,
> -		     gdb_byte *regp, int *regmap)
> +/* Fill register REGNO (if it is a general-purpose register) in
> +   *REGP with the value in GDB's register array.  If REGNO is -1,
> +   do this for all registers.  */
> +void
> +fill_gregset (const struct regcache *regcache, gregset_t *regp, int regno)

Another instance of a missing empty line after the comment.