From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18932 invoked by alias); 14 Dec 2014 19:35:18 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 18907 invoked by uid 89); 14 Dec 2014 19:35:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Sun, 14 Dec 2014 19:35:15 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 78ADB1163CE; Sun, 14 Dec 2014 14:35:13 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id NMkwjXQGRr1N; Sun, 14 Dec 2014 14:35:13 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 6074E1163CD; Sun, 14 Dec 2014 14:35:13 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 293A040164; Sun, 14 Dec 2014 14:35:13 -0500 (EST) Date: Sun, 14 Dec 2014 19:35:00 -0000 From: Joel Brobecker To: Chen Gang Cc: Andreas Schwab , gdb-patches@sourceware.org Subject: Re: [PATCH v4] gdb/hppa-tdep.c: Fix logical working flow issues and check additional store instructions Message-ID: <20141214193513.GT5457@adacore.com> References: <548D93E4.2000405@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <548D93E4.2000405@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-12/txt/msg00357.txt.bz2 > 2014-12-14 Chen Gang > > * hppa-tdep.c (inst_saves_gr): Fix logical working flow issues > and check additional store instructions. Review full disclosure: This is only a spot-check review, since I no longer have much clue nor interest in hppa. I normally find that it's fine to just trust the submitter, but then usually the submitter has a way to actually test the patch, which is not your case IIUC. Regardless, I will trust you that your patch does the right thing (tm)... > --- > gdb/hppa-tdep.c | 111 +++++++++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 85 insertions(+), 26 deletions(-) > > diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c > index 627f31a..4f52e43 100644 > --- a/gdb/hppa-tdep.c > +++ b/gdb/hppa-tdep.c > @@ -1376,37 +1376,96 @@ is_branch (unsigned long inst) > } > > /* Return the register number for a GR which is saved by INST or > - zero it INST does not save a GR. */ > + zero it INST does not save a GR. While changing this, can you change "zero it" -> "zero if". > + Store a word or doubleword using an absolute memory address formed > + using short or long displace- ment or indexed ^^^^^^^^^^^^^^ "displace- ment" -> "displacement". > +static int > +inst_saves_gr (unsigned long inst) > +{ > + switch ((inst >> 26) & 0x0f) > + { > + case 0x03: > + switch ((inst >> 6) & 0x0f) > + { > + case 0x08: > + case 0x09: > + case 0x0a: > + case 0x0b: > + case 0x0c: > + case 0x0d: > + case 0x0e: > + case 0x0f: > + break; > + default: > + return 0; > + } > + /* Fall through */ > + case 0x18: > + case 0x19: > + case 0x1a: > + case 0x1b: > + case 0x1c: > + /* no 0x1d or 0x1e -- according to parisc 2.0 document */ > + case 0x1f: > + return hppa_extract_5R_store (inst); > + default: > + return 0; Quite honestly, I find your code quite confusing because of the fall-through. It's up to you, but wouldn't it be clearer if you had "hppa_extract_5R_store (inst)" instead of "break" + the "/* Fall through */" ? -- Joel