RE: [Patch, microblaze]: Add support of microblaze software single stepping

[Ajit Kumar Agarwal <ajit.kumar.agarwal@xilinx.com>]

[-- Attachment #1: Type: text/plain, Size: 5151 bytes --]

Hello Joel:

Please find the patch with the updated review comments. Please let me know if the changes looks okay.

[Patch, microblaze]: Add support of microblaze software single stepping.

This patch adds the support of microblaze software single stepping. It
handles the cases of branch and return with delay slot and imm instruction
in microblaze.

ChangeLog:
2014-07-12  Ajit Agarwal  <ajitkum@xilinx.com>

        * microblaze-tdep.c (microblaze_software_single_step): New.
        (microblaze_gdbarch_init): Use of set_gdbarch_software_single_step.

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

>>Please also explain how this patch was tested.

The changes were tested with the application for barematel with remote debug with XMD debugger( internal to Xilinx)which connects to the target and opens the gdbserver connection. Single stepping command were used for next_pc in straight line code, with imm instruction and the branch with delay Slot.

Thanks & Regards
Ajit
-----Original Message-----
From: Joel Brobecker [mailto:brobecker@adacore.com] 
Sent: Friday, July 11, 2014 7:21 PM
To: Ajit Kumar Agarwal
Cc: gdb-patches@sourceware.org; Michael Eager; Pedro Alves; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch, microblaze]: Add support of microblaze software single stepping

> Based on the feedback review comments are incorporated. 
> ChangeLog:
> 2014-07-11  Ajit Agarwal  <ajitkum@xilinx.com>
> 
> 	* microblaze-tdep.c (microblaze_software_single_step): New.
> 	(microblaze_gdbarch_init): Use of set_gdbarch_software_single_step.

Thank you. Some additional trivial coments below.

Please also explain how this patch was tested.

> +/* This function handles software single step, branches with delay slot
> +   imm instruction in microblaze.  */ static int 
> +microblaze_software_single_step (struct frame_info *frame)

Two comments, in this case: My first comment is that the GDB coding style requires a space after the comment describing the function, before the function itself starts.

The second is that functions meant to be installed as gdbarch hooks should be documented as such. The theory is that the purpose of these functions, as well as their API, is documented in the gdbarch code, and thus does not need to be repeated here. So, the typical function description for these functions is:

/* Implement the software_single_step gdbarch method.  */

If you feel that the extra information you had above is useful, just add it after the introductory comment, like so, for instance:

/* Implement the software_single_step gdbarch method.

   This function handles software single step, branches with delay slot
   imm instruction in microblaze.  */

> +  /* Set a breakpoint at the next instruction. If the current
> +     instruction is an imm, set it at the inst after. If the
> +     instruction has a delay slot, skip the delay slot.  */

This comment could also be merged with the comment above.  Or, if you prefer, the extra info from the function documentation could also be merged with this comment.

Formatting note: 2 spaces after periods.

> +  pc = get_frame_pc (frame);
> + 
> +  insn = microblaze_fetch_instruction (pc);
> +
> +  minstr = get_insn_microblaze (insn,
> +                                &isunsignednum,
> +                                &insn_type,
> +                                &delay_slots);

I will leave it to you to decide, but we typically do not write function calls with one argument per line, unless perhaps when the arguments are function calls, where having one per line would help the reader find which arguments belong to which function call. Particularly in the case below....

> +      minstr = microblaze_decode_insn (insn,
> +                                       &rd,
> +                                       &ra,
> +                                       &rb,
> +                                       &imm);

... joining all arguments on the same line would save you a lot of screen real estate.

> +  if (insn_type != return_inst)
> +    breaks[0] = pc + delay_slots * INST_WORD_SIZE + INST_WORD_SIZE;
> +    

Trailing spaces here.

> +      bfd_boolean targetvalid;
> +      bfd_boolean unconditionalbranch;
> + 

And here.

> +      if (lrb >= 0 && lrb < MICROBLAZE_NUM_REGS)
> +        rb = get_frame_register_unsigned (frame, lrb);
> +      else
> +        rb = 0;
> + 

And here.

> +      address = microblaze_get_target_address (insn,
> +                                               immfound,
> +                                               imm,
> +                                               pc,
> +                                               ra,
> +                                               rb,
> +                                               &targetvalid,
> +                                               &unconditionalbranch);
> +        
> +      if (!unconditionalbranch)
> +        breaks[1] = address;
> +    }
> +    

And there (3 lines have trailing spaces)

Thank you,
--
Joel

