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

  Thanks Pedro for the suggestions and review comments. Based on the feedback, the patch
attached.

    [Patch, microblaze]: Fix for remote G Packet message too long error for baremetal.
    
    Prior to version MicroBlaze v8.10.a,EDK 13.1, XMD's gdbserver stub returned 57
    registers in response to GDB's G request. Starting with version MicroBlaze
    v8.10.a, EDK 13.1, XMD added the slr and shr register, for a count of 59
    registers. This patch adds these registers to the expected G response. This patch
    fixes the above problem for baremetal and also supports the backward compatibility.
    
    ChangeLog:
    2014-06-24  Ajit Agarwal  <ajitkum@xilinx.com>
    
        * microblaze-tdep.c (microblaze_register_names): Add
        the rshr and rslr register names.
        (microblaze_gdbarch_init): Use of tdesc_has_registers.
        Use of tdesc_find_feature. Use of tdesc_data_alloc.
        Use of tdesc_numbered_register. Use of
        microblaze_register_g_packet_guesses. Use of
        tdesc_use_registers. Use of set_gdbarch_register_type.
        (microblaze_register_g_packet_guesses): New.
        * microblaze-tdep.h (microblaze_reg_num): Add
        field MICROBLAZE_SLR_REGNUM MICROBLAZE_SHR_REGNUM
        MICROBLAZE_NUM_REGS and MICROBLAZE_NUM_CORE_REGS.
        (microblaze_frame_cache): Use of MICROBLAZE_NUM_REGS.
        * features/microblaze-core.xml: New file.
        * features/microblaze-stack-protect.xml: New file.
        * features/microblaze-with-stack-protect.c: New file.
        * features/microblaze-with-stack-protect.xml: New file.
        * features/microblaze.xml: New file.
        * features/microblaze.c: New file.
        * features/Makefile (microblaze-with-stack-protect): Add
        microblaze-with-stack-protect microblaze and
        microblaze-expedite.
        * regformats/microblaze-with-stack-protect.dat: New file.
        * regformats/microblaze.dat: New file.

    Signed-off-by:Ajit Agarwal ajitkum@xilinx.com

---
 gdb/features/Makefile                            |    2 +
 gdb/features/microblaze-core.xml                 |   67 ++++++++++++++++++
 gdb/features/microblaze-stack-protect.xml        |   12 +++
 gdb/features/microblaze-with-stack-protect.c     |   79 ++++++++++++++++++++++
 gdb/features/microblaze-with-stack-protect.xml   |   12 +++
 gdb/features/microblaze.c                        |   75 ++++++++++++++++++++
 gdb/features/microblaze.xml                      |   11 +++
 gdb/microblaze-tdep.c                            |   64 +++++++++++++++++-
 gdb/microblaze-tdep.h                            |   48 +++++++------
 gdb/regformats/microblaze-with-stack-protect.dat |   63 +++++++++++++++++
 gdb/regformats/microblaze.dat                    |   61 +++++++++++++++++
 11 files changed, 468 insertions(+), 26 deletions(-)
 create mode 100644 gdb/features/microblaze-core.xml
 create mode 100644 gdb/features/microblaze-stack-protect.xml
 create mode 100644 gdb/features/microblaze-with-stack-protect.c
 create mode 100644 gdb/features/microblaze-with-stack-protect.xml
 create mode 100644 gdb/features/microblaze.c
 create mode 100644 gdb/features/microblaze.xml
 create mode 100644 gdb/regformats/microblaze-with-stack-protect.dat
 create mode 100644 gdb/regformats/microblaze.dat
  

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.
  

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

On 06/24/2014 11:28 AM, Ajit Kumar Agarwal wrote:

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

But you've already added the G packet size guess for that.

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

Right.

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

The GDB manual, gdb/doc/gdb.texinfo, describes all the standard XML target
features.  See the "Standard Target Features" node, and add a new subsection
for MicroBlaze.
  

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Tuesday, June 24, 2014 5:36 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.

On 06/24/2014 11:28 AM, Ajit Kumar Agarwal wrote:

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

>> But you've already added the G packet size guess for that.

In this case is it correct to say 
If (tdesc  == NULL)
  tdesc = tdesc_microblaze; 

instead of tdesc_microblaze_with_stack_protect?

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

Right.

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

>> The GDB manual, gdb/doc/gdb.texinfo, describes all the standard XML target features.  See the "Standard Target Features" node, and add a new subsection for MicroBlaze.

Thanks !! I will add subsection for Microblaze target.
--
Pedro Alves
  

On 06/24/2014 01:31 PM, Ajit Kumar Agarwal wrote:

>>
>> The default is choosen to assume stack protect to make compatible with the handling of stack protect registers in XMD Debugger.
> 
>>> But you've already added the G packet size guess for that.
> 
> In this case is it correct to say 
> If (tdesc  == NULL)
>   tdesc = tdesc_microblaze; 
> 
> instead of tdesc_microblaze_with_stack_protect?

Yes.

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

