Re: [PATCH v4] gdb/hppa-tdep.c: Fix logical working flow issues and check additional store instructions

[Joel Brobecker <brobecker@adacore.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