This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v4] gdb/hppa-tdep.c: Fix logical working flow issues and check additional store instructions
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Chen Gang <gang dot chen dot 5i5j at gmail dot com>
- Cc: Andreas Schwab <schwab at suse dot de>, gdb-patches at sourceware dot org
- Date: Sun, 14 Dec 2014 14:35:13 -0500
- Subject: Re: [PATCH v4] gdb/hppa-tdep.c: Fix logical working flow issues and check additional store instructions
- Authentication-results: sourceware.org; auth=none
- References: <548D93E4 dot 2000405 at gmail dot com>
> 2014-12-14 Chen Gang <gang.chen.5i5j@gmail.com>
>
> * 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