Actually, not right...  This comment doesn't really appear to be correct:

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


static struct type *
microblaze_register_type (struct gdbarch *gdbarch, int regnum)
{
  if (regnum == MICROBLAZE_SP_REGNUM)
    return builtin_type (gdbarch)->builtin_data_ptr;

  if (regnum == MICROBLAZE_PC_REGNUM)
    return builtin_type (gdbarch)->builtin_func_ptr;

  return builtin_type (gdbarch)->builtin_int;
}

MICROBLAZE_SP_REGNUM and MICROBLAZE_PC_REGNUM clearly
aren't builtin_int...

Doesn't your patch change the output of "ptype $sp" and
"ptype $pc" ?

That points at something missing in the target description:

> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> +<feature name="org.gnu.gdb.microblaze.core">
> +  <reg name="r1" bitsize="32"/>
...
> +  <reg name="rpc" bitsize="32"/>

AFAICS, SP is "r1", and PC is "rpc".  These should be marked with
type="data_ptr" and type="code_ptr" .

> 
>>>> 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.
> 
>>> The GDB manual, gdb/doc/gdb.texinfo, describes all the standard XML target features.  See the "Standard Target Features" node, and add a new subsection for MicroBlaze.
> 
> Thanks !! I will add subsection for Microblaze target.

Thank you.
  

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Tuesday, June 24, 2014 6:17 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.

On 06/24/2014 01:31 PM, Ajit Kumar Agarwal wrote:

>>
>> The default is choosen to assume stack protect to make compatible with the handling of stack protect registers in XMD Debugger.
> 
>>> But you've already added the G packet size guess for that.
> 
> In this case is it correct to say
> If (tdesc  == NULL)
>   tdesc = tdesc_microblaze;
> 
> instead of tdesc_microblaze_with_stack_protect?

Yes.

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

Actually, not right...  This comment doesn't really appear to be correct:

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



>>static struct type *
>>microblaze_register_type (struct gdbarch *gdbarch, int regnum) {
  >>if (regnum == MICROBLAZE_SP_REGNUM)
   >> return builtin_type (gdbarch)->builtin_data_ptr;

  >>if (regnum == MICROBLAZE_PC_REGNUM)
    >>return builtin_type (gdbarch)->builtin_func_ptr;

  >>return builtin_type (gdbarch)->builtin_int; }

What I meant the stack protect register are neither MICROBLAZE_SP_REGNUM nor MICROBLAZE_PC_REGNUM so it always return register_type as builtin_int as derived from the code.

MICROBLAZE_SP_REGNUM and MICROBLAZE_PC_REGNUM clearly aren't builtin_int...

>>Doesn't your patch change the output of "ptype $sp" and "ptype $pc" ?

I will check this and add the changes mentioned below in xml files.

That points at something missing in the target description:

> +<!DOCTYPE feature SYSTEM "gdb-target.dtd"> <feature 
> +name="org.gnu.gdb.microblaze.core">
> +  <reg name="r1" bitsize="32"/>
...
> +  <reg name="rpc" bitsize="32"/>

>>AFAICS, SP is "r1", and PC is "rpc".  These should be marked with type="data_ptr" and type="code_ptr" .

Surely I will do this.

> 
>>>> 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.
> 
>>> The GDB manual, gdb/doc/gdb.texinfo, describes all the standard XML target features.  See the "Standard Target Features" node, and add a new subsection for MicroBlaze.
> 
> Thanks !! I will add subsection for Microblaze target.

Thank you.

--
Pedro Alves
  

On 06/24/14 05:05, Pedro Alves wrote:
> On 06/24/2014 11:28 AM, Ajit Kumar Agarwal wrote:
>>>> 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.
>
> The GDB manual, gdb/doc/gdb.texinfo, describes all the standard XML target
> features.  See the "Standard Target Features" node, and add a new subsection
> for MicroBlaze.

microblaze-gdb connects to the target using a JTAG interface which talks
to a Xilinx-proprietary program called XMD.  XMD implements the gdbserver
protocol.  As far as I'm aware, XMD does not provide target feature
descriptions.
  

-----Original Message-----
From: Michael Eager [mailto:eager@eagerm.com] 

Sent: Tuesday, June 24, 2014 7:41 PM
To: Pedro Alves; 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.

On 06/24/14 05:05, Pedro Alves wrote:
> On 06/24/2014 11:28 AM, Ajit Kumar Agarwal wrote:

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

>

> The GDB manual, gdb/doc/gdb.texinfo, describes all the standard XML 

> target features.  See the "Standard Target Features" node, and add a 

> new subsection for MicroBlaze.


>>microblaze-gdb connects to the target using a JTAG interface which talks to a Xilinx-proprietary program called XMD.  XMD implements the gdbserver protocol.  As far as I'm aware, >>XMD does not provide target feature descriptions.


XMD gdb server does not have target feature descriptions. 
-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077
  