[-- Attachment #2: 0001-Patch-microblaze-Add-support-of-microblaze-software-.patch --]
[-- Type: application/octet-stream, Size: 4306 bytes --]

From 21c0c9581faba72b98061c46ad06e58a78dd6b2d Mon Sep 17 00:00:00 2001
From: Ajit Kumar Agarwal <ajitkum@xhdspdgnu.(none)>
Date: Sat, 12 Jul 2014 00:53:35 +0530
Subject: [PATCH] [Patch, microblaze]: Add support of microblaze software single stepping.

This patch adds the support of microblaze software single stepping. It
handles the cases of branch and return with delay slot and imm instruction
in microblaze.

ChangeLog:
2014-07-12  Ajit Agarwal  <ajitkum@xilinx.com>

	* microblaze-tdep.c (microblaze_software_single_step): New.
	(microblaze_gdbarch_init): Use of set_gdbarch_software_single_step.

Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
---
 gdb/microblaze-tdep.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
index 14c1b52..a1f29d6 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -628,6 +628,91 @@ microblaze_stabs_argument_has_addr (struct gdbarch *gdbarch, struct type *type)
   return (TYPE_LENGTH (type) == 16);
 }
 
+/* Implement the software_single_step gdbarch method.
+   This function handles software single step, branches with delay slot
+   imm instruction in microblaze.  Set a breakpoint at the next instruction.
+   If the current instruction is an imm, set it at the inst after.  If the
+   instruction has a delay slot, skip the delay slot.  */
+
+static int
+microblaze_software_single_step (struct frame_info *frame)
+{
+  struct gdbarch *arch = get_frame_arch (frame);
+  struct address_space *aspace = get_frame_address_space (frame);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (arch);
+  enum bfd_endian byte_order = gdbarch_byte_order (arch);
+  int ret = 0;
+  int ii;
+  CORE_ADDR pc;
+  long insn;
+  enum microblaze_instr minstr;
+  bfd_boolean isunsignednum;
+  enum microblaze_instr_type insn_type;
+  short delay_slots;
+  int imm;
+  bfd_boolean immfound = FALSE;
+  CORE_ADDR breaks[2] = {-1,-1};
+  CORE_ADDR address;
+  int targetvalid;
+
+  pc = get_frame_pc (frame);
+ 
+  insn = microblaze_fetch_instruction (pc);
+
+  minstr = get_insn_microblaze (insn, &isunsignednum, &insn_type, &delay_slots);
+
+  if (insn_type == immediate_inst)
+    {
+      int rd, ra, rb;
+
+      immfound = TRUE;
+      minstr = microblaze_decode_insn (insn, &rd, &ra, &rb, &imm);
+      pc = pc + INST_WORD_SIZE;
+      insn = microblaze_fetch_instruction (pc);
+      minstr = get_insn_microblaze (insn, &isunsignednum, &insn_type, &delay_slots);
+    }
+ 
+  if (insn_type != return_inst)
+    breaks[0] = pc + delay_slots * INST_WORD_SIZE + INST_WORD_SIZE;
+  /* Now check for branch or return instructions.  */
+  if (insn_type == branch_inst || insn_type == return_inst)
+    {
+      int limm;
+      int lrd, lra, lrb;
+      int ra, rb;
+      bfd_boolean targetvalid;
+      bfd_boolean unconditionalbranch;
+      microblaze_decode_insn (insn, &lrd, &lra, &lrb, &limm);
+      if (lra >= 0 && lra < MICROBLAZE_NUM_REGS)
+        ra = get_frame_register_unsigned (frame, lra);
+      else
+        ra = 0;
+      if (lrb >= 0 && lrb < MICROBLAZE_NUM_REGS)
+        rb = get_frame_register_unsigned (frame, lrb);
+      else
+        rb = 0;
+      address = microblaze_get_target_address (insn, immfound, imm,
+                                               pc, ra, rb,
+                                               &targetvalid,
+                                               &unconditionalbranch);
+      if (!unconditionalbranch)
+        breaks[1] = address;
+    }
+  /* Insert the breakpoints.  */
+  if (breaks[0] != -1)
+    {
+      insert_single_step_breakpoint (arch, aspace, breaks[0]);
+      ret = 1;
+    }
+  if (breaks[1] != -1)
+    {
+      insert_single_step_breakpoint (arch, aspace, breaks[1]);
+      ret = 1;
+    }
+
+  return ret;
+}
+ 
 static void
 microblaze_write_pc (struct regcache *regcache, CORE_ADDR pc)
 {
@@ -708,6 +793,8 @@ microblaze_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   set_gdbarch_breakpoint_from_pc (gdbarch, microblaze_breakpoint_from_pc);
 
+  set_gdbarch_software_single_step (gdbarch, microblaze_software_single_step);
+
   set_gdbarch_frame_args_skip (gdbarch, 8);
 
   set_gdbarch_print_insn (gdbarch, print_insn_microblaze);
-- 
1.7.1