diff mbox

[microblaze] : Add support of microblaze software single stepping

Message ID a0a86472-2b0f-4a69-bd7d-ddea295e0f5e@BN1BFFO11FD011.protection.gbl
State New, archived
Headers

Commit Message

Ajit Kumar Agarwal July 6, 2014, 6:13 a.m. UTC 
  PING!

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

 
+  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

Comments

Joel Brobecker July 7, 2014, 2:56 p.m. UTC | #1 
> 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
Ajit Kumar Agarwal July 11, 2014, 12:34 p.m. UTC | #2 
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
Joel Brobecker July 11, 2014, 1:51 p.m. UTC | #3 
> 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,
Ajit Kumar Agarwal July 11, 2014, 2:05 p.m. UTC | #4 
Hello Joel:

I will incorporate the formatting  changes as you have mentioned.

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

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.

> +/* 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
Ajit Kumar Agarwal July 11, 2014, 7:34 p.m. UTC | #5 
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
Michael Eager July 11, 2014, 7:57 p.m. UTC | #6 
On 07/11/14 12:34, Ajit Kumar Agarwal wrote:

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

Your terminology may be a bit confusing.  Gdbserver
is a program (part of GDB) which runs on a platform (e.g.
Linux), and which implements the GDB Remote Serial Protocol.
XMD does not run gdbserver.  It has an independent implementation
of the GDB Remote Serial Protocol.  It would be better to
say that XMD is a remote stub.

Please provide a disassembly of the test case and
a log of the commands to gdb and gdb's output, showing
stepping over imm and delay slot instructions.

Is there a test case which shows single stepping failing
without the patch?

Even better would be to update the gdb regression
test suite with the test case.
Ajit Kumar Agarwal July 14, 2014, 10:27 a.m. UTC | #7 
Here are the dump of the disassembly and the output of single step command for branch with delay slot and imm instructions.

>>Please provide a disassembly of the test case and a log of the commands to gdb and gdb's output, showing stepping over imm and delay slot instructions


BRANCH WITH DELAY SLOT

Breakpoint 1, main () at ../src/helloworld.c:28
28	    init_platform();
(gdb) display /i $pc
1: x/i $pc
=> 0xc00001b0 <main+16>:	brlid	r15, 172	// 0xc000025c
(gdb) si
init_platform () at ../src/platform.c:53
53	{
1: x/i $pc
=> 0xc000025c <init_platform>:	addik	r1, r1, -32

Disassembly of main :

(gdb) disassem main
Dump of assembler code for function main:
   0xc00001a0 <+0>:	addik	r1, r1, -32
   0xc00001a4 <+4>:	swi	r15, r1, 0
   0xc00001a8 <+8>:	swi	r19, r1, 28
   0xc00001ac <+12>:	addk	r19, r1, r0
   0xc00001b0 <+16>:	brlid	r15, 172	// 0xc000025c
   0xc00001b4 <+20>:	or	r0, r0, r0
   0xc00001b8 <+24>:	imm	-16384
   0xc00001bc <+28>:	addik	r5, r0, 2548
   0xc00001c0 <+32>:	brlid	r15, 252	// 0xc00002bc
   0xc00001c4 <+36>:	or	r0, r0, r0
   0xc00001c8 <+40>:	brlid	r15, 196	// 0xc000028c
   0xc00001cc <+44>:	or	r0, r0, r0
   0xc00001d0 <+48>:	addk	r3, r0, r0
   0xc00001d4 <+52>:	lwi	r15, r1, 0
   0xc00001d8 <+56>:	addk	r1, r19, r0
   0xc00001dc <+60>:	lwi	r19, r1, 28
   0xc00001e0 <+64>:	addik	r1, r1, 32
   0xc00001e4 <+68>:	rtsd	r15, 8
   0xc00001e8 <+72>:	or	r0, r0, r0

WITH IMM INSTRUCTION:

(gdb) si
main () at ../src/helloworld.c:30
30	    print("Hello World\n\r");
2: x/i $pc
=> 0xc00001b8 <main+24>:	imm	-16384
1: x/i $pc
=> 0xc00001b8 <main+24>:	imm	-16384
 (gdb) si
0xc00001c0	30	    print("Hello World\n\r");
2: x/i $pc
=> 0xc00001c0 <main+32>:	brlid	r15, 252	// 0xc00002bc
1: x/i $pc
=> 0xc00001c0 <main+32>:	brlid	r15, 252	// 0xc00002bc

>>Is there a test case which shows single stepping failing without the patch?


The same disassembly with <main> function given above 
Without Single Stepping implementation there is a warning with the "warning: Remote failure reply: E01"  with branch with delay slot.

(gdb) display /i $pc
1: x/i $pc
=> 0xc00001b0 <main+16>:	brlid	r15, 172	// 0xc000025c
(gdb) si
warning: Remote failure reply: E01

[Thread <main>] #1 stopped.
init_platform () at ../src/platform.c:53
53	{
1: x/i $pc
=> 0xc000025c <init_platform>:	addik	r1, r1, -32

WITH IMM instruction : There is a same warning without the single stepping patch  "warning:Remote failure reply : E01".
 

[Thread <main>] #1 stopped.
main () at ../src/helloworld.c:30
30	    print("Hello World\n\r");
1: x/i $pc
=> 0xc00001b8 <main+24>:	imm	-16384
(gdb) 
warning: Remote failure reply: E01

[Thread <main>] #1 stopped.
0xc00001c0 in main () at ../src/helloworld.c:30
30	    print("Hello World\n\r");

Thanks & Regards
Ajit
-----Original Message-----
From: Michael Eager [mailto:eager@eagercon.com] 

Sent: Saturday, July 12, 2014 1:28 AM
To: Ajit Kumar Agarwal; Joel Brobecker
Cc: gdb-patches@sourceware.org; Pedro Alves; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch, microblaze]: Add support of microblaze software single stepping

On 07/11/14 12:34, Ajit Kumar Agarwal wrote:

>

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


Your terminology may be a bit confusing.  Gdbserver is a program (part of GDB) which runs on a platform (e.g.
Linux), and which implements the GDB Remote Serial Protocol.
XMD does not run gdbserver.  It has an independent implementation of the GDB Remote Serial Protocol.  It would be better to say that XMD is a remote stub.

Please provide a disassembly of the test case and a log of the commands to gdb and gdb's output, showing stepping over imm and delay slot instructions.

Is there a test case which shows single stepping failing without the patch?

Even better would be to update the gdb regression test suite with the test case.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077
Ajit Kumar Agarwal July 20, 2014, 12:57 p.m. UTC | #8 
PING!!

-----Original Message-----
From: Ajit Kumar Agarwal 
Sent: Saturday, July 12, 2014 1:04 AM
To: 'Joel Brobecker'
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

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
Michael Eager Sept. 15, 2014, 4:06 p.m. UTC | #9 
On 07/14/14 03:27, Ajit Kumar Agarwal wrote:
> Here are the dump of the disassembly and the output of single step command for branch with delay slot and imm instructions.

Once again, please do not top post.

>>> Please provide a disassembly of the test case and a log of the commands to gdb and gdb's output, showing stepping over imm and delay slot instructions
>
> BRANCH WITH DELAY SLOT
>
> Breakpoint 1, main () at ../src/helloworld.c:28
> 28	    init_platform();
> (gdb) display /i $pc
> 1: x/i $pc
> => 0xc00001b0 <main+16>:	brlid	r15, 172	// 0xc000025c
> (gdb) si
> init_platform () at ../src/platform.c:53
> 53	{
> 1: x/i $pc
> => 0xc000025c <init_platform>:	addik	r1, r1, -32

I see the same behavior on a bare-metal MicroBlaze target without
your patch.

> The same disassembly with <main> function given above
> Without Single Stepping implementation there is a warning with the "warning: Remote failure reply: E01"  with branch with delay slot.
>
> (gdb) display /i $pc
> 1: x/i $pc
> => 0xc00001b0 <main+16>:	brlid	r15, 172	// 0xc000025c
> (gdb) si
> warning: Remote failure reply: E01
>
> [Thread <main>] #1 stopped.
> init_platform () at ../src/platform.c:53
> 53	{
> 1: x/i $pc
> => 0xc000025c <init_platform>:	addik	r1, r1, -32
>
> WITH IMM instruction : There is a same warning without the single stepping patch  "warning:Remote failure reply : E01".

I'm not able to reproduce this error on a bare-metal system
either big-endian or little-endian.  The test case which
you sent me offline does not show this error.  Single-stepping
on MicroBlaze appears to work correctly.

It appears that the "passing" test you show above was run on a
bare-metal system, while the "failing" test was run on a multi-threaded
system, presumably Linux using gdbserver.  Perhaps the error you are
seeing is in gdbserver.
diff mbox

Patch

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