On 06/24/2014 03:10 PM, Michael Eager wrote:
> On 06/24/14 05:05, Pedro Alves wrote:
>> On 06/24/2014 11:28 AM, Ajit Kumar Agarwal wrote:
>>>>> 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.
>>
>> The GDB manual, gdb/doc/gdb.texinfo, describes all the standard XML target
>> features.  See the "Standard Target Features" node, and add a new subsection
>> for MicroBlaze.
> 
> microblaze-gdb connects to the target using a JTAG interface which talks
> to a Xilinx-proprietary program called XMD.  XMD implements the gdbserver
> protocol.  As far as I'm aware, XMD does not provide target feature
> descriptions.

It's expected that XMD does not provide target descriptions, because the support
on the GDB side was missing.  Ajit's patch teaches the microblaze port about
a couple new standard features.  All standard xml target features are defined
by GDB, and should be documented.  Nothing stops GDB from talking to servers other
than XMD after all.  After the patch is in, if future XMDs (or other servers) want to
expose random microblaze registers to GDB, like, e.g., I/O control registers, it
just needs to send in a target description that includes them, and GDB will
present them (without GDB changes).  Meanwhile, Ajit's patch keeps compatibility
with current XMDs by adding a G packet size to target description
mapping (a standard transitioning mechanism also used by ARM and MIPS).

Hope that clarifies things.

Thanks,
  

Based on the feedback, updated the patch with the review comments.
    
[Patch, microblaze]: Fix for remote G Packet message too long error for baremetal.
    
    Prior to version MicroBlaze v8.10.a,EDK 13.1, XMD's gdbserver stub returned 57
    registers in response to GDB's G request. Starting with version MicroBlaze
    v8.10.a, EDK 13.1, XMD added the slr and shr register, for a count of 59
    registers. This patch adds these registers to the expected G response. This patch
    fixes the above problem for baremetal and also supports the backward compatibility.
    
    ChangeLog:
    2014-06-26  Ajit Agarwal  <ajitkum@xilinx.com>
    
        * microblaze-tdep.c (microblaze_register_names): Add
        the rshr and rslr register names.
        (microblaze_gdbarch_init): Use of tdesc_has_registers.
        Use of tdesc_find_feature. Use of tdesc_data_alloc.
        Use of tdesc_numbered_register. Use of
        microblaze_register_g_packet_guesses. Use of
        tdesc_use_registers. Use of set_gdbarch_register_type.
        (microblaze_register_g_packet_guesses): New.
        * microblaze-tdep.h (microblaze_reg_num): Add
        field MICROBLAZE_SLR_REGNUM MICROBLAZE_SHR_REGNUM
        MICROBLAZE_NUM_REGS and MICROBLAZE_NUM_CORE_REGS.
        (microblaze_frame_cache): Use of MICROBLAZE_NUM_REGS.
        * features/microblaze-core.xml: New file.
        * features/microblaze-stack-protect.xml: New file.
        * features/microblaze-with-stack-protect.c: New file.
        * features/microblaze-with-stack-protect.xml: New file.
        * features/microblaze.xml: New file.
        * features/microblaze.c: New file.
        * features/Makefile (microblaze-with-stack-protect): Add
        microblaze-with-stack-protect microblaze and
        microblaze-expedite.
        * regformats/microblaze-with-stack-protect.dat: New file.
        * regformats/microblaze.dat: New file.
        * doc/gdb.texinfo (MicroBlaze Features): New.
    
    Signed-off-by:Ajit Agarwal ajitkum@xilinx.com

> In this case is it correct to say
> If (tdesc  == NULL)
>   tdesc = tdesc_microblaze;
> 
> instead of tdesc_microblaze_with_stack_protect?

>>Yes.

Instead of tdesc_microblaze_with_stack_protect if I use tdesc_microblaze  the "G Packet message is too long" error is not resolved.
The patch is unchanged with tdesc_microblaze_stack_protect.

Thanks & Regards
Ajit
-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Tuesday, June 24, 2014 6:17 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.

On 06/24/2014 01:31 PM, Ajit Kumar Agarwal wrote:

>>
>> The default is choosen to assume stack protect to make compatible with the handling of stack protect registers in XMD Debugger.
> 
>>> But you've already added the G packet size guess for that.
> 
> In this case is it correct to say
> If (tdesc  == NULL)
>   tdesc = tdesc_microblaze;
> 
> instead of tdesc_microblaze_with_stack_protect?

Yes.

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

Actually, not right...  This comment doesn't really appear to be correct:

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


static struct type *
microblaze_register_type (struct gdbarch *gdbarch, int regnum) {
  if (regnum == MICROBLAZE_SP_REGNUM)
    return builtin_type (gdbarch)->builtin_data_ptr;

  if (regnum == MICROBLAZE_PC_REGNUM)
    return builtin_type (gdbarch)->builtin_func_ptr;

  return builtin_type (gdbarch)->builtin_int; }

MICROBLAZE_SP_REGNUM and MICROBLAZE_PC_REGNUM clearly aren't builtin_int...

Doesn't your patch change the output of "ptype $sp" and "ptype $pc" ?

That points at something missing in the target description:

