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

[Pedro Alves <palves at redhat dot com>]

Hi,

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

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?

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

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

> +  /* 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?

> +    }
>    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.

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

-- 
Pedro Alves