RE: [Patch, microblaze]: Fix for remote G Packet message too long error for baremetal.

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

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Tuesday, June 24, 2014 2:44 PM
To: Ajit Kumar Agarwal
Cc: gdb-patches@sourceware.org; Michael Eager; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch, microblaze]: Fix for remote G Packet message too long error for baremetal.

Hi,

>>I read this again and found things I should have mentioned before or things I mentioned before but weren't addressed.  See below.

Thanks !!

On 06/23/2014 08:36 PM, Ajit Kumar Agarwal wrote:

> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c index 
> 14c1b52..c57437c 100644
> --- a/gdb/microblaze-tdep.c
> +++ b/gdb/microblaze-tdep.c
> @@ -33,13 +33,15 @@
>  #include "frame-unwind.h"
>  #include "dwarf2-frame.h"
>  #include "osabi.h"
> -
> +#include "features/microblaze-with-stack-protect.c"
> +#include "features/microblaze.c"
>  #include "gdb_assert.h"

>>A little odd to see the .c files included in the middle of the .h files.  All other files seem to include the ".c" files after all the .h files.  I wonder which existing file you modelled from?

I will incorporate this change.

>  #include <string.h>
>  #include "target-descriptions.h"
>  #include "opcodes/microblaze-opcm.h"
>  #include "opcodes/microblaze-dis.h"
>  #include "microblaze-tdep.h"
> +#include "remote.h"
>  
> 
>  /* Instruction macros used for analyzing the prologue.  */
>  /* This set of instruction macros need to be changed whenever the @@ 
> -73,7 +75,8 @@ static const char *microblaze_register_names[] =
>    "rpc",  "rmsr", "rear", "resr", "rfsr", "rbtr",
>    "rpvr0", "rpvr1", "rpvr2", "rpvr3", "rpvr4", "rpvr5", "rpvr6",
>    "rpvr7", "rpvr8", "rpvr9", "rpvr10", "rpvr11",
> -  "redr", "rpid", "rzpr", "rtlbx", "rtlbsx", "rtlblo", "rtlbhi"
> +  "redr", "rpid", "rzpr", "rtlbx", "rtlbsx", "rtlblo", "rtlbhi",  
> + "rslr", "rshr"
>  };
>  
>  #define MICROBLAZE_NUM_REGS ARRAY_SIZE (microblaze_register_names) @@ 
> -663,17 +666,66 @@ microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
>    gdb_assert (reg < sizeof (dwarf2_to_reg_map));
>    return dwarf2_to_reg_map[reg];
>  }
> +static void

>>Please make you sure there's an empty line between functions.  I believe I commented on this before.

I will make this change.

> +microblaze_register_g_packet_guesses (struct gdbarch *gdbarch) {
> +  register_remote_g_packet_guess (gdbarch,
> +                                  MICROBLAZE_NUM_CORE_REGS, 
> +                                  tdesc_microblaze);
>  
> +  register_remote_g_packet_guess (gdbarch,
> +                                  MICROBLAZE_NUM_REGS,
> +                                  
> +tdesc_microblaze_with_stack_protect);
> +}
>  static struct gdbarch *

Here too.

>  microblaze_gdbarch_init (struct gdbarch_info info, struct 
> gdbarch_list *arches)  {
>    struct gdbarch_tdep *tdep;
>    struct gdbarch *gdbarch;
> +  struct tdesc_arch_data *tdesc_data = NULL;  const struct 
> + target_desc *tdesc = info.target_desc;
>  
>    /* If there is already a candidate, use it.  */
>    arches = gdbarch_list_lookup_by_info (arches, &info);
>    if (arches != NULL)
>      return arches->gdbarch;
> +  if (tdesc == NULL)
> +    tdesc = tdesc_microblaze_with_stack_protect;

Shouldn't the default be to _not_ assume stack protect ?

The default is choosen to assume stack protect to make compatible with the handling of stack protect registers in XMD Debugger.

> +  /* Check any target description for validity.  */  if 
> + (tdesc_has_registers (tdesc))
> +    {
> +      const struct tdesc_feature *feature;
> +      int valid_p;
> +      int i;
> +
> +      feature = tdesc_find_feature (tdesc,
> +                                    "org.gnu.gdb.microblaze.core");
> +      if (feature == NULL)
> +        return NULL;
> +      tdesc_data = tdesc_data_alloc ();
> +
> +      valid_p = 1;
> +      for (i = 0; i < MICROBLAZE_NUM_CORE_REGS; i++)
> +        valid_p &= tdesc_numbered_register (feature, tdesc_data, i,
> +                                            microblaze_register_names[i]);
> +      feature = tdesc_find_feature (tdesc,
> +                                    
> + "org.gnu.gdb.microblaze.stack-protect");
> +
> +      if (feature != NULL)
> +        {
> +          valid_p = 1;
> +          valid_p &= tdesc_numbered_register (feature, tdesc_data,
> +                                              MICROBLAZE_SLR_REGNUM,
> +                                              "rslr");
> +          valid_p &= tdesc_numbered_register (feature, tdesc_data,
> +                                              MICROBLAZE_SHR_REGNUM,
> +                                              "rshr");
> +        }
> +     }
> +  tdep = xcalloc (1, sizeof (struct gdbarch_tdep));  gdbarch = 
> + gdbarch_alloc (&info, tdep);
> +
> +  microblaze_register_g_packet_guesses (gdbarch);
>  
>    /* Allocate space for the new architecture.  */
>    tdep = XNEW (struct gdbarch_tdep);
> @@ -725,7 +777,11 @@ microblaze_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    dwarf2_append_unwinders (gdbarch);
>    frame_unwind_append_unwinder (gdbarch, &microblaze_frame_unwind);
>    frame_base_append_sniffer (gdbarch, dwarf2_frame_base_sniffer);
> -
> +  if (tdesc_data != NULL)
> +    {
> +      tdesc_use_registers (gdbarch, tdesc, tdesc_data);
> +      set_gdbarch_register_type (gdbarch, microblaze_register_type);

>>Hmm, why is this set_gdbarch_register_type call necessary?

/* Override tdesc_register_type to adjust the types of VFP
         registers for NEON.  */
This is done for arm target  to set the different type for VFP registers for Neon with Boolean flags is set before this call for VFP registers. In the microblaze target it's not required for special case of stack protect as the microblaze_register_type always return builtin_int for these stack protect registers.


> +    }
>    return gdbarch;
>  }
>  


> +  /* Offsets to saved registers.  */
> +  int register_offsets[MICROBLAZE_NUM_REGS];    /* Must match MICROBLAZE_NUM_REGS.  */

>>The "Must match MICROBLAZE_NUM_REGS" comment now looks unnecessary to me.

I will make this change.

>>As I mentioned before, please don't forget to document the new target features in the manual.

Would you mind in explaining which manual need to be changed for the new target.
--
Pedro Alves