> +<!DOCTYPE feature SYSTEM "gdb-target.dtd"> <feature 
> +name="org.gnu.gdb.microblaze.core">
> +  <reg name="r1" bitsize="32"/>
...
> +  <reg name="rpc" bitsize="32"/>

AFAICS, SP is "r1", and PC is "rpc".  These should be marked with type="data_ptr" and type="code_ptr" .

> 
>>>> 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.
> 
>>> The GDB manual, gdb/doc/gdb.texinfo, describes all the standard XML target features.  See the "Standard Target Features" node, and add a new subsection for MicroBlaze.
> 
> Thanks !! I will add subsection for Microblaze target.

Thank you.

--
Pedro Alves
  

On 06/30/2014 11:32 AM, Ajit Kumar Agarwal wrote:
>     Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
> 
>> > In this case is it correct to say
>> > If (tdesc  == NULL)
>> >   tdesc = tdesc_microblaze;
>> > 
>> > instead of tdesc_microblaze_with_stack_protect?
>>> >>Yes.
> Instead of tdesc_microblaze_with_stack_protect if I use tdesc_microblaze  the "G Packet message is too long" error is not resolved.

Then it sounds like the G packet size guesses you're adding
aren't actually triggering.  Why?

> The patch is unchanged with tdesc_microblaze_stack_protect.
  

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Monday, June 30, 2014 4:25 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.

On 06/30/2014 11:32 AM, Ajit Kumar Agarwal wrote:
>     Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
> 
>> > In this case is it correct to say
>> > If (tdesc  == NULL)
>> >   tdesc = tdesc_microblaze;
>> > 
>> > instead of tdesc_microblaze_with_stack_protect?
>>> >>Yes.
> Instead of tdesc_microblaze_with_stack_protect if I use tdesc_microblaze  the "G Packet message is too long" error is not resolved.

>>Then it sounds like the G packet size guesses you're adding aren't actually triggering.  Why?

I have checked the guesses are actually triggering as it works fine with backward compatibility with the Designs there is no stack-protect registers. For the Design that has the 
Stack protect register, it reports the message " G packet too long ". 

> The patch is unchanged with tdesc_microblaze_stack_protect.

--
Pedro Alves
  

On 06/30/2014 12:13 PM, Ajit Kumar Agarwal wrote:
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com] 
> Sent: Monday, June 30, 2014 4:25 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.
> 
> On 06/30/2014 11:32 AM, Ajit Kumar Agarwal wrote:
>>     Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
>>
>>>> In this case is it correct to say
>>>> If (tdesc  == NULL)
>>>>   tdesc = tdesc_microblaze;
>>>>
>>>> instead of tdesc_microblaze_with_stack_protect?
>>>>>> Yes.
>> Instead of tdesc_microblaze_with_stack_protect if I use tdesc_microblaze  the "G Packet message is too long" error is not resolved.
> 
>>> Then it sounds like the G packet size guesses you're adding aren't actually triggering.  Why?
> 
> I have checked the guesses are actually triggering as it works fine with backward compatibility with the Designs there is no stack-protect registers. For the Design that has the 
> Stack protect register, it reports the message " G packet too long ". 

If the G guess is triggering OK, and so GDB picks the description with the
stack protect registers based on the G packet size, why would the default
target description matter at all?  If it does, then something sounds broken.
What are you doing different from the other ports that use this mechanism?
Please debug this a bit further.
  

On 06/30/14 03:32, Ajit Kumar Agarwal wrote:
> Based on the feedback, updated the patch with the review comments.

How has this been tested?  Please be specific.

