Re: [PATCH v2] AndesTech NDS32 port

[Kevin Buettner <kevinb@redhat.com>]

Hi Yan-Ting Lin,

Please submit a separate patch in which you add yourself to the "Write
After Approval" section in gdb/MAINTAINERS.

See my comments below regarding some things that I noticed while
looking over your patch.  Anything that I didn't quote looks okay
to me.

Kevin

On Thu, 5 May 2016 15:16:30 +0800
Yan-Ting Lin <currygt52@gmail.com> wrote:

> diff --git a/gdb/features/nds32-core.xml b/gdb/features/nds32-core.xml
> new file mode 100644
> index 0000000..c98d91e
> --- /dev/null
> +++ b/gdb/features/nds32-core.xml
> @@ -0,0 +1,44 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2012-2016 Free Software Foundation, Inc.

Given that this is a new file, I suspect that the starting copyright
year should not be 2012.  Should it be 2016 only?

> diff --git a/gdb/features/nds32-fpu.xml b/gdb/features/nds32-fpu.xml
> new file mode 100644
> index 0000000..0c60ca3
> --- /dev/null
> +++ b/gdb/features/nds32-fpu.xml
> @@ -0,0 +1,42 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2012-2016 Free Software Foundation, Inc.

Ditto.

> diff --git a/gdb/features/nds32-system.xml b/gdb/features/nds32-system.xml
> new file mode 100644
> index 0000000..b772dac
> --- /dev/null
> +++ b/gdb/features/nds32-system.xml
> @@ -0,0 +1,14 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2012-2016 Free Software Foundation, Inc.

Ditto.

> diff --git a/gdb/features/nds32.xml b/gdb/features/nds32.xml
> new file mode 100644
> index 0000000..a728c3e
> --- /dev/null
> +++ b/gdb/features/nds32.xml
> @@ -0,0 +1,14 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2012-2016 Free Software Foundation, Inc.

Ditto.

> diff --git a/gdb/nds32-tdep.c b/gdb/nds32-tdep.c
> new file mode 100644
> index 0000000..a8add5e
> --- /dev/null
> +++ b/gdb/nds32-tdep.c
> @@ -0,0 +1,2191 @@
> +/* Target-dependent code for the NDS32 architecture, for GDB.
> +
> +   Copyright (C) 2012-2016 Free Software Foundation, Inc.
> +   Contributed by Andes Technology Corporation.

Ditto for this copyright too.  (On the other hand, if it was actually
published in some form back in any form in 2012, disregard my comments
regarding copyright.)

> +struct nds32_frame_cache
> +{
> +  /* The previous frame's inner most stack address.  Used as this
> +     frame ID's stack_addr.  */
> +  CORE_ADDR prev_sp;
> +
> +  /* The frame's base, optionally used by the high-level debug info.  */
> +  CORE_ADDR base;
> +
> +  /* During prologue analysis, keep how far the SP and FP have been offset
> +     from the start of the stack frame (as defined by the previous frame's
> +     stack pointer).
> +     During epilogue analysis, keep how far the SP has been offset from the
> +     current stack pointer.  */
> +  CORE_ADDR sp_offset;
> +  CORE_ADDR fp_offset;
> +  int use_frame;

Perhaps add a comment for use_frame?

> +
> +  /* The address of the first instruction in this function.  */
> +  CORE_ADDR pc;
> +
> +  /* Saved registers.  */
> +  CORE_ADDR saved_regs[NDS32_NUM_SAVED_REGS];
> +};
> +

Please add a comment for nds32_alloc_frame_cache(), below.

> +static struct nds32_frame_cache *
> +nds32_alloc_frame_cache (void)
> +{
> +  struct nds32_frame_cache *cache;
> +  int i;
...

Please add comment for nds32_analyze_epilogue_isns16, below.

> +static inline int
> +nds32_analyze_epilogue_insn16 (uint32_t insn, struct nds32_frame_cache *cache)
> +{
> +  if (insn == N16_TYPE5 (RET5, REG_LP))
> +    /* ret5 $lp */
> +    return INSN_RETURN;
> +  else if (CHOP_BITS (insn, 10) == N16_TYPE10 (ADDI10S, 0))

I'll take this opportunity to compliment you on your instruction
decoding.  It's very clean.

All in all, this port looks really good to me, aside from the nits noted
earlier.

Kevin