Re: [Patch, microblaze]: Port of Linux gdbserver

[Pedro Alves <palves@redhat.com>]

On 09/23/2014 01:49 PM, Ajit Kumar Agarwal wrote:

>>>> >>> Note nothing is done with valid_p.  It's write-only.  Compare with other ports, like arm-tdep.c or mips-tdep.c.
>> > 
>> > Would look into this and will make the modification.
> Thanks.

I'd much prefer if we had that patch in the tree before
accepting further patches that tweak things around register
names, etc.  Could you send that (as an independent patch,
in a new thread).

> +#define microblaze_num_regs	\
> +  (sizeof microblaze_regmap / sizeof microblaze_regmap[0])

#define microblaze_num_regs ARRAY_SIZE (microblaze_regmap)

> +
> +static int
> +microblaze_cannot_store_register (int regno)
> +{
> +  if (microblaze_regmap[regno] == -1 || regno == 0)
> +    return 1;
> +
> +  return 0;
> +}
> +
> +static int
> +microblaze_cannot_fetch_register (int regno)
> +{
> +  return 0;
> +}
> +
> +static CORE_ADDR
> +microblaze_get_pc (struct regcache *regcache)
> +{
> +  unsigned long pc;
> +  collect_register_by_name (regcache, "rpc", &pc);

Empty line after declaration.  In several more places in the patch.
Please fix them all.

> +  return (CORE_ADDR) (pc);
> +}



> +  if (regno == 0)
> +    {
> +      unsigned long regbuf_0 = 0;
> +      /* Clobbering r0 so that it is always 0 as enforced by hardware.  */
> +      supply_register (regcache, regno, (const char*)&regbuf_0);

	supply_register_zeroed (regcache, regno);

> +    }
> +  else
> +    {
> +      if (size < sizeof (long))
> +        supply_register (regcache, regno, buf + sizeof (long) - size);
> +      else
> +        supply_register (regcache, regno, buf);
> +    }
> +}
> +
> +/* Provide only a fill function for the general register set.  ps_lgetregs
> +   will use this for NPTL support.  */
> +
> +static void microblaze_fill_gregset (struct regcache *regcache, void *buf)

Line break after "static void".  Function name goes on column 0:

static void
microblaze_fill_gregset (struct regcache *regcache, void *buf)

Please make sure that's correct throughout.


> +{
> +  int i;
> +
> +  for (i = 0; i < microblaze_num_regs; i++)
> +    microblaze_collect_ptrace_register (regcache, i,
> +                                        (char *) buf + microblaze_regmap[i]);
> +}
> +
> +static void
> +microblaze_store_gregset (struct regcache *regcache, const void *buf)
> +{
> +  int i;
> +  for (i = 0; i < microblaze_num_regs; i++)
> +    supply_register (regcache, i, (char *) buf + microblaze_regmap[i]);
> +}
> +
> +#endif /* HAVE_PTRACE_GETREGS */
> +
> +static struct regset_info microblaze_regsets[] = {
> +#ifdef HAVE_PTRACE_GETREGS

What's the #ifdef for?

Did this kernel port make it upstream without PTRACE_GETREGSET?
If there's support for that, can you please switch to using it?

PTRACE_GETREGS is supposed to an old way of doing things...

> +  { PTRACE_GETREGS, PTRACE_SETREGS, 0, 36 * sizeof (elf_gregset_t),
> +    GENERAL_REGS, microblaze_fill_gregset, microblaze_store_gregset },
> +  { 0, 0, 0, -1, -1, NULL, NULL },
> +#endif /* HAVE_PTRACE_GETREGS */


> +  { 0, 0, 0, -1, -1, NULL, NULL }
> +};
> +



> diff --git a/gdb/regformats/microblaze-with-stack-protect.dat b/gdb/regformats/microblaze-with-stack-protect.dat
> index f71c111..e349b4a 100644
> --- a/gdb/regformats/microblaze-with-stack-protect.dat
> +++ b/gdb/regformats/microblaze-with-stack-protect.dat
> @@ -1,7 +1,7 @@
>  # DO NOT EDIT: generated from microblaze-with-stack-protect.xml
     ^^^^^^^^^^^
     ^^^^^^^^^^^
     ^^^^^^^^^^^
     ^^^^^^^^^^^
     ^^^^^^^^^^^

Please send a preparatory, independent, patch that updates
features/Makefile instead and generates this file, in a new
thread, with self-contained description, following the
checklist:
 https://sourceware.org/gdb/wiki/ContributionChecklist

>  name:microblaze_with_stack_protect
>  xmltarget:microblaze-with-stack-protect.xml
> -expedite:r1,pc
> +expedite:r1,rpc
>  32:r0
>  32:r1
>  32:r2
> -- 1.7.1
> 


Thanks,
Pedro Alves