>
> [Patch, microblaze]: Fix for remote G Packet message too long error for baremetal.
>
>      Prior to version MicroBlaze v8.10.a,EDK 13.1, XMD's gdbserver stub returned 57
>      registers in response to GDB's G request. Starting with version MicroBlaze
>      v8.10.a, EDK 13.1, XMD added the slr and shr register, for a count of 59
>      registers. This patch adds these registers to the expected G response. This patch
>      fixes the above problem for baremetal and also supports the backward compatibility.
>
>      ChangeLog:
>      2014-06-26  Ajit Agarwal  <ajitkum@xilinx.com>
>
>          * microblaze-tdep.c (microblaze_register_names): Add
>          the rshr and rslr register names.
>          (microblaze_gdbarch_init): Use of tdesc_has_registers.
>          Use of tdesc_find_feature. Use of tdesc_data_alloc.
>          Use of tdesc_numbered_register. Use of
>          microblaze_register_g_packet_guesses. Use of
>          tdesc_use_registers. Use of set_gdbarch_register_type.
>          (microblaze_register_g_packet_guesses): New.
>          * microblaze-tdep.h (microblaze_reg_num): Add
>          field MICROBLAZE_SLR_REGNUM MICROBLAZE_SHR_REGNUM
>          MICROBLAZE_NUM_REGS and MICROBLAZE_NUM_CORE_REGS.
>          (microblaze_frame_cache): Use of MICROBLAZE_NUM_REGS.
>          * features/microblaze-core.xml: New file.
>          * features/microblaze-stack-protect.xml: New file.
>          * features/microblaze-with-stack-protect.c: New file.
>          * features/microblaze-with-stack-protect.xml: New file.
>          * features/microblaze.xml: New file.
>          * features/microblaze.c: New file.
>          * features/Makefile (microblaze-with-stack-protect): Add
>          microblaze-with-stack-protect microblaze and
>          microblaze-expedite.
>          * regformats/microblaze-with-stack-protect.dat: New file.
>          * regformats/microblaze.dat: New file.
>          * doc/gdb.texinfo (MicroBlaze Features): New.
>
>      Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
>
>> In this case is it correct to say
>> If (tdesc  == NULL)
>>    tdesc = tdesc_microblaze;
>>
>> instead of tdesc_microblaze_with_stack_protect?
>
>>> Yes.
>
> Instead of tdesc_microblaze_with_stack_protect if I use tdesc_microblaze  the "G Packet message is too long" error is not resolved.
> The patch is unchanged with tdesc_microblaze_stack_protect.
>
> Thanks & Regards
> Ajit
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Tuesday, June 24, 2014 6:17 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.
>
> On 06/24/2014 01:31 PM, Ajit Kumar Agarwal wrote:
>
>>>
>>> The default is choosen to assume stack protect to make compatible with the handling of stack protect registers in XMD Debugger.
>>
>>>> But you've already added the G packet size guess for that.
>>
>> In this case is it correct to say
>> If (tdesc  == NULL)
>>    tdesc = tdesc_microblaze;
>>
>> instead of tdesc_microblaze_with_stack_protect?
>
> Yes.
>
>>
>>>> -
>>>> +  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.
>>
>> Right.
>
> Actually, not right...  This comment doesn't really appear to be correct:
>
>> 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.
>
>
> static struct type *
> microblaze_register_type (struct gdbarch *gdbarch, int regnum) {
>    if (regnum == MICROBLAZE_SP_REGNUM)
>      return builtin_type (gdbarch)->builtin_data_ptr;
>
>    if (regnum == MICROBLAZE_PC_REGNUM)
>      return builtin_type (gdbarch)->builtin_func_ptr;
>
>    return builtin_type (gdbarch)->builtin_int; }
>
> MICROBLAZE_SP_REGNUM and MICROBLAZE_PC_REGNUM clearly aren't builtin_int...
>
> Doesn't your patch change the output of "ptype $sp" and "ptype $pc" ?
>
> That points at something missing in the target description:
>
>> +<!DOCTYPE feature SYSTEM "gdb-target.dtd"> <feature
>> +name="org.gnu.gdb.microblaze.core">
>> +  <reg name="r1" bitsize="32"/>
> ...
>> +  <reg name="rpc" bitsize="32"/>
>
> AFAICS, SP is "r1", and PC is "rpc".  These should be marked with type="data_ptr" and type="code_ptr" .
>
>>
>>>>> 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.
>>
>>>> The GDB manual, gdb/doc/gdb.texinfo, describes all the standard XML target features.  See the "Standard Target Features" node, and add a new subsection for MicroBlaze.
>>
>> Thanks !! I will add subsection for Microblaze target.
>
> Thank you.
>
> --
> Pedro Alves
>
  

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Monday, June 30, 2014 4:53 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.

On 06/30/2014 12:13 PM, Ajit Kumar Agarwal wrote:
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Monday, June 30, 2014 4:25 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.
> 
> On 06/30/2014 11:32 AM, Ajit Kumar Agarwal wrote:
>>     Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
>>
>>>> In this case is it correct to say
>>>> If (tdesc  == NULL)
>>>>   tdesc = tdesc_microblaze;
>>>>
>>>> instead of tdesc_microblaze_with_stack_protect?
>>>>>> Yes.
>> Instead of tdesc_microblaze_with_stack_protect if I use tdesc_microblaze  the "G Packet message is too long" error is not resolved.
> 
>>> Then it sounds like the G packet size guesses you're adding aren't actually triggering.  Why?
> 
> I have checked the guesses are actually triggering as it works fine 
> with backward compatibility with the Designs there is no stack-protect registers. For the Design that has the Stack protect register, it reports the message " G packet too long ".

>>If the G guess is triggering OK, and so GDB picks the description with the stack protect registers based on the G packet size, why would the default target description matter at all?  If it >>does, then something sounds broken.

In Microblaze Port the gdbarch_info has  tdesc always  set to NULL. Thus the gdbarch_init doesn't populates the register info based feature target description. Though the microblaze_register_g_packet_guesses is called by gdbarch_init and  register the guesses,  but the gdbarch_info.tdesc == NULL always and  features target description info is populated based on default target description.

>>What are you doing different from the other ports that use this mechanism?

In Microblaze port as compared to other architecture port lacking the function microblaze_read_description which sets the tdesc and the architecture set up based the Linux Kernel and Ptrace parameters. The other architecture  like ARM and MIPS has  arm_read_description and arm_linux_read_description which sets the tdesc info current()->Process.

