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

[Ajit Kumar Agarwal <ajit dot kumar dot agarwal at xilinx dot com>]

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Tuesday, September 30, 2014 5:14 PM
To: Ajit Kumar Agarwal; Michael Eager; Joel Brobecker
Cc: gdb-patches@sourceware.org; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch, microblaze]: Port of Linux gdbserver

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).

I will do this and send a separate independent patch.

> +#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

I have created this file instead of editing. I should have written as  New file in the ChangeLog. Still You want me to send a separate patch.

Thanks  & Regards
Ajit

>  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