Re: [Patch, microblaze]: Add slr and shr regs and little-endian breakpoint

[Pedro Alves <palves@redhat.com>]

On 05/21/2014 02:40 PM, Ajit Kumar Agarwal wrote:
> Based on the feedback I have splitted the patch and the following is the patch for Add slr and shr regs support.    
> 
> [Patch, microblaze]: Add slr and shr regs
>     
>     ChangeLog:
>     2014-05-20 Ajit Agarwal <ajitkum@xilinx.com>
>     
>         * gdb/gdbserver/Makefile.in (microblaze-linux.c): New rule.
>     
>         * gdb/microblaze-tdep.c (microblaze_register_names): Added
>         the rshr and rslr register names.
>     
>         * gdb/microblaze-tdep.h (microblaze_reg_num): Addition of
>         field MICROBLAZE_SLR_REGNUM and MICROBLAZE_SHR_REGNUM.
>         (microblaze_frame_cache): Change in the index of
>         register_offsets.
>     
>         * gdb/regformats/reg-microblaze.dat: New Register data file.
>     
>     Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
> 
> Thanks & Regards

> ChangeLog:
> 2014-05-20 Ajit Agarwal <ajitkum@xilinx.com>
> 
> 	* gdb/gdbserver/Makefile.in (microblaze-linux.c): New rule.

gdb and gdbserver each have their own ChangeLog files.  Filenames
in entries should be relative to the corresponding ChangeLog file.

So:

 	* Makefile.in (microblaze-linux.c): New rule.

But looks like microblaze-linux.c is missing from the patch?

> 
> 	* gdb/microblaze-tdep.c (microblaze_register_names): Added
> 	the rshr and rslr register names.

And:
 	* microblaze-tdep.c (microblaze_register_names): Added
 	the rshr and rslr register names.

But use present tense "Add".

> 
> 	* gdb/microblaze-tdep.h (microblaze_reg_num): Addition of
> 	field MICROBLAZE_SLR_REGNUM and MICROBLAZE_SHR_REGNUM.

	* microblaze-tdep.h (microblaze_reg_num): Add
	MICROBLAZE_SLR_REGNUM and MICROBLAZE_SHR_REGNUM.


> 	(microblaze_frame_cache): Change in the index of
> 	register_offsets.

What change?

> 
> 	* gdb/regformats/reg-microblaze.dat: New Register data file.

What for upper/lower case mixups.  "New file." is sufficient though:

 	* regformats/reg-microblaze.dat: New file.

>    /* Offsets to saved registers.  */
> -  int register_offsets[57];	/* Must match MICROBLAZE_NUM_REGS.  */
> +  int register_offsets[59];	/* Must match MICROBLAZE_NUM_REGS.  */

How about we let the compiler worry about this?

Define MICROBLAZE_NUM_REGS like:

enum microblaze_regnum
...
  MICROBLAZE_RTLBX_REGNUM,
  MICROBLAZE_RTLBSX_REGNUM,
  MICROBLAZE_RTLBLO_REGNUM,
  MICROBLAZE_RTLBHI_REGNUM
  MICROBLAZE_RTLBHI_REGNUM,
  MICROBLAZE_SLR_REGNUM,
  MICROBLAZE_SHR_REGNUM
+
+  MICROBLAZE_NUM_REGS

And then you can write:

  int register_offsets[MICROBLAZE_NUM_REGS];

But, most importantly, I've no clue what these slr and
shr registers are, but, do all microblaze cpus have those?

-- 
Pedro Alves