>>Please debug this a bit further.

--
Pedro Alves
  

-----Original Message-----
From: Michael Eager [mailto:eager@eagercon.com] 

Sent: Monday, June 30, 2014 8:18 PM
To: Ajit Kumar Agarwal; Pedro Alves
Cc: gdb-patches@sourceware.org; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch, microblaze]: Fix for remote G Packet message too long error for baremetal.

On 06/30/14 03:32, Ajit Kumar Agarwal wrote:
> Based on the feedback, updated the patch with the review comments.


>>How has this been tested?  Please be specific.


The patch has been tested with in-house applications with the older and newer designs for tar remote localhost:1234( 1234 port number) command.
>

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

>

>      Prior to version MicroBlaze v8.10.a,EDK 13.1, XMD's gdbserver stub returned 57

>      registers in response to GDB's G request. Starting with version MicroBlaze

>      v8.10.a, EDK 13.1, XMD added the slr and shr register, for a count of 59

>      registers. This patch adds these registers to the expected G response. This patch

>      fixes the above problem for baremetal and also supports the backward compatibility.

>

>      ChangeLog:

>      2014-06-26  Ajit Agarwal  <ajitkum@xilinx.com>

>

>          * microblaze-tdep.c (microblaze_register_names): Add

>          the rshr and rslr register names.

>          (microblaze_gdbarch_init): Use of tdesc_has_registers.

>          Use of tdesc_find_feature. Use of tdesc_data_alloc.

>          Use of tdesc_numbered_register. Use of

>          microblaze_register_g_packet_guesses. Use of

>          tdesc_use_registers. Use of set_gdbarch_register_type.

>          (microblaze_register_g_packet_guesses): New.

>          * microblaze-tdep.h (microblaze_reg_num): Add

>          field MICROBLAZE_SLR_REGNUM MICROBLAZE_SHR_REGNUM

>          MICROBLAZE_NUM_REGS and MICROBLAZE_NUM_CORE_REGS.

>          (microblaze_frame_cache): Use of MICROBLAZE_NUM_REGS.

>          * features/microblaze-core.xml: New file.

>          * features/microblaze-stack-protect.xml: New file.

>          * features/microblaze-with-stack-protect.c: New file.

>          * features/microblaze-with-stack-protect.xml: New file.

>          * features/microblaze.xml: New file.

>          * features/microblaze.c: New file.

>          * features/Makefile (microblaze-with-stack-protect): Add

>          microblaze-with-stack-protect microblaze and

>          microblaze-expedite.

>          * regformats/microblaze-with-stack-protect.dat: New file.

>          * regformats/microblaze.dat: New file.

>          * doc/gdb.texinfo (MicroBlaze Features): New.

>

>      Signed-off-by:Ajit Agarwal ajitkum@xilinx.com

>

>> In this case is it correct to say

>> If (tdesc  == NULL)

>>    tdesc = tdesc_microblaze;

>>

>> instead of tdesc_microblaze_with_stack_protect?

>

>>> Yes.

>

> Instead of tdesc_microblaze_with_stack_protect if I use tdesc_microblaze  the "G Packet message is too long" error is not resolved.

> The patch is unchanged with tdesc_microblaze_stack_protect.

>

> Thanks & Regards

> Ajit

> -----Original Message-----

> From: Pedro Alves [mailto:palves@redhat.com]

> Sent: Tuesday, June 24, 2014 6:17 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.

>

> On 06/24/2014 01:31 PM, Ajit Kumar Agarwal wrote:

>

>>>

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

>>

>>>> But you've already added the G packet size guess for that.

>>

>> In this case is it correct to say

>> If (tdesc  == NULL)

>>    tdesc = tdesc_microblaze;

>>

>> instead of tdesc_microblaze_with_stack_protect?

>

> Yes.

>

>>

>>>> -

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

>>

>> Right.

>

> Actually, not right...  This comment doesn't really appear to be correct:

>

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

>

>

> static struct type *

> microblaze_register_type (struct gdbarch *gdbarch, int regnum) {

>    if (regnum == MICROBLAZE_SP_REGNUM)

>      return builtin_type (gdbarch)->builtin_data_ptr;

>

>    if (regnum == MICROBLAZE_PC_REGNUM)

>      return builtin_type (gdbarch)->builtin_func_ptr;

>

>    return builtin_type (gdbarch)->builtin_int; }

>

> MICROBLAZE_SP_REGNUM and MICROBLAZE_PC_REGNUM clearly aren't builtin_int...

>

> Doesn't your patch change the output of "ptype $sp" and "ptype $pc" ?

>

> That points at something missing in the target description:

>

>> +<!DOCTYPE feature SYSTEM "gdb-target.dtd"> <feature 

>> +name="org.gnu.gdb.microblaze.core">

>> +  <reg name="r1" bitsize="32"/>

> ...

>> +  <reg name="rpc" bitsize="32"/>

>

