From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gdb-patches-return-131376-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Received: (qmail 58870 invoked by alias); 5 May 2016 23:46:50 -0000
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <gdb-patches.sourceware.org>
List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org>
List-Archive: <http://sourceware.org/ml/gdb-patches/>
List-Post: <mailto:gdb-patches@sourceware.org>
List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs>
Sender: gdb-patches-owner@sourceware.org
Received: (qmail 58810 invoked by uid 89); 5 May 2016 23:46:46 -0000
Authentication-Results: sourceware.org; auth=none
X-Virus-Found: No
X-Spam-SWARE-Status: No, score=-4.0 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=compliment, Approval, corporation
X-HELO: mx1.redhat.com
Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 05 May 2016 23:46:44 +0000
Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22])	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))	(No client certificate requested)	by mx1.redhat.com (Postfix) with ESMTPS id 8958C62648	for <gdb-patches@sourceware.org>; Thu,  5 May 2016 23:46:43 +0000 (UTC)
Received: from pinnacle.lan (ovpn-113-39.phx2.redhat.com [10.3.113.39])	by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u45Nkge2005485	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA256 bits=256 verify=NO)	for <gdb-patches@sourceware.org>; Thu, 5 May 2016 19:46:43 -0400
Date: Thu, 05 May 2016 23:46:00 -0000
From: Kevin Buettner <kevinb@redhat.com>
To: gdb-patches@sourceware.org
Subject: Re: [PATCH v2] AndesTech NDS32 port
Message-ID: <20160505164641.3cac5f17@pinnacle.lan>
In-Reply-To: <3561f390-ea41-2ee3-a5a9-d870429497c3@gmail.com>
References: <3561f390-ea41-2ee3-a5a9-d870429497c3@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
X-IsSubscribed: yes
X-SW-Source: 2016-05/txt/msg00082.txt.bz2

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