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

[Joel Brobecker <brobecker at adacore dot com>]

> 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