This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 1/4] code changes for Nios II target, version 2
- From: Pedro Alves <palves at redhat dot com>
- To: Sandra Loosemore <sandra at codesourcery dot com>
- Cc: Joel Brobecker <brobecker at adacore dot com>, gdb-patches at sourceware dot org
- Date: Fri, 03 May 2013 19:09:05 +0100
- Subject: Re: [patch 1/4] code changes for Nios II target, version 2
- References: <517481E7 dot 7030608 at codesourcery dot com> <20130426063617 dot GO3525 at adacore dot com> <51833147 dot 8090206 at codesourcery dot com>
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