> AFAICS, SP is "r1", and PC is "rpc".  These should be marked with type="data_ptr" and type="code_ptr" .

>

>>

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

>>

>>>> The GDB manual, gdb/doc/gdb.texinfo, describes all the standard XML target features.  See the "Standard Target Features" node, and add a new subsection for MicroBlaze.

>>

>> Thanks !! I will add subsection for Microblaze target.

>

> Thank you.

>

> --

> Pedro Alves

>



-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077
  

On 07/01/14 09:06, Ajit Kumar Agarwal wrote:
>
>
> -----Original Message-----
> From: Michael Eager [mailto:eager@eagercon.com]
> Sent: Monday, June 30, 2014 8:18 PM
> To: Ajit Kumar Agarwal; Pedro Alves
> Cc: gdb-patches@sourceware.org; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
> Subject: Re: [Patch, microblaze]: Fix for remote G Packet message too long error for baremetal.
>
> On 06/30/14 03:32, Ajit Kumar Agarwal wrote:
>> Based on the feedback, updated the patch with the review comments.
>
>>> How has this been tested?  Please be specific.
>
> The patch has been tested with in-house applications with the older and newer designs for tar remote localhost:1234( 1234 port number) command.

When I said "be specific" I meant to give me *all* of the
details of the environment.

Saying "older" and "newer" is much less than specific.
Saying "target remote ..." tells me absolutely nothing.

What target running which configurations built with
which versions of EDK?

Which versions of XMD were tested?
  

Hello Pedro:

Please find the updated patch. Incorporated review comments.

    [Patch, microblaze]: Fix for remote G Packet message too long error for baremetal.
    
    Prior to version MicroBlaze v8.10.a,EDK 13.1, XMD's gdbserver stub returned 57
    registers in response to GDB's G request. Starting with version MicroBlaze
    v8.10.a, EDK 13.1, XMD added the slr and shr register, for a count of 59
    registers. This patch adds these registers to the expected G response. This patch
    fixes the above problem for baremetal and also supports the backward compatibility.
    
    ChangeLog:
    2014-07-02  Ajit Agarwal  <ajitkum@xilinx.com>
    
        * microblaze-tdep.c (microblaze_register_names): Add
        the rshr and rslr register names.
        (microblaze_gdbarch_init): Use of tdesc_has_registers.
        Use of tdesc_find_feature. Use of tdesc_data_alloc.
        Use of tdesc_numbered_register. Use of
        microblaze_register_g_packet_guesses. Use of
        tdesc_use_registers. Use of set_gdbarch_register_type.
        (microblaze_register_g_packet_guesses): New.
        * microblaze-tdep.h (microblaze_reg_num): Add
        field MICROBLAZE_SLR_REGNUM MICROBLAZE_SHR_REGNUM
        MICROBLAZE_NUM_REGS and MICROBLAZE_NUM_CORE_REGS.
        (microblaze_frame_cache): Use of MICROBLAZE_NUM_REGS.
        * features/microblaze-core.xml: New file.
        * features/microblaze-stack-protect.xml: New file.
        * features/microblaze-with-stack-protect.c: New file.
        * features/microblaze-with-stack-protect.xml: New file.
        * features/microblaze.xml: New file.
        * features/microblaze.c: New file.
        * features/Makefile (microblaze-with-stack-protect): Add
        microblaze-with-stack-protect microblaze and
        microblaze-expedite.
        * regformats/microblaze-with-stack-protect.dat: New file.
        * regformats/microblaze.dat: New file.
        * doc/gdb.texinfo (MicroBlaze Features): New.
    
    Signed-off-by:Ajit Agarwal ajitkum@xilinx.com

>>If the G guess is triggering OK, and so GDB picks the description with the stack protect registers based on the G packet size, why would the default target description matter at all?  If it >>does, then something sounds broken.
>>What are you doing different from the other ports that use this mechanism?
>>Please debug this a bit further.

Thanks for the above suggestion. The G guesses were not triggering because of the following code.

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

I have changed the above code with

  register_remote_g_packet_guess (gdbarch,
                                  4 * MICROBLAZE_NUM_CORE_REGS,
                                  tdesc_microblaze);

  register_remote_g_packet_guess (gdbarch,
                                  4 * MICROBLAZE_NUM_REGS,
                                  tdesc_microblaze_with_stack_protect);

With this change the G Packet guesses are triggered and the 'info registers" were not showing stack protect registers for the design where stack protect registers are not supported.
With the design where the stack protect registers are supported "info registers" were showing the stack protect registers. The target remote command are working fine with and without stack protect registers designs. The default target description is also been set with tdesc_microblaze as you have suggested.

Thanks & Regards
Ajit
-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Monday, June 30, 2014 4:53 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.

