This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch 1/4] code changes for Nios II target, version 2


Hi Sandra,

On 05/03/2013 04:38 AM, Sandra Loosemore wrote:

> I realized that there was some inconsistency in the code about expectations for register numbering, no doubt as a result of being hacked on by different people at different times.  In particular, the list of expected registers in the target description included only a subset of the NIOS2_NUM_REGS registers, so the additional control registers listed in the provided features/nios2-cpu.xml file were being assigned numbers >= NIOS2_NUM_REGS, with a gap in the numbering.  Other places in the code were clearly assuming the NIOS2_NUM_REGS registers were numbered consecutively.
> 
> I've cleaned this up by having nios2_gdbarch_init enumerate the full set of NIOS2_NUM_REGS registers when checking the target description.  (The additional control registers are documented in the processor manual and it's reasonable to expect that they will always be there.)

An alternative, if the extra registers aren't really part of the core set,
and might be different on different part numbers, would be to add them
in a separate xml target feature, leaving only the core registers in the core
feature.  But I leave that up to you to decide, of course.

> I also merged the redundant arrays of register names and fixed the xml file to use the same canonical names that nios2_register_name uses for printing.  With the additional checking enabled, I discovered a typo in the xml file in the name of the "pteaddr" control register, which is fixed here.
> 
> Is this version OK to commit?

It looks pretty good to me too.

The new new xml target features need to be documented in
the manual though, as well as the new command(s).

A NEWS entry is necessary as well.

A few minor nits below.

> Index: gdb/nios2-tdep.h
> ===================================================================
> RCS file: gdb/nios2-tdep.h
> diff -N gdb/nios2-tdep.h
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/nios2-tdep.h	2 May 2013 22:56:39 -0000
> @@ -0,0 +1,81 @@

> +/* Registers.  */
> +#define SP_REGNUM 27  /* Stack Pointer */
> +#define FP_REGNUM 28  /* Frame Pointer */
> +#define PC_REGNUM 32

These should be prefixed with NIOS2_ as well.

> +/* The following structures are used in the cache for prologue
> +   analysis; see the reg_value and reg_saved tables in
> +   struct nios2_unwind_cache, respectively.  */
> +
> +/* REG_VALUE is used to record that a register has the same value
> +   as reg at the given offset from the start of a function.  */
> +

> +typedef struct
> +{
...
> +} REG_VALUE;
> +

> +typedef struct
> +{
...
> +} REG_SAVED;

Any special reason these two use the typedef style, and uppercase?
If not, can we drop the typedef, and down case them?  They read as
a macro, as is.

> +static const wild_insn profiler_insn[] =
> +{
> +  { 0x0010e03a, 0x00000000 }, /* nextpc r8 */
> +  { 0xf813883a, 0x00000000 }, /* mov    r9,ra */
> +  { 0x02800034, 0x003FFFC0 }, /* movhi  r10,257 */
> +  { 0x52800004, 0x003FFFC0 }, /* addi   r10,r10,-31992 */
> +  { 0x00000000, 0xFFFFFFC0 }, /* call   <mcount> */
> +  { 0x483f883a, 0x00000000 }  /* mov    ra,r9 */

There's a inconsistency in the patch over the use of uppercase
vs lowercase in the hex constants.  Can we make them all lowercase,
please?


> +  /* Find the prologue instructions.  Fortunately we're in 32bit
> +     paradise.  */

Not exactly sure what this is referring to, but note we can use
uint32_t now.


> +  /* If this_frame is NULL, we are being called from skip_prologue

THIS_FRAME

> +     and are only interested in the prologue_end value, so just

PROLOGUE_END

> +     return that now and skip over the cache updates, which depend
> +     on having frame information.  */

> +/* Implement the breakpoint_from_pc gdbarch hook.  */
> +
> +static const unsigned char*

gdb_byte

> +nios2_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *bp_addr,
> +			  int *bp_size)
> +{
> +  /* break encoding: 31->27  26->22  21->17  16->11 10->6 5->0 */
> +  /*                 00000   00000   0x1d    0x2d   11111 0x3a */
> +  /*                 00000   00000   11101   101101 11111 111010 */
> +  /* In bytes:       00000000 00111011 01101111 11111010 */
> +  /*                 0x0       0x3b    0x6f     0xfa */
> +  static gdb_byte breakpoint_le[] = {0xfa, 0x6f, 0x3b, 0x0};
> +  static gdb_byte breakpoint_be[] = {0x0, 0x3b, 0x6f, 0xfa};

static const

> +
> +  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
> +
> +  *bp_size = 4;
> +  if (gdbarch_byte_order_for_code (gdbarch) == BFD_ENDIAN_BIG)
> +    return breakpoint_be;
> +  else
> +    return breakpoint_le;
> +}
> +

> +/* Implement the unwind_pc gdbarch method.  */
> +
> +static CORE_ADDR
> +nios2_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame)
> +{
> +  gdb_byte buf[8];

buf[4] ?

-- 
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]