This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.


[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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]