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/18/14 04:05, Ajit Kumar Agarwal wrote:

Based on the feedback incorporated the changes as mentioned below. Could you please
review the patch and let me know if its okay.


From a brief review, the patch looks OK.

There were a number of issues raised previously.  I haven't reviewed the
the comments to see if all have been addressed.

I'll let you know if there are any remaining issues.




    [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-18 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 and MICROBLAZE_SHR_REGNUM
         MICROBLAZE_NUM_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/Makefile (microblaze-linux): Add
         microblaze-linux and microblaze-expedite.
         * regformats/microblaze-with-stack-protect.dat: New file.

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

Thanks & Regards
Ajit

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com]
Sent: Tuesday, June 17, 2014 4:47 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/12/2014 09:56 AM, Ajit Kumar Agarwal wrote:

Based on the feedbacks, all changes have been incorporated.

May I know who will committing this patch to the upstream ?

[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
     fixes the above problem for baremetal and also supports the backward compatibility.

     ChangeLog:

     2014-06-12 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 and MICROBLAZE_SHR_REGNUM
         MICROBLAZE_NUM_REGS.
         (microblaze_frame_cache): Use of MICROBLAZE_NUM_REGS.

         * features/microblaze-cpu.xml: New file.

         * features/microblaze-linux.c: New file.

         * features/microblaze-linux.xml: New file.

         * regformats/reg-microblaze.dat: New file.

         * features/Makefile (microblaze-linux): Add microblaze-linux
         and microblaze-expedite.

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

Thanks & Regards
Ajit


0001-Patch-microblaze-Fix-for-remote-G-Packet-message-too.patch


 From ce1e6c1f1cd7d49df24727fc179df4c3f6f66bee Mon Sep 17 00:00:00 2001
From: Ajit Kumar Agarwal <ajitkum@xhdspdgnu.(none)>
Date: Thu, 12 Jun 2014 12:50:16 +0530
Subject: [PATCH] [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 fixes the above problem for baremetal and also supports the backward compatibility.


Yet, you've named the xml files "linux", and, made the default description assume GNU/Linux osabi.  That doesn't look right.



ChangeLog:

2014-06-12 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 and MICROBLAZE_SHR_REGNUM
	MICROBLAZE_NUM_REGS.
	(microblaze_frame_cache): Use of MICROBLAZE_NUM_REGS.

	* features/microblaze-cpu.xml: New file.

	* features/microblaze-linux.c: New file.

	* features/microblaze-linux.xml: New file.

	* regformats/reg-microblaze.dat: New file.

	* features/Makefile (microblaze-linux): Add microblaze-linux
	and microblaze-expedite.


No empty line between entries, please.






+++ b/gdb/features/microblaze-cpu.xml
@@ -0,0 +1,69 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2007-2014 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd"> <feature
+name="org.gnu.gdb.MicroBlaze.cpu">
+  <reg name="r0" bitsize="32" regnum="0"/>
+  <reg name="r1" bitsize="32"/>
+  <reg name="r2" bitsize="32"/>
+  <reg name="r3" bitsize="32"/>
+  <reg name="r4" bitsize="32"/>
+  <reg name="r5" bitsize="32"/>
+  <reg name="r6" bitsize="32"/>
+  <reg name="r7" bitsize="32"/>
+  <reg name="r8" bitsize="32"/>
+  <reg name="r9" bitsize="32"/>
+  <reg name="r10" bitsize="32"/>
+  <reg name="r11" bitsize="32"/>
+  <reg name="r12" bitsize="32"/>
+  <reg name="r13" bitsize="32"/>
+  <reg name="r14" bitsize="32"/>
+  <reg name="r15" bitsize="32"/>
+  <reg name="r16" bitsize="32"/>
+  <reg name="r17" bitsize="32"/>
+  <reg name="r18" bitsize="32"/>
+  <reg name="r19" bitsize="32"/>
+  <reg name="r20" bitsize="32"/>
+  <reg name="r21" bitsize="32"/>
+  <reg name="r22" bitsize="32"/>
+  <reg name="r23" bitsize="32"/>
+  <reg name="r24" bitsize="32"/>
+  <reg name="r25" bitsize="32"/>
+  <reg name="r26" bitsize="32"/>
+  <reg name="r27" bitsize="32"/>
+  <reg name="r28" bitsize="32"/>
+  <reg name="r29" bitsize="32"/>
+  <reg name="r30" bitsize="32"/>
+  <reg name="r31" bitsize="32"/>
+  <reg name="rpc" bitsize="32"/>
+  <reg name="rmsr" bitsize="32"/>
+  <reg name="rear" bitsize="32"/>
+  <reg name="resr" bitsize="32"/>
+  <reg name="rfsr" bitsize="32"/>
+  <reg name="rbtr" bitsize="32"/>
+  <reg name="rpvr0" bitsize="32"/>
+  <reg name="rpvr1" bitsize="32"/>
+  <reg name="rpvr2" bitsize="32"/>
+  <reg name="rpvr3" bitsize="32"/>
+  <reg name="rpvr4" bitsize="32"/>
+  <reg name="rpvr5" bitsize="32"/>
+  <reg name="rpvr6" bitsize="32"/>
+  <reg name="rpvr7" bitsize="32"/>
+  <reg name="rpvr8" bitsize="32"/>
+  <reg name="rpvr9" bitsize="32"/>
+  <reg name="rpvr10" bitsize="32"/>
+  <reg name="rpvr11" bitsize="32"/>
+  <reg name="redr" bitsize="32"/>
+  <reg name="rpid" bitsize="32"/>
+  <reg name="rzpr" bitsize="32"/>
+  <reg name="rtlbx" bitsize="32"/>
+  <reg name="rtlbsx" bitsize="32"/>
+  <reg name="rtlblo" bitsize="32"/>
+  <reg name="rtlbhi" bitsize="32"/>
+  <reg name="rslr" bitsize="32"/>
+  <reg name="rshr" bitsize="32"/>
+</feature>


I don't think this is right.  What you want to do, is create a "core" feature, that includes only the set of generic purpose registers that are always available in all microblaze configurations.  As discussed, rslr and rshr aren't of that kind.  Note that most ports call that feature "core":

  $ grep "org.gnu" * | grep cpu
  mips-tdep.c:                                "org.gnu.gdb.mips.cpu");
  nios2-tdep.c:      feature = tdesc_find_feature (tdesc, "org.gnu.gdb.nios2.cpu");

  $ grep "org.gnu" * | grep core
  aarch64-tdep.c:  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.core");
  arm-tdep.c:                                 "org.gnu.gdb.arm.core");
  i386-tdep.c:  feature_core = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.core");
  m68k-tdep.c:                                "org.gnu.gdb.m68k.core");
  m68k-tdep.c:                                    "org.gnu.gdb.coldfire.core");
  m68k-tdep.c:                                    "org.gnu.gdb.fido.core");
  rs6000-tdep.c:                              "org.gnu.gdb.power.core");
  s390-linux-tdep.c:      feature = tdesc_find_feature (tdesc, "org.gnu.gdb.s390.core");
  tic6x-tdep.c:      feature = tdesc_find_feature (tdesc, "org.gnu.gdb.tic6x.core");