On 06/30/2014 12:13 PM, Ajit Kumar Agarwal wrote:
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Monday, June 30, 2014 4:25 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.
> 
> On 06/30/2014 11:32 AM, Ajit Kumar Agarwal wrote:
>>     Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
>>
>>>> In this case is it correct to say
>>>> If (tdesc  == NULL)
>>>>   tdesc = tdesc_microblaze;
>>>>
>>>> instead of tdesc_microblaze_with_stack_protect?
>>>>>> Yes.
>> Instead of tdesc_microblaze_with_stack_protect if I use tdesc_microblaze  the "G Packet message is too long" error is not resolved.
> 
>>> Then it sounds like the G packet size guesses you're adding aren't actually triggering.  Why?
> 
> I have checked the guesses are actually triggering as it works fine 
> with backward compatibility with the Designs there is no stack-protect registers. For the Design that has the Stack protect register, it reports the message " G packet too long ".

If the G guess is triggering OK, and so GDB picks the description with the stack protect registers based on the G packet size, why would the default target description matter at all?  If it does, then something sounds broken.
What are you doing different from the other ports that use this mechanism?
Please debug this a bit further.

--
Pedro Alves
  

-----Original Message-----
From: Michael Eager [mailto:eager@eagercon.com] 

Sent: Tuesday, July 01, 2014 10:16 PM
To: Ajit Kumar Agarwal; Pedro Alves
Cc: gdb-patches@sourceware.org; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch, microblaze]: Fix for remote G Packet message too long error for baremetal.

On 07/01/14 09:06, Ajit Kumar Agarwal wrote:
>

>

> -----Original Message-----

> From: Michael Eager [mailto:eager@eagercon.com]

> Sent: Monday, June 30, 2014 8:18 PM

> To: Ajit Kumar Agarwal; Pedro Alves

> Cc: gdb-patches@sourceware.org; Vinod Kathail; Vidhumouli Hunsigida; 

> Nagaraju Mekala

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

>

> On 06/30/14 03:32, Ajit Kumar Agarwal wrote:

>> Based on the feedback, updated the patch with the review comments.

>

>>> How has this been tested?  Please be specific.

>

> The patch has been tested with in-house applications with the older and newer designs for tar remote localhost:1234( 1234 port number) command.


When I said "be specific" I meant to give me *all* of the details of the environment.

Saying "older" and "newer" is much less than specific.
Saying "target remote ..." tells me absolutely nothing.

>>What target running which configurations built with which versions of EDK?


Target is microblaze-xilinx-elf New version is EDK 13.3 and  New Microblaze version is 8.30.a( This supports the stack protect registers).

Target is microblaze-xilinx-elf old EDK version 12.4 and old microblaze version is 8.00b(This doesn't support the stack protect registers).  

>>Which versions of XMD were tested?


Xilinx EDK 14.7 is used for testing. The designs with different configuration as mentioned above are downloaded and tested using this XMD.

Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077
  

On 07/01/14 12:36, Ajit Kumar Agarwal wrote:
> Hello Pedro:
>
> Please find the updated patch. Incorporated review comments.
>
>      [Patch, microblaze]: Fix for remote G Packet message too long error for baremetal.
>
>      Prior to version MicroBlaze v8.10.a,EDK 13.1, XMD's gdbserver stub returned 57
>      registers in response to GDB's G request. Starting with version MicroBlaze
>      v8.10.a, EDK 13.1, XMD added the slr and shr register, for a count of 59
>      registers. This patch adds these registers to the expected G response. This patch
>      fixes the above problem for baremetal and also supports the backward compatibility.
>
>      ChangeLog:
>      2014-07-02  Ajit Agarwal  <ajitkum@xilinx.com>
>
>          * microblaze-tdep.c (microblaze_register_names): Add
>          the rshr and rslr register names.
>          (microblaze_gdbarch_init): Use of tdesc_has_registers.
>          Use of tdesc_find_feature. Use of tdesc_data_alloc.
>          Use of tdesc_numbered_register. Use of
>          microblaze_register_g_packet_guesses. Use of
>          tdesc_use_registers. Use of set_gdbarch_register_type.
>          (microblaze_register_g_packet_guesses): New.
>          * microblaze-tdep.h (microblaze_reg_num): Add
>          field MICROBLAZE_SLR_REGNUM MICROBLAZE_SHR_REGNUM
>          MICROBLAZE_NUM_REGS and MICROBLAZE_NUM_CORE_REGS.
>          (microblaze_frame_cache): Use of MICROBLAZE_NUM_REGS.
>          * features/microblaze-core.xml: New file.
>          * features/microblaze-stack-protect.xml: New file.
>          * features/microblaze-with-stack-protect.c: New file.
>          * features/microblaze-with-stack-protect.xml: New file.
>          * features/microblaze.xml: New file.
>          * features/microblaze.c: New file.
>          * features/Makefile (microblaze-with-stack-protect): Add
>          microblaze-with-stack-protect microblaze and
>          microblaze-expedite.
>          * regformats/microblaze-with-stack-protect.dat: New file.
>          * regformats/microblaze.dat: New file.
>          * doc/gdb.texinfo (MicroBlaze Features): New.
                                                     ^^^
                                          changed to Added.

Patch has trailing whitespace.

Committed 164224e9 without trailing whitespace.