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

[Ajit Kumar Agarwal <ajit dot kumar dot agarwal at xilinx dot com>]

Based on the feedback review comments are incorporated. 

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

Thanks & Regards
Ajit

-----Original Message-----
From: Joel Brobecker [mailto:brobecker@adacore.com] 
Sent: Monday, July 07, 2014 8:27 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

> PING!

A quick look at the patch shows that the code does not conform to the GNU Coding style:
  - '{' brace at start of function should be on next line,
  - Sentences in comments should start with a capital letter and
    end with a period
  - etc.
Also, GDB Coding Style, which is a superset of the GCS, require that every single new function have proper documentation a the start of them.

You can refer to:
https://sourceware.org/gdb/wiki/Internals%20Coding-Standards

Last but not least, it looks like some lines got joined together, putting multiple statements on the same line.

> 
> -----Original Message-----
> From: Ajit Kumar Agarwal
> Sent: Friday, June 20, 2014 11:40 AM
> To: 'gdb-patches@sourceware.org'
> Cc: 'Michael Eager'; 'Pedro Alves'; Vinod Kathail; Vidhumouli 
> Hunsigida; Nagaraju Mekala
> Subject: RE: [Patch, microblaze]: Add support of microblaze software 
> single stepping
> 
> Please find the patch that supports the microblaze software single stepping. This patch handles the cases of branch and return with delay slot
> and the imm instruction in microblaze. Could you please review and let me know if its 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-06-19 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 |   79 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 79 insertions(+), 0 deletions(-)
> 
> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c index 
> 14c1b52..3de2f70 100644
> --- a/gdb/microblaze-tdep.c
> +++ b/gdb/microblaze-tdep.c
> @@ -628,6 +628,83 @@ microblaze_stabs_argument_has_addr (struct gdbarch *gdbarch, struct type *type)
>    return (TYPE_LENGTH (type) == 16);
>  }
>  
> +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;
> +
> +  /* 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  */  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 +785,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
> 
> Thanks & Regards
> Ajit

--
Joel

Attachment: 0001-Patch-microblaze-Add-support-of-microblaze-software-.patch
Description: 0001-Patch-microblaze-Add-support-of-microblaze-software-.patch