Btw, note how feature names are all lowercase, so

  "org.gnu.gdb.microblaze"

would be more standard.

Then, you'll want to create another xml description feature for the C_USE_STACK_PROTECTION microblaze feature, like:

<!DOCTYPE feature SYSTEM "gdb-target.dtd"> <feature name="org.gnu.gdb.microblaze.stack-protect">
   <reg name="r0" bitsize="32" regnum="0"/>
   <reg name="rslr" bitsize="32"/>
   <reg name="rshr" bitsize="32"/>
</feature>

Finally, a machine that has the stack protection feature enabled can report a description that makes use of xi:include, like:

<!DOCTYPE target SYSTEM "gdb-target.dtd"> <target>
   <xi:include href="microblaze-core.xml"/>
   <xi:include href="microblaze-stack-protect.xml"/>
</target>

There are several examples of this in the features/ directory.
E.g., see arm-with-vfpv2.xml.


+static void
+microblaze_register_g_packet_guesses (struct gdbarch *gdbarch) {
+      register_remote_g_packet_guess (gdbarch,
+                                      MICROBLAZE_NUM_REGS,
+                                      tdesc_microblaze_linux);

+      register_remote_g_packet_guess (gdbarch,
+                                      MICROBLAZE_NUM_REGS-2,
+                                      tdesc_microblaze_linux);


Now here you'll want to guess different descriptions.  The second case should guess a description that doesn't include rslr and rshr at all.

Formatting isn't following the BTW, coding conventions.
Spaces around '-' missing and statements should indented with
2 spaces, not 6.

Thanks,
--
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]