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

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

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