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

Luis Machado
2013-07-15T14:46:17Z

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.